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

Add an official way to ignore model fields #26

Open
lvoliveira opened this issue Oct 7, 2020 · 8 comments
Open

Add an official way to ignore model fields #26

lvoliveira opened this issue Oct 7, 2020 · 8 comments

Comments

@lvoliveira
Copy link
Contributor

lvoliveira commented Oct 7, 2020

Currently, all the fields of the model are serialized by default. It is possible to restrict the direction of the serialization with the Field parameters load_only and dump_only.

There is a hack to get a field ignored by setting both load_only and dump_only to true.

class MySerializer(ModelSerializer):
    field_to_ignore = Field(load_only=True, dump_only=True)

But would be nice to have o more official and elegant way to do it. I had some ideas

Opition 1: Add a new flag to Field to make that field ignored

class MySerializer(ModelSerializer):
    field_to_ignore = Field(ignore=True)

Opition 2: add meta information about the fields to ignore

class MySerializer(ModelSerializer):
   _ignore_fields_ = ['field_to_ignore', 'another_field_to_ignore']
   
   ...
@nicoddemus
Copy link
Member

The "read-only", "write-only", and "ignore" are actually the same option, as the hack "load_only=True/dump_only=True" suggests.

How about:

class MySerializer(ModelSerializer):
    field = Field(mode="load-only")

With mode being "load-only", "dump-only", and "ignore"?

@lvoliveira
Copy link
Contributor Author

Sounds reasonable, since this would solve the inconsistency of having more the one of this options set to True. The only thing I personally don't like is having to remember the possible values for the mode parameter.

Another option would be to have specific field classes for each of these behaviors

class MySerializer(ModelSerializer):
    dump_only_field = DumpField()
    load_only_field = LoadField()
    field_to_ignore = IgnoreField()

@nicoddemus
Copy link
Member

nicoddemus commented Oct 7, 2020

Indeed. The mode however gives the possibility to include more options in the future.

Well let's see what @igortg has to say. 👍

@nobreconfrade
Copy link
Contributor

nobreconfrade commented Oct 7, 2020

I personally think it's reasonable to have a ignore=True|False flag.

It's simple to check the function interface and if I see this option, I automatically think the field with it will not be serialized either way. It even comes with the plus of being easier to implement.

@igortg
Copy link
Contributor

igortg commented Oct 7, 2020

Maybe we can fix that by solving a bad architectural decision (IMHO) we did when creating the library: by default, ModelSerializer dump/load all fields of the model, even the ones not explicitly declared. It's somewhat counter intuitive that you must declare a field that you want ignore.

My suggestion is changing the default behavior of ModelSerializer to only serialize fields that are explicitly declared. Then we create a decorator (e.g., @autofields) that would reproduce the current behavior. This would be more in accord to the "explicit is better than implicit" rule.

This would not completely solve the problem... we may still have to set Field(load_only=True, dump_only=True) in some particular cases.

Please, feel free to argue that I'm going "to far from the scope".

@lvoliveira
Copy link
Contributor Author

@igortg I agree with you, but as you spotted out, your solution may not solve the problem of the Field(load_only=True, dump_only=True). So, I think we could continue with the discussion on that on another issue and get a solution to the original problem here.

@lincrosenbach
Copy link
Contributor

Agree with @lvoliveira and @nobreconfrade. ignore=True|False (default=False) would be enough. @igortg explicit declare a field to ignored is good IMHO, first when not declared warnings are thrown (it is a sign you forgot something)... and to say explicitly to ignore you know what you are doing. @nicoddemus I doubt it will have more modes. Write only, read only, both or None.
Looking on other solutions for other frameworks is basically the same (example in Django Rest Framework):
https://www.django-rest-framework.org/api-guide/fields/
Instead of ignore field, there is the required field.

@igortg
Copy link
Contributor

igortg commented Oct 8, 2020

@igortg I agree with you, but as you spotted out, your solution may not solve the problem of the Field(load_only=True, dump_only=True). So, I think we could continue with the discussion on that on another issue and get a solution to the original problem here.

Ok. So you can go with the simplest solution: field_to_ignore = Field(ignore=True). Considering we'd implement the @autofield strategy in the future, let's do what would have the smallest impact.

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

No branches or pull requests

5 participants