add redis #25
add redis #25
Conversation
agave/filters.py
Outdated
| exclude_fields = { | ||
| 'created_before', | ||
| 'created_after', | ||
| 'active', | ||
| 'limit', | ||
| 'page_size', | ||
| 'key', | ||
| } | ||
| fields = query.dict(exclude=exclude_fields) | ||
| fields = {**fields, **kwargs} | ||
| if 'count' in fields: | ||
| del fields['count'] | ||
| if len(filters) == 0: | ||
| filters = fields | ||
| return filters | ||
| return filters |
There was a problem hiding this comment.
Yep, se agrego en una función
agave/models/helpers.py
Outdated
| from base64 import urlsafe_b64encode | ||
| from typing import Any | ||
|
|
||
| from rom import Column, Model, PrimaryKey |
There was a problem hiding this comment.
You can't have this here. It'll break if running on a server without redis
agave/models/helpers.py
Outdated
| def sanitize_item(item: Any) -> Any: | ||
| if isinstance(item, dt.date): | ||
| rv = item.isoformat() | ||
| elif hasattr(item, 'dict'): | ||
| rv = item.dict() | ||
| else: | ||
| rv = item | ||
| return rv |
There was a problem hiding this comment.
@Felipaoo I thought we agreed to use the one from cuenca-validations
There was a problem hiding this comment.
Sip se va a agregar en cuenca-validations, solo lo puse para probar redis, este código no estará aqui.
There was a problem hiding this comment.
Yes, we are going to take the one from cuenca-validations and remove it
There was a problem hiding this comment.
Listo el pr en cuenca-validations cuenca-mx/cuenca-validations#74
| class AccountRedis(RedisModel): | ||
| id = String( | ||
| default=uuid_field('US'), | ||
| required=True, | ||
| unique=True, | ||
| index=True, | ||
| keygen=util.IDENTITY, | ||
| ) | ||
| name = String(required=True, index=True, keygen=util.IDENTITY) | ||
| user_id = String(required=True, index=True, keygen=util.IDENTITY) | ||
| secret = String() | ||
| created_at = DateTime(default=dt.datetime.utcnow, index=True) | ||
| deactivated_at = DateTime(default=DEFAULT_MISSING_DATE, index=True) |
There was a problem hiding this comment.
why is this in agave—instead of authed? Where else is it used?
There was a problem hiding this comment.
Este es código solo lo puse para probar los tests con redis.
There was a problem hiding this comment.
Podemos moverlo a las pruebas entonces?
There was a problem hiding this comment.
Este codigo deberia estar en la ruta 'examples/chalicelib/models/accounts_redis.py' donde actualmente esta, ya que forma parte de los examples con Redis
Codecov Report
@@ Coverage Diff @@
## main #25 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 8 14 +6
Lines 238 326 +88
Branches 38 43 +5
=========================================
+ Hits 238 326 +88
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
felipao-mx
left a comment
felipao-mx
left a comment
There was a problem hiding this comment.
encontré algunos detalles, pero va por buen camino
agave/filters.py
Outdated
| def generic_mongo_query(query: QueryParams) -> Q: | ||
| filters = Q() | ||
| if query.created_before: | ||
| filters &= Q(created_at__lt=query.created_before) | ||
| if query.created_after: | ||
| filters &= Q(created_at__gt=query.created_after) | ||
| fields = exclude_fields(query) | ||
| return filters & Q(**fields) | ||
|
|
||
|
|
||
| def generic_redis_query(query: QueryParams, **kwargs) -> Dict[str, Any]: | ||
| filters: Dict[str, Any] = dict() | ||
| if query.created_before or query.created_after: | ||
| # Restamos o sumamos un microsegundo porque la comparación | ||
| # aquí es inclusiva | ||
| created_at_lt = ( | ||
| query.created_before.replace(tzinfo=None) | ||
| + dt.timedelta(microseconds=-1) | ||
| if query.created_before | ||
| else None | ||
| ) | ||
| created_at_gt = ( | ||
| query.created_after.replace(tzinfo=None) | ||
| + dt.timedelta(microseconds=1) | ||
| if query.created_after | ||
| else None | ||
| ) | ||
| filters['created_at'] = (created_at_gt, created_at_lt) | ||
| fields = exclude_fields(query) | ||
| fields = {**fields, **kwargs} | ||
| if len(filters) == 0: | ||
| filters = fields | ||
| return filters |
There was a problem hiding this comment.
en este bloque de código se tendrán problemas al importar estas funciones cuando un proyecto no tenga instalado mongoengine o rom. Debes aplicar una estructura similar a lo que se hizo con los modelos
agave/blueprints/rest_api.py
Outdated
| cls.model.objects.order_by("-created_at") | ||
| .filter(filters) | ||
| .limit(limit) | ||
| items, items_limit = cls.model.filter_limit( |
agave/blueprints/rest_api.py
Outdated
| data = cls.model.retrieve(cls, id=id) | ||
| if self.user_id_filter_required(): | ||
| id_query = id_query & Q(user_id=self.current_user_id) | ||
| data = cls.model.objects.get(id_query) | ||
| except DoesNotExist: | ||
| data = cls.model.retrieve( | ||
| cls, id=id, user_id=self.current_user_id | ||
| ) |
There was a problem hiding this comment.
puedes simplificar un poco este bloquee de código
There was a problem hiding this comment.
Que exactamente quieres que se simplifique? @Felipaoo
Gleekzone
force-pushed
the
compatible-agave
branch
from
eb88f8c to
414ccd2
Compare
…btener el get en los test
Gleekzone
force-pushed
the
compatible-agave
branch
from
5328249 to
8b13889
Compare
agave/blueprints/rest_api.py
Outdated
| except ValidationError as e: | ||
| return Response(e.json(), status_code=400) | ||
| except DoesNotExist: | ||
| except ObjectDoesNotExist: |
agave/models/mongo/mongo_model.py
Outdated
| return self._dict(mongo_to_dict) | ||
|
|
||
| @classmethod | ||
| def retrieve(cls, id: str, user_id: Optional[str] = None): |
There was a problem hiding this comment.
refer to https://www.python.org/dev/peps/pep-0570/
should be
def retrieve(cls, id: str, *, user_id: Optional[str] = None):This means that it, if user_id is passed in, is _must_ be passed in as a kwarg. retrieve('222', '3333')will be rejected and onlyretrieve('222', user_id='3333')` will be accepted. This forces greater clarity in the code and allows more flexibility if new kwargs are added in the future.
agave/models/mongo/mongo_model.py
Outdated
| query = Q(id=id) | ||
| if user_id: | ||
| query = query & Q(user_id=user_id) |
There was a problem hiding this comment.
this doesn't need to be in the try block. It's impossible for these lines to raise DoesNotExist
agave/models/mongo/mongo_model.py
Outdated
| return id_obj | ||
|
|
||
| @classmethod | ||
| def count(cls, filters: Any) -> int: |
There was a problem hiding this comment.
shouldn't use Any. It's the equivalent of not having type hinting
agave/models/mongo/mongo_model.py
Outdated
|
|
||
| @classmethod | ||
| def has_more(cls, items: Any, limit: int): | ||
| has_more = items.limit(limit + 1).count() > limit |
There was a problem hiding this comment.
don't use the name of the method as a variable name
agave/models/mongo/mongo_model.py
Outdated
| return count | ||
|
|
||
| @classmethod | ||
| def filter_limit(cls, filters: Any, limit: int): |
There was a problem hiding this comment.
should be def all(cls, filters, *, limit=DEFAULT_LIMIT)
agave/models/mongo/mongo_model.py
Outdated
| items = ( | ||
| cls.objects.order_by("-created_at").filter(filters).limit(limit) | ||
| ) | ||
| return items |
There was a problem hiding this comment.
change to: return items, has_more
agave/models/redis/filters.py
Outdated
| filters['created_at'] = (created_at_gt, created_at_lt) | ||
| fields = exclude_fields(query) | ||
| fields = {**fields, **kwargs} | ||
| if len(filters) == 0: |
agave/models/redis/filters.py
Outdated
| return filters | ||
| return filters |
There was a problem hiding this comment.
why are there two return statements?
agave/models/redis/redis_model.py
Outdated
|
|
||
|
|
||
| def redis_to_dit(obj, exclude_fields: list = None) -> dict: | ||
| excluded = ['o_id'] |
There was a problem hiding this comment.
if it's always the same, it should be a module-level constant
agave/models/redis/redis_model.py
Outdated
| return value.decode('utf-8') | ||
|
|
||
|
|
||
| def redis_to_dit(obj, exclude_fields: list = None) -> dict: |
There was a problem hiding this comment.
should use list for type-hinting. It should be List[key_type, value_type]
There was a problem hiding this comment.
@matin en lugar de devolver un dict debería devolver una lista?
agave/models/redis/redis_model.py
Outdated
| return self._dict(redis_to_dit) | ||
|
|
||
| @classmethod | ||
| def retrieve(cls, id: str, user_id: Optional[str] = None): |
There was a problem hiding this comment.
def retrieve(cls, id: str, *, user_id: Optional[str] = None):
| from agave.models.mongo.filters import generic_mongo_query | ||
| from ..models.mongo_models import Account as AccountModel | ||
| from ..validators import AccountQuery, AccountRequest, AccountUpdateRequest | ||
| from .base import app | ||
| from agave.exc import ObjectDoesNotExist |
There was a problem hiding this comment.
agave in this case is a third-party package, which means it should be grouped with other third-party packages vs being grouped with local, relational packages
|
@Felipaoo ¿qué es el estatus de este PR? |
|
quedó pendiente el review de @kerycdiaz |
|
Ouuu no recordaba que tenía que revisar este PR @Felipaoo mañana temprano lo hago vaa |
keryc
left a comment
keryc
left a comment
There was a problem hiding this comment.
Algunas sugerencias y preguntas.
|
|
||
| def _count(filters: Q): | ||
| count = cls.model.objects.filter(filters).count() | ||
| def _count(filters): |
| return dict(count=count) | ||
|
|
||
| def _all(query: QueryParams, filters: Q): | ||
| def _all(query: QueryParams, filters): |
| class RedisModel(BaseModel, Model): | ||
| meta = {'allow_inheritance': True} | ||
| o_id = PrimaryKey() # Para que podamos usar `id` en los modelos | ||
|
|
||
| def dict(self) -> DictStrAny: | ||
| return self._dict(redis_to_dict) |
There was a problem hiding this comment.
Si este modeo es muy similar a MongoModel, podemos tener una clase base no?
There was a problem hiding this comment.
podrías ampliar un poco más tu idea?
Esta clase al igual que MongoModel heredan de BaseModel que es donde se definió los elementos en común. Ves algo más que podamos unificar?
There was a problem hiding this comment.
Si claro
- Ambos tienen el
meta = {'allow_inheritance': True} - El retrieve o unico distinto en ambos es el query en si.
- Incluso el count puede estar en el base si el cls.query y el cls.object tuvieran el mismo nombre.
| if not account: | ||
| raise NotFoundError('Not valid id') |
There was a problem hiding this comment.
Esto es innecesario, si no existe account debe entrar en el except mas arriba.
| @pytest.fixture | ||
| def user_id() -> str: | ||
| return 'US123456789' | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def another_user_id() -> str: | ||
| return 'US987654321' |
There was a problem hiding this comment.
No veo necesario usar un fixture para solo una variablle, puedes colocarla global en e archivo de tests no?
Se elimina código de mongo incluido en rest-api, esto para hacer compatible con redis.
Aun este código no esta listo para ser revisado, hay mucho código que se agrego para pruebas, pero se va a limpiar. Sobre todo para probar redis. Estará como draft, pero pueden ir agregando comentarios si lo desean.