Strict versions of record codecs that fail on unknown properties #6
Conversation
|
I like the idea for sure, I've done something similar in a Scala project but not needed it in PS yet. I think I'd rather have it entirely separate from the lax object codec though, as doing it this way imposes a performance penalty on all object decoders. I'd also remove the record from the state and just use the |
src/Data/Codec/JSON.purs
Outdated
| -- | See also `Data.Codec.JSON.Record.object'` for a more commonly useful | ||
| -- | version of this function. | ||
| object' ∷ ∀ a. { ignoreUnknownProps :: Boolean } -> PropCodec a -> Codec a | ||
| object' { ignoreUnknownProps } codec = Codec.codec' dec enc |
There was a problem hiding this comment.
Agreed with Gary that the implementation for this should be separate from the lax case, at which point we can avoid the additional parameter (since it would always be false) and name the function in a way that more clearly expresses the intent, e.g. objectStrict
fsoikin
force-pushed
the
track-unknown-props
branch
from
0a42df8 to
cbcb337
Compare
fsoikin
changed the title
|
Premature optimization is the root of all evil. |
|
|
||
| -- | The class used to enable the building of `Record` codecs by providing a | ||
| -- | record of codecs. | ||
| class RowListCodecStrict (rl ∷ RL.RowList Type) (ri ∷ Row Type) (ro ∷ Row Type) | rl → ri ro where |
There was a problem hiding this comment.
I looked at parametrizing this class or its method, but that turned out to be almost as much ceremony as duplicating it. And since we're duplicating everything else anyway...
| CJ.recordPropOptional (Proxy ∷ Proxy sym) codec tail | ||
| where | ||
| codec ∷ CJ.Codec a | ||
| codec = coerce (Rec.get (Proxy ∷ Proxy sym) codecs ∷ Optional a) | ||
|
|
||
| tail ∷ CJ.PropCodec (Record ro') | ||
| tail = rowListCodec (Proxy ∷ Proxy rs) ((unsafeCoerce ∷ Record ri → Record ri') codecs) | ||
| tail = rowListCodec @rs ((unsafeCoerce ∷ Record ri → Record ri') codecs) |
There was a problem hiding this comment.
Introducing type applications raises the minimum compiler version to run this to 0.15.x - I'd say we should either not use them at all or do a proper breaking change where all the Proxy are turned into type applications.
There was a problem hiding this comment.
I'd be fine converting everything to use type applications, so could accept this as-is and then update the rest in a follow up PR.
There was a problem hiding this comment.
Well, I don't have a problem either way. And since this is your repo @garyb, I'll do whatever you say.
There was a problem hiding this comment.
Sorry I've been slow about this! One minor suggestion and then I'm happy to merge.
I'll hold off on releasing it until the few remaining Proxy usages have been updated though. I won't have time to do that until the weekend, so unless you want to do that in a follow up PR too 😉 it'll be a few more days yet before it gets out.
| module Data.Codec.JSON.Strict | ||
| ( PropCodec | ||
| , ClaimedProps | ||
| , objectStrict |
There was a problem hiding this comment.
Could just call this object now I suppose?
There was a problem hiding this comment.
Then there is a high chance of mixing up the two in consumer code, and there won't be any warning, the code will just silently accept unknown fields.
|
Yes, I was going to do that in a follow-up PR. |
|
@garyb could we get this merged/released? |
|
Sorry, I thought I already had merged it. 🤦♂️ 😄 |
|
Published as v2 now, with the switch to VTAs throughout (aside from Thanks @fsoikin for the contribution, and sorry it took so long to get sorted out! |
|
Wonderful, thank you! 🙏 |
Turns out there is a use case for not ignoring unknown object keys when parsing records, but failing on them. When the program that parses JSON is not a consumer of upstream API, but takes JSON as input, it's useful to report unknown properties, lest the consumer make a typo without noticing.
This is something I implemented for Spago in purescript/spago#1272, but turns out it's needed in other upstream libraries, so I'm contributing here.