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

Don't throw on unknown JSON input fields #2553

Merged
merged 5 commits into from
Jun 29, 2021
Merged

Conversation

HenkKalkwater
Copy link
Contributor

@HenkKalkwater HenkKalkwater commented Apr 19, 2021

An attempt to fix #1647

Still WIP, because of:

  • All unit test compile and work again (right now, dmd generates cryptic template errors)

I've removed the assumption that, when deserializing a struct and
encountering an unknown field, the serializer wants to skip over
a field right now. Instead of deserializeValue(...) directly calling
ignoreValue, it is up to the serializer implementation to do this if the
entry_callback in readDictionary returns false.
I've removed the assumption that, when deserializing a struct and
encountering an unknown field, the serializer wants to skip over
a field right now. Instead of deserializeValue(...) directly calling
ignoreValue, it is up to the serializer implementation to do this if the
entry_callback in readDictionary returns false.

This also removes the requirement for serializer to implement a
ignoreValue() function.

This commit also fixes unit tests.
@HenkKalkwater HenkKalkwater changed the title WIP: Don't throw on unknown JSON input fields Don't throw on unknown JSON input fields Jun 21, 2021
@HenkKalkwater
Copy link
Contributor Author

I'm not sure what part the CI build failure caused on DC=ldc, DVersion=1.16.0, arch=x86. Is this the fault of my code or just bad luck?

@s-ludwig
Copy link
Member

I'm not sure what part the CI build failure caused on DC=ldc, DVersion=1.16.0, arch=x86. Is this the fault of my code or just bad luck?

I haven't seen the error message, but I guess it's a flaky test, there doesn't seem to be anything that looks like it could fail on 32-bit.

@s-ludwig
Copy link
Member

The change looks good so far, except that it makes a breaking change to the serialization API (there are some serializer implementations outside of vibe.d). I see three possible ways around this:

  1. Let the serialization module select the return value that is accepted by the serializer
  2. Introduce ignoreValue, or maybe skipValue as an optional primitive that is called by the serialization code instead of returning false
  3. Solve this fully within JsonSerializer, by using beginReadDictionaryEntry/endReadDictionaryEntry as a signal that the value has been consumed (requires something like a bool* m_dictionaryEntryConsumed field that points to a bool within readDictionary's stack frame and treated as true if it has been set to a different address after entry_callback has returned, to support nested dictionaries)

3. would obviously be the least intrusive one, but having 2. available might be handy for other serializers, too.

@HenkKalkwater
Copy link
Contributor Author

As you mentioned, I indeed didn't take API compatibility into account, perhaps since I assumed it wasn't an issue due to vibe-d still being under version 1.0.

I'll change my current implementation as mentioned in option 2. I'm not a huge fan of option 3, I believe it would complicate the execution flow of the code too much, making debugging harder. And I'm certain my implementation won't be perfect directly after writing it.

The entry_callback in readDictionary returns void once again, to undo an
otherwise breaking API change. Instead, the entry_callback itself checks
if the serializer has a skipValue() function and calls that instead.
This might break the API on occasions where serializers have already
defined a skipValue function theirselves, but it will hopefully happen
less often.

Instead on relying on the return value
Co-authored-by: Sönke Ludwig <sludwig@rejectedsoftware.com>
@s-ludwig s-ludwig merged commit 2466293 into vibe-d:master Jun 29, 2021
@HenkKalkwater
Copy link
Contributor Author

This should also have closed #1961 I believe.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

deserializeJson throws on unknown input fields
2 participants