Forbid extra fields when structuring dictionaries into attrs classes #142

bkurtz · 2021-04-02T05:43:49Z

Would resolve #101. I'm assuming this will need some further work (e.g. new tests; fixing other tests this breaks, etc), but wanted to go ahead and get it up for a first review to see if the approach is reasonable.

codecov · 2021-04-02T05:58:09Z

Codecov Report

Merging #142 (fea3292) into master (387ba5d) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #142      +/-   ##
==========================================
+ Coverage   98.21%   98.22%   +0.01%     
==========================================
  Files           6        6              
  Lines         615      620       +5     
==========================================
+ Hits          604      609       +5     
  Misses         11       11

Impacted Files	Coverage Δ
src/cattr/converters.py	`99.23% <100.00%> (+<0.01%)`	⬆️
src/cattr/gen.py	`95.62% <100.00%> (+0.11%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 387ba5d...fea3292. Read the comment docs.

bkurtz · 2021-04-02T14:41:50Z

src/cattr/gen.py

            )
    lines.append("    }")
+    if forbid_extra_fields:
+        post_lines.append("  allowed_fields = {")


Wasn't sure how much of a (non-)goal it is to have tidy generated code. For example, I could probably have written

post_lines.append(f" allowed_fields = {set(a.name for a in attrs)}")

which is shorter and maybe a little cleaner from the point of view of this function, but would likely lead to a very long line in the generated code in many cases.

Tinche

Hello,

thanks for your work on this, let's see about tidying this up and getting it into the next release.

I would also possibly add a parameter for the GenConverter, similar to omit_if_default, that would be used for generating all classes here. (At this level, it'd be OK for the flag to be called just forbid_extra_keys.)

Tinche · 2021-04-02T16:45:53Z

src/cattr/gen.py


-def make_dict_structure_fn(cl: Type, converter, **kwargs):
+def make_dict_structure_fn(
+    cl: Type, converter, forbid_extra_fields: bool = False, **kwargs


The argument at this layer should be a little obfuscated, since it might shadow an attribute name you're trying to override (in **kwargs). I'd change it to _cattrs_forbid_extra_fields. Note that we can add this flag on the GenConverter level with a better name.

Ah, I wondered about that. Will do.

src/cattr/gen.py

Tinche · 2021-04-02T16:58:07Z

src/cattr/gen.py

+            "  }",
+            "  unknown_fields = set(o.keys()) - allowed_fields",
+            "  if unknown_fields:",
+            "    raise KeyError(",


I wouldn't use a KeyError here. First of all, KeyError is raised when a key is missing from a dictionary, not when it's extra :)

Second, KeyError is a built-in Python exception. User libraries are supposed either throw Exception or something inheriting from Exception (see https://docs.python.org/3/library/exceptions.html#Exception). Ideally we would have a StructureError and you could inherit from that, but we don't currently and that should be a different task, so just throw Exception for now.

Hm, I think KeyError counts as "something inheriting from Exception", but agreed that custom exception subclasses would be best, and that a KeyError is a little weird.

try: raise KeyError('key error') except Exception: print("caught your key error") # prints caught your key error

I was thinking of it as the key being missing from the class, rather than from the dictionary. But I'm happy to put in Exception for now if that's your preference!

Yeah, let's switch to a bare Exception for now. KeyError is a bit of a stretch I think.

bkurtz · 2021-04-03T04:39:57Z

While working on this round of updates, I noticed that you'd suggested forbid_extra_keys as the GenConverter option and _cattr_forbid_extra_fields as the argument to make_dict_structure_fn. I figured that they should probably match (fields vs keys), and I felt sorta like "keys" was better, so I went with that, but happy to revert it if you prefer the other way.

Started thinking about tests a little tonight, but would probably appreciate some feedback on what sort of tests are desired here:

I assume there should be at least one test of the updated make_dict_structure_fn directly; should it be
- a one-off test to show that an error is raised in a single case where there's an extra field (fast)
- property testing with hypothesis to show that this works on a large variety of cases, e.g. add a character to a field name, double the field name, etc. (slower, probably harder to write a good test)
In either case, I'm guessing this test would live in test_dict_generation.py?
I assume there should be at least one test of using the setting with a GenConverter object as well, but it wasn't as clear to me where this one should live.

bkurtz · 2021-04-03T04:40:34Z

PS. Thanks for the quick turnaround with a review last night!

Tinche · 2021-04-04T00:33:31Z

While working on this round of updates, I noticed that you'd suggested forbid_extra_keys as the GenConverter option and _cattr_forbid_extra_fields as the argument to make_dict_structure_fn. I figured that they should probably match (fields vs keys), and I felt sorta like "keys" was better, so I went with that, but happy to revert it if you prefer the other way.

Er yeah, keys is better.

Started thinking about tests a little tonight, but would probably appreciate some feedback on what sort of tests are desired here:

* I assume there should be at least one test of the updated `make_dict_structure_fn` directly; should it be
  
  * a one-off test to show that an error is raised in a single case where there's an extra field (fast)
  * property testing with hypothesis to show that this works on a large variety of cases, e.g. add a character to a field name, double the field name, etc.  (slower, probably harder to write a good test)
  
  In either case, I'm guessing this test would live in `test_dict_generation.py`?

If you don't have Hypothesis experience it's ok to skip. Yeah, I'd just add a test or two to test_dict_generation. Perfectly fine for them to be simple.

* I assume there should be at least one test of using the setting with a `GenConverter` object as well, but it wasn't as clear to me where this one should live.

That would be great, yeah. You can add a test to metadata/test_genconverter.py.

Thanks!

bkurtz · 2021-04-08T05:42:30Z

Added some tests. Ended up feeling lazy tonight about tests for make_dict_structure_fn since GenConverter exercises that anyway, but can come back and add those another night if you think it'd be good to have both.

Started on docs, but not done there.

Tinche · 2021-04-08T23:39:31Z

Cool, no rush. Thanks!

bkurtz · 2021-04-10T15:20:38Z

Okay, I think I'd be ready to call this done. Let me know if there are other changes you'd like to see.

bkurtz · 2021-04-10T15:36:34Z

Also: I'm accustomed to a merge/squash workflow to keep git history tidy, but have been assuming it would be appropriate to keep the history as-is to make reviewing easy. LMK if/when it's appropriate to squash.

Tinche

Great job! Left a comment inline.

Also, to save me from opening up a PR to your PR, can you just apply this change:

 if _cattrs_forbid_extra_keys:
        allowed_fields = {a.name for a in attrs}
        globs["__c_a"] = allowed_fields
        post_lines += [
            "  unknown_fields = set(o.keys()) - __c_a",
            "  if unknown_fields:",
            "    raise Exception(",
            f"      'Extra fields in constructor for {cl_name}: ' + ', '.join(unknown_fields)"
            "    )",
        ]

(i.e. precalculate allowed_fields and make it available in the globals).

Tinche · 2021-04-11T22:49:35Z

tests/metadata/test_genconverter.py

+
+
+@given(simple_typed_attrs(defaults=True), unstructure_strats)
+def test_simple_roundtrip_defaults_with_extra_keys_forbidden(


Hm, not sure what this test is supposed to cover that the test before (test_simple_roundtrip_with_extra_keys_forbidden) doesn't? If nothing important, feel free to remove (Hypothesis tests take some time to run, would like to keep their number down).

I think what I had in mind at the time was to make sure that it hadn't somehow made it error in the wrong order (i.e. when there are keys present in the structured object that aren't present in the dict), but given the current implementation I think that's not really a concern, and test_forbid_extra_keys_nested_override would probably catch that in a simple case anyway, so I've gone ahead and deleted. Definitely nice to avoid having too many tests with hypothesis - can really increase the testing time.

bkurtz · 2021-04-12T04:10:56Z

Should be set I think. Pre-calculating the allowed fields is a nice touch - thanks for that pointer.
Let me know if I should squash changes or anything like that.

Tinche · 2021-04-12T09:58:58Z

I don't really have a policy for squashing, feel free if you feel like it. I will take another look at this in the next day or two and maybe merge it it. Note that GitHub's telling me this branch cannot be rebased to due conflicts for some reason

Default is to maintain the old behavior. - Add `_cattr_forbid_extra_keys` arg to `make_dict_structure_fn` and implement new behavior - Update `GenConverter` with `forbid_extra_keys` option to enable this for all classes - Add tests and update docs

bkurtz · 2021-04-13T16:07:03Z

Squashed.

Tinche · 2021-04-14T22:43:56Z

Great works, thanks a lot!

bkurtz commented Apr 2, 2021

View reviewed changes

Tinche self-requested a review April 2, 2021 16:43

Tinche requested changes Apr 2, 2021

View reviewed changes

Tinche requested changes Apr 12, 2021

View reviewed changes

bkurtz force-pushed the forbid_extra_fields branch from 5982028 to fea3292 Compare April 13, 2021 16:05

Tinche merged commit 3850539 into python-attrs:master Apr 14, 2021



		@given(simple_typed_attrs(defaults=True), unstructure_strats)
		def test_simple_roundtrip_defaults_with_extra_keys_forbidden(

Uh oh!

Forbid extra fields when structuring dictionaries into attrs classes #142

Forbid extra fields when structuring dictionaries into attrs classes #142

Uh oh!

Conversation

bkurtz commented Apr 2, 2021

Uh oh!

codecov bot commented Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

bkurtz Apr 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tinche left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tinche Apr 2, 2021

Choose a reason for hiding this comment

Uh oh!

bkurtz Apr 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Tinche Apr 2, 2021

Choose a reason for hiding this comment

Uh oh!

bkurtz Apr 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tinche Apr 4, 2021

Choose a reason for hiding this comment

Uh oh!

bkurtz commented Apr 3, 2021

Uh oh!

bkurtz commented Apr 3, 2021

Uh oh!

Tinche commented Apr 4, 2021

Uh oh!

bkurtz commented Apr 8, 2021

Uh oh!

Tinche commented Apr 8, 2021

Uh oh!

bkurtz commented Apr 10, 2021

Uh oh!

bkurtz commented Apr 10, 2021

Uh oh!

Tinche left a comment

Choose a reason for hiding this comment

Uh oh!

Tinche Apr 11, 2021

Choose a reason for hiding this comment

Uh oh!

bkurtz Apr 12, 2021

Choose a reason for hiding this comment

Uh oh!

bkurtz commented Apr 12, 2021

Uh oh!

Tinche commented Apr 12, 2021

Uh oh!

bkurtz commented Apr 13, 2021

Uh oh!

Tinche commented Apr 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Apr 2, 2021 •

edited

Loading

bkurtz Apr 2, 2021 •

edited

Loading

Tinche left a comment •

edited

Loading

bkurtz Apr 3, 2021 •

edited

Loading