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

Revamp handling of fields without declared field #809

Closed
taion opened this Issue May 15, 2018 · 0 comments

Comments

Projects
None yet
2 participants
@taion
Contributor

taion commented May 15, 2018

This is a follow-up from #783 (comment).

Currently, there's a fair amount of complexity in updating self.fields on Schema.

The current logic is quite odd and has a number of edge cases. It looks a little bit like:

  1. When dumping
    1. Check if the object type is in _types_seen
    2. If it's not in _types_seen, infer the field types for anything in e.g. Schema.Meta.fields but without a declared field from the types on the object to be dumped
    3. If the object is not of a mapping type, add its type to _types_seen
      1. Note that this includes list, which is what we get whenever dumping with many=True
  2. When loading
    1. Just use whatever is in self.fields from the last run
  3. For fields.Nested
    1. Only calculate fields once for every instance of fields.Nested

Assuming my understanding is correct, this leads to a number of weird things happening.

For example, if I try to dump an object of type Foo, then try to dump a dict, then try to dump another object of type Foo, the fields used will be the ones corresponding to the dict.

Likewise, if I keep trying to dump additional dicts, the fields well get recalculated every time. However, this won't happen if I try to repeatedly dump lists with many=True.

But when using a fields.Nested, the field inference will only happen once.

Additionally, this is all potentially non-thread-safe.


This is all quite complicated. My proposal for a simpler option is to add a new fields.Default that has the relevant logic, and instead only infer the fields once at schema instantiation time.

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