dump_only whitelist/inverse / load_only whilst including when dumping #453
Comments
I see what you mean with
A good way to illustrate why this is a strange interface is to create a schema with a field that is both
A clearer terminology might be "force" and "skip" which have clear connotations and no implicit behaviors.
The only ambiguity is priority, but I believe that in the rest of the library exclusion has priority over inclusion and instances have priority over classes. |
I see exactly what you're saying, and I understand the functionality of using Whilst better names such as |
No, those options wouldn't solve the issue of whitelisting deserialized fields. The option currently used to whitelist a set of fields on a schema is
It looks like partial loading (the deserializable_fields = ('fieldname',)
...
data, errors = schema.load(partial=deserializable_fields) If the white list really needs to be declared on a schema class or instance, maybe a separate schema class should be declared. |
@deckar01 Interesting comment. I feel the same about the Looks like we're bearing the weight of history. Starting from scratch, we'd use better terms, like These are features so common that there must be a lot of those in the codes using Marshmallow, and the benefit for changing this is minimal. You get struck by this once, then you learn and you move on. Yet, is there any reason (apart from the time it takes to do it...) not to propose new keywords, and slowly deprecate the old ones? |
What does "force" actually mean? If the field is not marked with "required", will "force" flag force to be required? I don't get why people are so concerned about naming and claim that it is used a lot. It should not. Most common - "password" is load_only, "id" - dump_only. If you have use cases that require you defining complex dependencies and load/dump schemas, you should really consider splitting them into a separate schemas (maybe with reusing common parts through base schema). That's it. PS I think load_only / dump_only semantics are OK. |
Just to clarify, there are two points at stake here:
Regarding the second point, there are two potentially surprising things about those names: As @nickw444 said in OP
And as @deckar01 said
I found this a bit surprising and so did my colleagues. As I said, it is no big deal. You get hit once, and then you understand and you know. It might not be worth the pain of changing load_only/dump_only in the whole library (and all the codes using it). But since it is a breaking change (mitigated by keeping old terms working although considered deprecated), the release of v3 would be a good time to do it. I don't mean to argue about this. I don't really mind, that was just a suggestion. And the "not worth the pain" argument is perfectly relevant. |
I agree with one thing: "load_only"/"dump_only" feilds in Meta class IS indeed confusing. It would be much easier to specify those flags directly in a field definition: class UserSchema(m.Schema):
id = mf.String(dump_only=True)
email = mf.String(required=True)
password = mf.String(load_only=True) So, in my understanding the Meta fields are mostly used when you're integrating with third-party schema system (e.g. SqlAlchemy), you want to automatically import all their stuff and then customize it further. So, I would say that this support should have never existed in the core library in the first place and let integration libraries handle all those customizations. |
I don't plan on adding a whitelist feature because it would be redundant and potentially confusing API. I think the OP's use case can be solved with a base class that uses the from marshmallow import Schema, fields
class ReadOnlySchema(Schema):
"""Schema that every field dump-only if it doesn't explicitly set load_only=True.
"""
def on_bind_field(self, field_name, field_obj):
# Any field that is not load-only will be set as dump-only
if not field_obj.load_only:
field_obj.dump_only = True
class MySchema(ReadOnlySchema):
id = fields.Str()
password = fields.Str(load_only=True)
schema = MySchema()
assert schema.fields['id'].dump_only is True
assert schema.fields['password'].load_only is True
assert schema.load({'id': 'ignored', 'password': 'secret'}).data == {'password': 'secret'}
assert schema.dump({'id': 'abc12', 'password': 'ignored'}).data == {'id': 'abc12'} As for the names Also, I generally try to avoid parameter names that have a negative word in them, e.g. "skip", because double negatives ( Closing for now. We can reopen if further discussion is necessary. |
I feel like this kind of touches on #375, but that didn't seem to align with the behaviour that I think many would find useful. Also highly related to #61
It would be useful to use the meta class (or constructor) to be able to specify a set of fields that are whitelisted for deserialisation. If the field doesn't exist in the iterable, it cannot be deserialised.
If a user of marshmallow had a large schema class, with many fields, but only wanted 1 of those fields to be deserializable/writable, from my understanding they could either:
dump_only
attribute (lots of fields, prone to errors, ie missing a field)only=('fieldname',)
upon instantiating the schema. (prone to errors, forgetting to put this in when instantiating)Am I missing something, or is this a shortcoming that could be addressed?
Also to note, the naming of
load_only
feels like a misnomer without reading the docs in detail - If you specify a field in load_only, it will only be available for loading, not dumping - but taking another perspective to this could imply that it's an iterable of fields that are whitelisted to be loaded, but can still be dumped.The text was updated successfully, but these errors were encountered: