umongo icon indicating copy to clipboard operation
umongo copied to clipboard

Useless reference_cls in ReferenceField

Open suconakh opened this issue 3 years ago • 1 comments

I would like to implement my own Reference class with a custom fetch method and i've noticed that ReferenceField has a reference_cls argument. I have tried to use it with my own Reference inheriting from MotorAsyncIOReference, but it didn't work well. By looking at the ReferenceField sources i noticed that the field itself actually uses a new reference_cls. I've also looked at umongo.frameworks.motor_asyncio and found out that MotorAsyncIOReference is hardcoded in MotorAsyncIOBuilder._patch_field. Is there a reason for that? https://github.com/Scille/umongo/blob/7d199dbc7ab04de174b4465a12fd6b977a4f1976/umongo/frameworks/motor_asyncio.py#L431-L433 I am not familiar with the code and the library development in general but could this be fixed by doing something like this?

if isinstance(field, ReferenceField): 
    field.io_validate.append(_reference_io_validate) 
    if isinstance(field.reference_cls, umongo.data_objects.Reference):
        field.reference_cls = MotorAsyncIOReference

Or perhaps we can get rid of umongo.data_objects.Reference as a default value for reference_cls in ReferenceField constructor and only set default framework reference if user did not set his own.

# umongo.fields
class ReferenceField(BaseField, ma_bonus_fields.Reference):
    def __init__(self, document, *args, reference_cls=None, **kwargs):
        ...

# umongo.frameworks.motor_asyncio
if isinstance(field, ReferenceField):
    field.io_validate.append(_reference_io_validate)
    if field.reference_cls is not None:
        # Additional check for compatibility?
        if not issubclass(field.reference_cls, MotorAsyncIOReference):
            raise IncompatibleReference
    else:
        field.reference_cls = MotorAsyncIOReference

I came up with this solution and i am pretty sure there is more elegant way to do it. Obviously, this should be done for every framework

suconakh avatar May 02 '21 09:05 suconakh

I didn't take the time to investigate much but from what you're writing here, we could do what you propose.

Use None as default and override in framework if None. I wouldn't even bother adding the compatibility check. Modifying this is an advanced feature and the user should know what he's doing.

lafrech avatar May 02 '21 20:05 lafrech