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

Test Cases For Custom Date Serialization #938

Merged
merged 1 commit into from Feb 26, 2020

Conversation

seamuslowry
Copy link
Contributor

@seamuslowry seamuslowry commented Feb 12, 2020

I am new to the open source world and this is the first time I'm writing any groovy. Please attribute any mistakes to ignorance rather than malice

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Feb 12, 2020

Good test case. Custom JSON serializers should have priority over built-in serializers. Since you have already found the place in javers-core where serializers are beeing registered, think about contributing a PR with the fix. But remember, custom serializers manage only how your data are stored in JaversRepository. Diff logic is customized in CustomComparators.

@seamuslowry
Copy link
Contributor Author

@seamuslowry seamuslowry commented Feb 13, 2020

It may take a couple days as I'm a little pressed for time right now, but I think I could do that. Would it be appropriate to just keep committing to this pull request?

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Feb 14, 2020

take your time,
yes you can commit here

@seamuslowry
Copy link
Contributor Author

@seamuslowry seamuslowry commented Feb 14, 2020

What do you think of that as a solution? It would be tedious to write the test cases such that they test all the adapters in Java8TypeAdapters and UtilTypeCoreAdapters for the ability to override, but I'll do it if you feel it is necessary.

I tried to generalize it but a lot of the types don't have a constructor valid for something like clazz.getDeclaredConstructor().newInstance() so I couldn't do a full generalization.

As it is, at least one type from each of Java8TypeAdapters and UtilTypeCoreAdapters is tested to ensure it can be overridden and I threw in a test to override the adapter for Atomic as well.

@seamuslowry
Copy link
Contributor Author

@seamuslowry seamuslowry commented Feb 15, 2020

It wasn't sitting well with me to leave cases untested, so I wrote tests to verify that the 17 built in adapters I identified and affected could be properly overridden. I tried hard to generalize the tests so there wouldn't need to be 17 new adapter classes for this test, but I couldn't get to a solution that both tested all 17 cases and was fully generalized.

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Feb 17, 2020

I'm on holiday, will back to you after a few days

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Feb 25, 2020

Indeed, these 17 new adapter classes in the test case are not nice

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Feb 25, 2020

I have fixed this test using composition, see https://github.com/javers/javers/pull/942/files

@bartoszwalacik bartoszwalacik merged commit 5bdf3a9 into javers:master Feb 26, 2020
1 check passed
@seamuslowry
Copy link
Contributor Author

@seamuslowry seamuslowry commented Feb 26, 2020

Clever. I never thought of using the real adapters as delegates. I'll keep that pattern in my back pocket. Thanks!

@seamuslowry seamuslowry deleted the custom_serialize_dates branch Feb 26, 2020
@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Feb 26, 2020

👍 thanks for your contribution
this fix just released in 5.8.10 https://javers.org/release-notes

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.

None yet

2 participants