Fix propagation of options to Nested containers

[lafrech]

Fixes #946.

With Dict, I figured the nested options would apply to values schema, not keys schema.

[sloria]

Do we need to do the same thing with partial?

[sloria]

Should we also propagate render_module? See #1044

[lafrech]

Yes, probably.

It could be worth factorizing this logic in a ContainerField for all fields with container fields.

[sloria]

@lafrech Will you be doing the ContainerField refactor, or would you like to defer it?

[lafrech]

I'd like to do that while finishing this PR. It shouldn't take more time with the refactor than without. I just didn't take the time to work on the PR yet.

[sloria]

OK, no problem.

[lafrech]

I extended the fix to the Tuple field and proposed a ContainerMixin to factorize the container logic.

The tests only test that the modifier is properly set on the container field, not the actual behaviour.

Indeed, we should extend this to other modifier. However, it looks more complicated than I expected.

At least, all params in Schema __init__ should be passed by Nested:

Schema __init__:

    def __init__(
        self, only=None, exclude=(), many=False, context=None,
        load_only=(), dump_only=(), partial=False, unknown=None,
    ):

Call to Schema in Nested:

                self.__schema = schema_class(
                    many=self.many,
                    only=self.only, exclude=self.exclude, context=context,
                    load_only=self._nested_normalized_option('load_only'),
                    dump_only=self._nested_normalized_option('dump_only'),
                )

    def _nested_normalized_option(self, option_name):
        nested_field = '%s.' % self.name
        return [field.split(nested_field, 1)[1]
                for field in getattr(self.root, option_name, set())
                if field.startswith(nested_field)]

See, only and exclude are stored in Nested, but other parameters must be taken from root. Maybe those should be stored in Nested as well.

Some parameters are not passed to the nested Schema at __init__ but on load:

    def _load(self, value, data, partial=None):
        try:
            valid_data = self.schema.load(
                value, unknown=self.unknown,
                partial=partial,
            )

unknown is stored in Nested and passed on load. partial is not stored in Nested at all, only passed on load.

And then there is the case of params such as render_module that are only passed as Meta option.

I didn't spend much time digging into this, maybe there is a good reason, but from a quick look, it seems modifiers management deserves to be unified.

It does not have to hold this PR. If the implementation is fine, we could merge this and open another issue to track work on what I wrote above.

[sloria]

Drive-by: this name is a bit misleading, since this method actually mutates self.

[lafrech]

Yeah, I'm not really satisfied with those method names.

[deckar01]

ContainerMixin.get_container_modifiers() -> ContainerMixin.set_modifiers()?

ContainerMixin.set_container_modifiers() -> Nested.set_modifiers()?

It seems like Nested should be responsible for setting its own modifiers if other fields are. Making it a no-op on Field and just defining it where needed would avoid the type check and allow propagation between containers (like List(List(Nested(S)))).

[sloria]

Seems like a reasonable approach.

[lafrech]

I pushed a new commit in which I tried to implement something along those lines. I still find it a bit awkward. But maybe I misunderstood your comments.

[sloria]

I think Jared is suggesting to only have set_modifiers which container fields override.

class Field(...):
    def set_modifiers(self):
        # noop

class ContainerMixin(...):

    def set_modifiers(self):
        # sets modifiers on inner fields

I think you can remove get_modifiers.

[lafrech]

And then there is the case of params such as render_module that are only passed as Meta option.

Since those are not passed at Schema init, we can ignore them here. They can be set in a BaseSchema inherited by all schemas in the application.

However, we need to make sure that all parameters that can be passed at Schema init are passed in Nested as well.

[lafrech]

Propagating unknown is discussed here: #980.

[sloria]

Since Python's MRO goes from left to right, ContainerMixin should come before Field.

[sloria]

Just did a quick glance for now; can look more in-depth later.

[sloria]

Once #1066 (comment) and #1066 (comment), this should be good to merge.

[lafrech]

Rebased. Fixed MRO issue.

I still don't get #1066 (comment).

[sloria]

Disregard #1066 (comment), actually. I wasn't fully understanding how this was working. I think your refactor in e48cf5a will suffice.

