Fix reject unknown fields with field label modifier #774
Conversation
* Apply `fieldLabelModifier` to known fields reflected from the
`FieldName` class. While NOT applying the `fieldLabelModofier` to the
encoding tags.
* Change the intermediary type returned by the `FieldName` class from
`Text` to `String` to reduce `{un,}pack` calls to a minimum.
* Update tests which specified the problem before to assert the fixed
semantics.
[fix haskell#773]
| @@ -1260,15 +1260,15 @@ instance (ProductFromJSON arity f, ProductSize f | |||
| -------------------------------------------------------------------------------- | |||
|
|
|||
| class FieldNames f where | |||
| fieldNames :: f a -> [Text] -> [Text] | |||
| fieldNames :: f a -> [String] -> [String] | |||
There was a problem hiding this comment.
I changed the type signature from [Text] to [String] deliberately to be able to call the fieldLabelModifier without a pack / unpack cycle.
changelog.md
Outdated
| @@ -3,6 +3,7 @@ For the latest version of this document, please see [https://github.com/bos/aeso | |||
| #### 1.4.7.1 | |||
|
|
|||
| * GHC 8.10 compatibility, thanks to Ryan Scott. | |||
| * Fix bug in `rejectUnknownFields` not respecting `fieldLabelModifier`. | |||
There was a problem hiding this comment.
I've noted that the changelog entries typically say thanks to $Author but I think I should not thank myself, especially while my code is under review. I hope the reviewer edits it in, if appropriate ;)
There was a problem hiding this comment.
Yeah i always go through this before release, but 1.4.7.1 is already out. We usually put new stuff under an Upcoming header, but I can do that when merging.
| @@ -141,7 +141,7 @@ outputGeneric choice = concat | |||
| (select | |||
| thSomeTypeParseJSONRejectUnknownFields | |||
| gSomeTypeParseJSONRejectUnknownFields) | |||
| [ "{\"tag\": \"record\", \"testOne\": 1.0, \"testZero\": 1}" | |||
| [ "{\"tag\": \"record\", \"testone\": 1.0, \"testZero\": 1}" | |||
There was a problem hiding this comment.
optsRejectUnknownFields actually had the fieldLabelModifier = map toLower set all the time. Hence this test input should have rejected the testOne originally and prior to my bugfix did not.
I chose to not change the expectations in the golden test but instead made the test input comply with the expectation.
| @@ -34,7 +34,7 @@ Error in $: not enough input. Expecting json list value | |||
| SomeType (reject unknown fields) | |||
| Error in $: parsing Types.SomeType(Record) failed, unknown fields: ["testZero"] | |||
| Error in $: parsing Types.SomeType failed, expected Object with key "tag" containing one of ["nullary","unary","product","record","list"], key "tag" not found | |||
| Error in $: parsing Types.SomeType(Record) failed, unknown fields: ["testtwo","testone","testthree"] | |||
| Error in $.testone: parsing Double failed, unexpected Boolean | |||
There was a problem hiding this comment.
The "real" error message was uncovered by my change. The previously reported fields where not unknown as the test case has fieldLabelModifier = map toLower applied.
| @@ -34,7 +34,7 @@ Error in $: not enough input. Expecting json list value | |||
| SomeType (reject unknown fields) | |||
| Error in $: Unknown fields: ["testZero"] | |||
| Error in $: key "tag" not found | |||
| Error in $: Unknown fields: ["testtwo","testone","testthree"] | |||
| Error in $.testone: parsing Double failed, unexpected Boolean | |||
There was a problem hiding this comment.
Same as 70d0927#r419038724 but for TH.
I noticed there where no tests to update to validate the TH bugfix so I created the first commit and extracted it so the reviewer can also see the TH behavior change being reflected in tests.
There was a problem hiding this comment.
For this comment to make sense, look at the 2nd commit in isolation.
mbj
force-pushed
the
fix/reject-unknown-fields-with-field-label-modifier
branch
2 times, most recently
from
865bc29 to
143da4e
Compare
changelog.md
Outdated
| @@ -3,6 +3,7 @@ For the latest version of this document, please see [https://github.com/bos/aeso | |||
| #### 1.4.7.1 | |||
|
|
|||
| * GHC 8.10 compatibility, thanks to Ryan Scott. | |||
| * Fix bug in `rejectUnknownFields` not respecting `fieldLabelModifier`. | |||
There was a problem hiding this comment.
Yeah i always go through this before release, but 1.4.7.1 is already out. We usually put new stuff under an Upcoming header, but I can do that when merging.
|
@bergmark Thanks for the quick review. What is the process after your approval? |
|
Nothing for this PR! @phadej Did you have any other breaking changes you wanted to include in the next release or should we ship 1.5.0.0? |
Any chance we can get this released? Can I help somewhere to make it happen? |
|
@bergmark the scanner rework is something which will force breaking change. Yet, I don't think that will be ready soon.
and then I think 1.5 would be solid. |
Chances we can get 1.4.7.2 meanwhile, which includes this PR? The changelog so far does not list a breaking change, so cutting another 1.4 may be eligible. This way we do not have to wait to get the fix out. |
|
Sorry for the delay @mbj, 1.5.0.0 was released now. We already had some breaking changes that weren't listed in the changelog. Best place to check for things like that is versioning labels on the current milestone, e.g. https://github.com/bos/aeson/milestone/21?closed=1 |
|
@bergmark Thanks for the release and pinging me, much appreciated. Cheers. |
My attempt to fix #773 I reported earlier.
This PR comes in 2 commits. The first adds error message gets for the TH parsing. The 2nd commit fixes the problem I reported with changing the (IMO wrong) error expectations to demonstrate the bugfix.
There are some minor implementation trade-offs. I'll comment in code below to highlight my thoughts on them.