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

(Improvement) Validate required fields #160

Closed

Conversation

dmegbert
Copy link

What does this PR do?

This adds a validator, validate_required_fields, that checks to see if fields that are blank=False or empty_strings_allowed=False have values.

What issue does this solve?

It attempts to solve #123. Prior to this PR you could save empty strings to the database for fields that had blank=False.

Where should the reviewer start?

Begin by looking at the validate_required_fields function. I am not sure I captured all the edge cases. I am filtering out fields that were auto_created or have a default. Not sure if there are others I missed. I'm not certain auto_created necessarily

What Worf gif best describes this PR or how it makes you feel?

I am scared that I broke something!

worfLight

Checklist

  • This PR increases test coverage
  • This PR includes README updates reflecting any new features/improvements to the framework

@dmegbert dmegbert requested a review from wadewilliams as a code owner March 26, 2023 19:31
@stevelacey stevelacey force-pushed the master branch 2 times, most recently from bef1e26 to 626898d Compare March 30, 2023 10:10
@stevelacey stevelacey changed the title Improvement/validate required fields (Improvement) Validate required fields May 17, 2023
@stevelacey
Copy link
Contributor

stevelacey commented May 17, 2023

@dmegbert the approach here is good, if you ever want to get this merged you'll want to run guild's tests against it first, there are instructions in guild for using ./worf locally, there are a few fails, and some problems that will need addressing:

  • What should this do on an update since the fields could already be set? Ignore? Only validate on create by default?
  • What about edge cases where the default behavior doesn't do what the developer wants; perhaps supporting a required_fields = [] attribute on the view so the list of fields can be customized explicitly is a good idea

Refactoring deserialization in worf might also be the way to go here, currently marshmallow is not being used for that aside from validating that only writable fields are passed; when really it could be doing both type and required field checking.

@dmegbert
Copy link
Author

@stevelacey - I think I should probably just close this PR.

Refactoring deserialization in worf might also be the way to go here, currently marshmallow is not being used for that aside from validating that only writable fields are passed; when really it could be doing both type and required field checking.

This definitely feels like the direction to take for deserialization work. One of marshmallow's main features is deserialization with validation capabilities. Seems like leveraging it in a similar spirit with how marshmallow is combined with some django model things for serialization could be really helpful. This might be especially important for anyone looking to use worf for APIs that service external users where validation needs are usually more important.

@dmegbert dmegbert closed this May 17, 2023
@dmegbert dmegbert deleted the improvement/validate-required-fields branch May 17, 2023 11:53
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.

2 participants