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

Change json serialization to Jackson #46

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

codekonzept
Copy link

No description provided.

Patrick Adamek and others added 2 commits December 20, 2021 15:34
@codekonzept codekonzept force-pushed the feature/jackson-migration branch from 60bfa53 to 4d74de1 Compare December 20, 2021 14:35
Signed-off-by: Patrick Adamek <[email protected]>
@codekonzept codekonzept force-pushed the feature/jackson-migration branch from 4e140c5 to 6657438 Compare December 20, 2021 17:34
@Test
@Ignore
public void canSerializeOptionals() {
// assertEquals("{\"myOpt\":\"test\",\"next\":null}", JsonSerialization.serialized(new MyOptional(Optional.of("test"), Optional.empty())));
Copy link
Author

Choose a reason for hiding this comment

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

Different serialization output

Copy link
Member

Choose a reason for hiding this comment

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

How's it different?

If it's something like spaces between elements, then perhaps we need custom assertions in order to normalise messages before comparing them.

Copy link
Author

Choose a reason for hiding this comment

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

One serializer just serializes a given value or null whereas the other serializer uses a sub-structure with a value field for that. Therefore the structures are different and one can't just deserialize the output of one with the other. This results in a compatibility problem. If one starts from scratch this does not seem to be a problem.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense. What's the impact of changed behaviour on end-users?

By the way, are we keeping both serializers? If that's the case it might be a good idea to have a common base test case for both. Differences between implementations could be put into specific test case implementations.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @codekonzept and @jakzal !

I really like the idea of isolating differences in tests.

Copy link
Author

Choose a reason for hiding this comment

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

@jakzal: I think it boils down to if we want to be backward compatible or not. If we want to be, leaving it how it is now results in not being able to deserialize existing objects. This is a no go. If we just want to give another serializer option this would be the way to go.

@VaughnVernon + @jakzal: i will work on separating the tests so that the differences in serializing optionals become more obvious.

public class JsonSerializationTest {

void executeJsonTest(Runnable assertFunction){
JsonSerialization.setEngine(new JacksonJsonSerialization());
Copy link
Author

Choose a reason for hiding this comment

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

Switch the engine

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.

3 participants