Repository navigation
Share the library-independent JSON converter code across the Jackson and kotlinx modules - #739
Merged
Merged
Conversation
…and kotlinx modules The Jackson 2, Jackson 3 and kotlinx.serialization modules each carried their own copy of the converter base, the ref factory thread-local, the key type cache and the ref creation. That code now lives in storm-core, and each module keeps only what its library's API forces. The two Jackson test suites, which had forked, are one suite again, which exposed that Jackson 3 ignored @JSON(failOnUnknown = true): both converters now set the two features explicitly. Fixes #409
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Unit tests drive JsonConverterSupport and RefJsonSupport without a JSON library; the Jackson suite gains the null wrapper payload, the keyless plain object, a deserializer-only field and unconstructible serializers. The sealed-type walk now looks inside a plain array, the integral id fallback returns an Integer when it fits, and a null wrapper payload reads as no ref on Jackson 2 as it does on Jackson 3.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #409
What is shared now
The library-independent half of the three JSON modules moved into storm-core, in the impl package the modules already import from:
JsonConverterSupport, theORMConverterbase: the column, writing to and reading from the database, the ref factory of the conversion in progress with its nesting rule, the mapper cache key, the sealed-type walk. A module binds a library by implementingwriteandread.RefJsonSupport: the JSON forms of a ref, the primary key type cache, and the creation of loaded, unloaded and compound-key refs.Jackson 2, Jackson 3 and kotlinx.serialization all build on these. The kotlinx module keeps its flag-keyed mapper cache and its Kotlin serializer resolution; the Jackson modules keep their serializer, deserializer and mapper builder.
What could not be shared
The issue estimated a twelve-line adapter. Jackson calls into the serializer and deserializer with its own types, which Jackson 3 moved to a new package and renamed (
ValueSerializer,SerializationContext,DatabindException,JacksonExceptionforIOException,writeName, the removedContextualSerializerinterface, the builder-only mapper). That code stays per module, with the Jackson 3 file as the source and the Jackson 2 file derived from it by the package rename plus those renames. Per module the production code went from 594 to 408 non-blank lines, with 377 shared lines in core; the net change of the PR is minus 911 lines.Bug found by unifying the tests
On Jackson 3,
@Json(failOnUnknown = true)was silently ignored: Jackson 3 turns the unknown-property check off by default and the converter only ever turned it off. Both converters now setfailOnUnknownandfailOnMissingexplicitly, and the suite has rejection tests for both. In the changelog.Tests
The two Jackson suites had forked to 59 and 83
StormModuleTestmethods with different names; they are one suite of 35 tests covering every assertion of both, identical across the modules modulo the package rename, import order and the API-forced lines in custom serializers.JsonORMConverterImplTestis folded into the integration test,JsonMetamodelTestand the Kotlin interop suite now run on Jackson 3 as well (the Jackson 3 Kotlin module is a test dependency), andJsonIntegrationTestwrites dates as ISO strings on both. Both Jackson modules pass 90 tests; the kotlinx module passes its 194 unchanged.