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

Fix reject unknown fields with field label modifier #774

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
2 changes: 1 addition & 1 deletion Data/Aeson/TH.hs
Original file line number Diff line number Diff line change
Expand Up @@ -939,7 +939,7 @@ parseRecord jc tvMap argTys opts tName conName fields obj inTaggedObject =
if inTaggedObject then (tagFieldName (sumEncoding opts) :) else id
knownFields = appE [|H.fromList|] $ listE $
map (\knownName -> tupE [appE [|T.pack|] $ litE $ stringL knownName, [|()|]]) $
tagFieldNameAppender $ map nameBase fields
tagFieldNameAppender $ map (fieldLabel opts) fields
checkUnknownRecords =
caseE (appE [|H.keys|] $ infixApp (varE obj) [|H.difference|] knownFields)
[ match (listP []) (normalB [|return ()|]) []
Expand Down
11 changes: 6 additions & 5 deletions Data/Aeson/Types/FromJSON.hs
Original file line number Diff line number Diff line change
Expand Up @@ -1260,15 +1260,15 @@ instance (ProductFromJSON arity f, ProductSize f
--------------------------------------------------------------------------------

class FieldNames f where
fieldNames :: f a -> [Text] -> [Text]
fieldNames :: f a -> [String] -> [String]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the type signature from [Text] to [String] deliberately to be able to call the fieldLabelModifier without a pack / unpack cycle.


instance (FieldNames a, FieldNames b) => FieldNames (a :*: b) where
fieldNames _ =
fieldNames (undefined :: a x) .
fieldNames (undefined :: b y)

instance (Selector s) => FieldNames (S1 s f) where
fieldNames _ = (pack (selName (undefined :: M1 _i s _f _p)) :)
fieldNames _ = (selName (undefined :: M1 _i s _f _p) :)

class RecordFromJSON arity f where
recordParseJSON
Expand All @@ -1282,9 +1282,10 @@ instance ( FieldNames f
\obj -> checkUnknown obj >> recordParseJSON' p obj
where
knownFields :: H.HashMap Text ()
knownFields = H.fromList $ map (,()) $
fieldNames (undefined :: f a)
[pack (tagFieldName (sumEncoding opts)) | fromTaggedSum]
knownFields = H.fromList $ map ((,()) . pack) $
[tagFieldName (sumEncoding opts) | fromTaggedSum] <>
(fieldLabelModifier opts <$> fieldNames (undefined :: f a) [])

checkUnknown =
if not (rejectUnknownFields opts)
then \_ -> return ()
Expand Down
3 changes: 3 additions & 0 deletions changelog.md
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
For the latest version of this document, please see [https://github.com/bos/aeson/blob/master/changelog.md](https://github.com/bos/aeson/blob/master/changelog.md).

## 1.5.0.0
* Fix bug in `rejectUnknownFields` not respecting `fieldLabelModifier`, thanks to Markus Schirp.

#### 1.4.7.1

* GHC 8.10 compatibility, thanks to Ryan Scott.
Expand Down
3 changes: 2 additions & 1 deletion tests/ErrorMessages.hs
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ tests :: [TestTree]
tests =
[ aesonGoldenTest "simple" "tests/golden/simple.expected" output
, aesonGoldenTest "generic" "tests/golden/generic.expected" (outputGeneric G)
, aesonGoldenTest "generic" "tests/golden/th.expected" (outputGeneric TH)
]

output :: Output
Expand Down Expand Up @@ -140,7 +141,7 @@ outputGeneric choice = concat
(select
thSomeTypeParseJSONRejectUnknownFields
gSomeTypeParseJSONRejectUnknownFields)
[ "{\"tag\": \"record\", \"testOne\": 1.0, \"testZero\": 1}"
[ "{\"tag\": \"record\", \"testone\": 1.0, \"testZero\": 1}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

, "{\"testZero\": 1}"
, "{\"tag\": \"record\", \"testone\": true, \"testtwo\": null, \"testthree\": null}"
]
Expand Down
2 changes: 1 addition & 1 deletion tests/golden/generic.expected
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Foo (reject unknown fields)
Error in $: parsing Types.Foo(Foo) failed, unknown fields: ["tag"]
Foo (reject unknown fields, tagged single)
Expand Down
48 changes: 48 additions & 0 deletions tests/golden/th.expected
Original file line number Diff line number Diff line change
@@ -0,0 +1,48 @@
OneConstructor
Error in $: When parsing the constructor OneConstructor of type Types.OneConstructor expected Array but got String.
Error in $: When parsing the constructor OneConstructor of type Types.OneConstructor expected an empty Array but got Array of length 1.
Nullary
Error in $: When parsing Types.Nullary expected a String with the tag of a constructor but got X.
Error in $: When parsing Types.Nullary expected String but got Array.
SomeType (tagged)
Error in $: parsing Int failed, expected Number, but encountered Boolean
Error in $: key "contents" not found
Error in $: When parsing the record record of type Types.SomeType the key testone was not present.
Error in $.testone: parsing Double failed, unexpected Boolean
Error in $: When parsing Types.SomeType expected an Object with a tag field where the value is one of [nullary, unary, product, record, list], but got X.
Error in $: key "tag" not found
Error in $: When parsing Types.SomeType expected Object but got Array.
SomeType (single-field)
Error in $: parsing Int failed, expected Number, but encountered Object
Error in $: parsing Int failed, expected Number, but encountered Array
Error in $: When parsing Types.SomeType expected an Object with a single tag/contents pair where the tag is one of [nullary, unary, product, record, list], but got X.
Error in $: When parsing Types.SomeType expected an Object with a single tag/contents pair but got 2 pairs.
Error in $: When parsing Types.SomeType expected an Object with a single tag/contents pair but got 0 pairs.
Error in $: When parsing Types.SomeType expected Object but got Array.
Error in $: not enough input. Expecting ':'
Error in $: not enough input. Expecting object value
Error in $: not enough input. Expecting ',' or '}'
SomeType (two-element array)
Error in $: parsing Int failed, expected Number, but encountered Boolean
Error in $: When parsing the constructor Record of type Types.SomeType expected Object but got Null.
Error in $: When parsing Types.SomeType expected a 2-element Array with a tag and contents element where the tag is one of [nullary, unary, product, record, list], but got X.
Error in $: When parsing Types.SomeType expected an Array of 2 elements where the first element is a String but got Null at the first element.
Error in $: When parsing Types.SomeType expected an Array of 2 elements but got 0 elements
Error in $: When parsing Types.SomeType expected Array but got Object.
Error in $: not enough input. Expecting ',' or ']'
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 $.testone: parsing Double failed, unexpected Boolean
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For this comment to make sense, look at the 2nd commit in isolation.

Foo (reject unknown fields)
Error in $: Unknown fields: ["tag"]
Foo (reject unknown fields, tagged single)
Error in $: Unknown fields: ["unknownField"]
EitherTextInt
Error in $: When parsing the constructor NoneNullary of type Types.EitherTextInt expected String but got String.
Error in $: When parsing the constructor NoneNullary of type Types.EitherTextInt expected String but got Array.
Product2 Int Bool
Error in $: expected Bool, but encountered Null
Error in $: When parsing the constructor Product2 of type Types.Product2 expected Array of length 2 but got Array of length 0.
Error in $: When parsing the constructor Product2 of type Types.Product2 expected Array but got Object.