How about renaming ContainerMixin.get_container_modifiers() to ContainerMixin.init_container_modifiers?

After reviewing this, it occurs to me that the container attribute is a misnomer (copy pasta from Flask-RESTful). We should probably rename it to something like inner. That can happen in a different PR, though.

[lafrech]

How about renaming ContainerMixin.get_container_modifiers() to ContainerMixin.init_container_modifiers?

Done.

After reviewing this, it occurs to me that the container attribute is a misnomer (copy pasta from Flask-RESTful). We should probably rename it to something like inner. That can happen in a different PR, though.

Done.

I'm still unsure about #1066 (comment). I'd like to be sure adding other modifiers wouldn't break the design before merging this.

[sloria]

Great! Go ahead and merge whenever you feel confident in this.

[lafrech]

Do we need to do the same thing with partial?

partial does not need to be propagated the same way because it is propagated in Schema._deserialize (because it can be passed as parameter inSchema.load). Wrong. See below.

[lafrech]

Parameter	Type	Set at	Dot delimiter	Propagation mechanism	Nested parameter
only	tuple	init	Yes	From Schema (_normalize_nested_options)	Yes
exclude	tuple	init	Yes	From Schema (_normalize_nested_options)	Yes
load_only	tuple	init	Yes	From Nested (_nested_normalized_option)	No
dump_only	tuple	init	Yes	From Nested (_nested_normalized_option)	No
partial	tuple / boolean	init / load	Yes	From Schema (_deserialize)	Yes if #1227
unknown	string	init / load	-	Not propagated (#980)	Yes

unknown is a special case: it is not propagated. partial is also a special case: it can be set at load time.

The issue with load_only/dump_only is that they can be schema attributes (tuples of field names) or field attributes (booleans). Due to this, they can't be passed to Nested.__init__ like only/exclude and they can't be set in Schema._normalize_nested_options. It is a pity because they must be set in Nested._nested_normalized_option by fetching stuff from parent Schema using the root attribute (related to #1046). This is something we may want to address in MA 4.x. Meanwhile, I don't see a nice and straightforward way to make the two propagation methods converge.

Since only / exclude are the only parameters concerned with the fix in this PR, and since I don't find that ContainerMixin that elegant after all, I'd like to try and provide another PR without that factorization. And with better tests (also tests other modifiers propagation, and perhaps test the behaviour, not the parameter passed).

[lafrech]

One way to solve the issue with load_only/dump_only schema/field parameters ambiguity would be to decide that Nested does not pass parameters to its schema and nested schemas can only be parametrized by passing instances.

# Bad
nested = Nested(Schema, only=...)
# Good
nested = Nested(Schema(only=...))

This respects the "only one way to do one thing" principle. But it prevents from passing modifiers to a schema passed as string.

Perhaps just passing schema parameters in a dedicated dict. It doesn't look that good, but it would be required only for schemas passed as strings.

nested_as_instance = Nested(Schema(only=...))
nested_as_string = Nested('Schema', nested_kwargs={'only':...})

Edit: In this comment and below, @deckar01 proposes just the opposite (only pass classes, not instances). In any case, having only one way to support parametrizing nested schemas would make things easier.

[lafrech]

I'm not satisfied with the ContainerMixin. Besides, since only two parameters are passed this way, the factorization is not that useful. I submitted another PR with a simple changeset that does the job: #1229.

Things we may want to do later on but are beyond the scope:

• Rename container in to inner. Can be done quickly after Fix propagation of options to Nested containers #1229 is merged.
• Harmonize the way the modifiers can be passed (as Nested parameters, as Meta params, at load time, in schema init, as string with dot delimiter, etc.).
• Merge Schema._normalize_nested_options and Nested._nested_normalized_option into a single method.

The two latter may be postponed to MA 4.

[lafrech]

Do we need to do the same thing with partial?

partial does not need to be propagated the same way because it is propagated in Schema._deserialize (because it can be passed as parameter inSchema.load).

This is absolutely wrong.

There's a test case to prove it in #779 (comment).

Draft PR: #1230

[sloria]

Let's go with #1229 .