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

WIP: Parse dont validate #1140

Closed
wants to merge 6 commits into from
Closed

Conversation

dtkav
Copy link
Collaborator

@dtkav dtkav commented Jan 25, 2020

Changes proposed in this pull request:

  • move type coercion into one place (with validation)
  • move sanitization to just before populating function arguments to avoid over-matching (like "$abc" and "abc$")
  • refactor + simplify notorious _get_body_argument() function.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 96.28% when pulling 4e63c82 on dtkav:parse_dont_validate into 4b93890 on zalando:master.

Copy link
Contributor

@cognifloyd cognifloyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this is WIP. I'm excited to see you separating parsing and validation. Your refactored code is much easier to follow.
One question about additionalProperites (it might just be not implemented yet because this PR is WIP). So, if this helps great. if not, just ignore. :)

for k, v in d.items():
if k in schema['properties']:
d[k] = cast_leaves(v, schema['properties'][k])
elif schema.get('additionalProperites'):
Copy link
Contributor

@cognifloyd cognifloyd Jan 29, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does this handle an explicit additionalProperties: true?
It looks like it calls cast_leaves -> coerce_type so param ends up as a True bool, so param.get("schema", param) (line 54) should fail.
If additionalProperties is not defined or None (ie the default value of True applies), then this skips handling it which is different than an explict true.

Suggested change
elif schema.get('additionalProperites'):
elif schema.get('additionalProperites') and not isinstance(schema.get('additionalProperties'), bool):

But that doesn't handle cases where additionalProperties are just allowed, and no schema for them is defined.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also spelling - 'additionalProperites' --> 'additionalProperties'

@RobbeSneyders
Copy link
Member

This split is now done as part of #1489.

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

Successfully merging this pull request may close these issues.

5 participants