Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RFC] Don't pass attr, obj, data to fields (de)serialize methods #2039

Open
lafrech opened this issue Aug 25, 2022 · 4 comments
Open

[RFC] Don't pass attr, obj, data to fields (de)serialize methods #2039

lafrech opened this issue Aug 25, 2022 · 4 comments

Comments

@lafrech
Copy link
Member

lafrech commented Aug 25, 2022

From a quick look, those are not used by existing fields. I suspect this might be or have been useful in specific fields such as Method.

I can't investigate this thoroughly right now and anyway that would be a breaking change, but opening this here for discussion.

For separation of concerns, (de)ser methods shouldn't need the parent obj/data and field name.

Also, this means that one needs an object and a field name to serialize some data with a field, which may not always be the case. They may pass None but then they don't respect the API (since it won't work with a field (de)ser method that actually relies on those). I faced this specific problem with EnumValue field, when building self.choices, see #2017.

@lafrech lafrech added this to the 4.0 milestone Aug 25, 2022
@lafrech
Copy link
Member Author

lafrech commented Sep 1, 2022

I gave it a quick look.

attr and obj are needed in serialize to get the value from the object.

In _serialize, deserialize and _deserialize, they seem to be only used by Function / Method fields, so removing those would allow us to simplify the implementation.

I guess some users rely on those fields. OTOH, if they didn't exist, I doubt we'd modify (de)ser methods to pass attr and obj just to add them.

I don't have a simple idea to replace those fields, though. pre_load/dump methods don't really answer because we don't want to modify the object. Unless we add some sort of proxy around the object but this sounds complicated.

@lafrech lafrech changed the title Don't pass attr, obj, data to fields (de)serialize methods [RFC] Don't pass attr, obj, data to fields (de)serialize methods Oct 18, 2022
@lafrech
Copy link
Member Author

lafrech commented Oct 18, 2022

An option could be to remove Function and Method fields and add an argument or two to Field to allow the user to pass a custom function/method to get the value from the object. This function/method could be called from get_value. In fact, it would be a get_value override.

So it wouldn't be a field by itself but rather a customization of an existing field. There would be a benefit in terms of auto-documentation, since the type of the value would be known (the user may choose to use Field for a type-free value).

We could add this feature and deprecate Function and Method (as well as the use of attr and obj) in marshmallow 3.x.


I was thinking of opening another issue about moving get_value logic to the schema. The schema would just need to access the attribute attribute of the field. If we do what I describe above, the schema would also need to call the custom accessor (the function/method). It's not obvious to me where the logic should be, but I still find it strange to pass an obj, an accessor function, etc.

@lafrech lafrech pinned this issue Oct 18, 2022
@lafrech
Copy link
Member Author

lafrech commented Oct 18, 2022

We could add accessor parameter to Field.

class Field(FieldABC):
   def __init__(..., accessor,...):
        ...
        self.get_value = accessor
        ...

Allowing the user to do that:

def ser_function(...):
    ...

class MySchema(Schema):

    def ser_method(...):
        ...

    # Deprecated
    old_function = fields.Function(ser_function)
    # Replace with
    new_function = fields.Field(accessor=ser_function)

    # Deprecated
    old_method = fields.Method(ser_method)
    # Replace with
    new_method = fields.Field(accessor=ser_method)

@lafrech
Copy link
Member Author

lafrech commented Oct 18, 2022

I still need to get my head around this. If Field/Method/Constant used serialize rather than _serialize, we could remove the _CHECK_ATTRIBUTE hack.

But then we wouldn't call get_value so the simple solution above wouldn't work.

Also, it is worth noting that fields working at get_value level couldn't be embedded in container fields, as those are meant to pass the value, not the object (according to this issue and #1995 (comment)). Perhaps we can live with this limitation. I don't see the point of using such a field in a container field.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant