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

Manage wrong data type with unknown!=EXCLUDE #857

Merged

Conversation

@lafrech
Copy link
Member

commented Jun 27, 2018

No description provided.

@lafrech lafrech force-pushed the Nobatek:fix_decorator_none_raise_include branch from 8f0c900 to 0e34326 Jun 27, 2018

@lafrech lafrech changed the title Fix corner case with unknown!=EXCLUDE and decorated processor returns None Fix case with unknown!=EXCLUDE and decorated processor returns None Jun 27, 2018

@lafrech

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

Not really a corner case since RAISE shall become default.

@lafrech lafrech force-pushed the Nobatek:fix_decorator_none_raise_include branch from 5559590 to fdc5188 Jun 27, 2018

@lafrech lafrech added this to the 3.0 milestone Jun 27, 2018

@lafrech

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

Hold this. It is a bit more complicated than I first thought.

It is not only the about the None case but about all wrong data types.

Current approach is to iterate on fields and get keys from input data. If data has no get method, then AttributeError is caught and an error is stored.

There are a few issues with this:

  • When unknown != EXCLUDE, the code that handles unknown keys fails as data is not iterable and set(data) won't work. This is what brought me here in the first place.
  • The error message for this is the "type" error message from the first field that triggers this, while this is a schema error. It does not really make sense IMHO to pick a random Field error message for a schema error.
  • If the schema has no field, no error is triggered. A schema without field might not be an obvious use case, but why not? It could make sense when used with unknown=INCLUDE, or it could be a corner case of dynamically creating Schemas.

I added two commits showing two different approaches to this.

The first catches the TypeError when dealing with unknown fields and skips the treatment. It looks awkward and redundant. And it only addresses the first issue in the list above.

The second checks the type of data before doing anything. It adds a check to the normal execution path rather than relying solely on try/catch, which is not ideal in terms of performance, but not that bad. Also, it works with AttributeError. If data has a get method but is not an iterable, it is bound to fail. This might not be an issue in practice. I mean it will only break if you try hard. And the 'Invalid input type' error message is hardcoded.

This reveals underlying design issues. It is not the simple change I thought. (And I came there while trying to tackle #851...)

Thoughts welcome.

@lafrech

This comment has been minimized.

Copy link
Member Author

commented Jun 27, 2018

In fact, the whole wrong data type case management is smelly from the start:

                errors = self.get_errors(index=index)
                msg = field_obj.error_messages['type'].format(
                    input=data, input_type=data.__class__.__name__,
                )
                self.error_field_names = [SCHEMA]
                errors = self.get_errors()
                errors.setdefault(SCHEMA, []).append(msg)

We're trying to fit a schema error in a mechanism that was meant for field errors. And the logic here is a bit twisted: the first call to get_errors mutates self.errors so it can't be removed, which is not obvious when reading this part of the code.

Perhaps store_error / get_errors need refactoring and should provide a clean interface to store schema errors.

This looks quite like a WWdD issue...

@lafrech lafrech changed the title Fix case with unknown!=EXCLUDE and decorated processor returns None WIP - Manage wrong data type with unknown!=EXCLUDE Jun 27, 2018

@sloria

This comment has been minimized.

Copy link
Member

commented Jul 1, 2018

We're trying to fit a schema error in a mechanism that was meant for field errors.

I agree that it's awkward right now. We should come up with a solution to configure both "invalid type" and "unknown field" (#852) error messages on the Schema class.

the first call to get_errors mutates self.errors so it can't be removed, which is not obvious when reading this part of the code.

Yeah, it's not ideal. Either get_errors should be renamed or we should separate the mutation of self.errors from it.

try:
# Check data is a dict
try:
data.get('dummy', missing)

This comment has been minimized.

Copy link
@sloria

sloria Jul 1, 2018

Member

I think doing if isinstance(data, collections.Mapping) is more appropriate in this case.

data.get('dummy', missing)
except AttributeError:
errors = self.get_errors(index=index)
msg = 'Invalid input type.'

This comment has been minimized.

Copy link
@sloria

sloria Jul 1, 2018

Member

This is fine for now until #852 is addressed. Might as well remove 'type' in Field.default_error_messages while you're at it.

@sloria
Copy link
Member

left a comment

Just a couple suggestions: using isinstance insead of EAFP for checking the data type, and removing type from default error messages.

@lafrech lafrech force-pushed the Nobatek:fix_decorator_none_raise_include branch from fdc5188 to ada43b4 Jul 1, 2018

@lafrech lafrech changed the title WIP - Manage wrong data type with unknown!=EXCLUDE Manage wrong data type with unknown!=EXCLUDE Jul 1, 2018

@lafrech

This comment has been minimized.

Copy link
Member Author

commented Jul 1, 2018

Suggestions applied. Rebased.

@lafrech lafrech referenced this pull request Jul 1, 2018
@sloria
sloria approved these changes Jul 2, 2018

@sloria sloria merged commit 691fbf4 into marshmallow-code:dev Jul 2, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@lafrech lafrech deleted the Nobatek:fix_decorator_none_raise_include branch Jul 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.