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.

Sign up for GitHub

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

Jump to bottom

Feature/strict schema #169

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Feature/strict schema #169

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
6e5bce9
Add constructor to ResponseSchema
Sytten Feb 8, 2020
01b0e8f
Add StrictSchema
Sytten Feb 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions flask_rebar/validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,13 @@ def disallow_extra_fields(self, processed_data, original_data):
RequestSchema = Schema


# Define a constructor to please some IDE type checking
class ResponseSchema(ActuallyRequireOnDumpMixin, Schema):
def __init__(self, **kwargs):
super().__init__(**kwargs)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Function call overhead in python is significant. It’s best practice not to introduce this extra no-op function call that reduces performance just for some IDEs that aren’t currently finding the inherited method. Can you follow up with your IDE vendor instead of adding this here?



class StrictSchema(RequestSchema, ResponseSchema):
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • Better to inherit the mixin classes directly
  • I wonder if there’s a better name than StrictSchema for this, since strict is overloaded with other meaning in Marshmallow v2 (complicated by how Rebar currently already always does a strict load with unstrict Marshmallow v2 schemas, i.e. raising rather than returning a value with errors in it)

pass
Copy link
Collaborator

Choose a reason for hiding this comment

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

Missing docstring for public class.

Once added, the pass will be unnecessary.



Expand Down
26 changes: 25 additions & 1 deletion tests/test_validation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
from flask_rebar import messages
from flask_rebar.utils.request_utils import normalize_schema
from tests.helpers import skip_if_marshmallow_not_v2
from flask_rebar.validation import ActuallyRequireOnDumpMixin
from flask_rebar.validation import ActuallyRequireOnDumpMixin, StrictSchema
from flask_rebar.validation import CommaSeparatedList
from flask_rebar.validation import DisallowExtraFieldsMixin
from flask_rebar.validation import QueryParamList
Expand Down Expand Up @@ -117,6 +117,30 @@ def test_required_failed_validate(self):
self.assertIn("one_of_validation", ctx.exception.messages)


class ActuallyStrictSchema(StrictSchema):
Copy link
Collaborator

Choose a reason for hiding this comment

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

TestStrictSchema or MyStrictSchema? Shout out to #170.

optional = fields.Str()
required = fields.Str(required=True, allow_none=False)


class TestStrictSchema(TestCase):
def test_load_nominal(self):
ActuallyStrictSchema().load({"optional": "foo", "required": "bar"})

def test_load_unexpected_field(self):
data, errs = ActuallyStrictSchema().load(
{"optional": "foo", "required": "bar", "foo": "bar"}
)
self.assertEqual(errs, {"_schema": [messages.unsupported_fields(["foo"])]})

def test_dump_nominal(self):
ActuallyStrictSchema().dump({"optional": "foo", "required": "bar"})

def test_dump_missing_required(self):
with self.assertRaises(ValidationError) as ctx:
compat.dump(ActuallyStrictSchema(), {"optional": "foo"})
self.assertIn("required", ctx.exception.messages)


class StringList(Schema):
foos = CommaSeparatedList(fields.String())

Expand Down