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

With last version of jackson fasterxml since 2.17.1 to 2.18.0 included in the new spring boot 3.4.1 #514

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

RouxAntoine
Copy link

@RouxAntoine RouxAntoine commented Jan 14, 2025

look at issue #513

@fhussonnois
Copy link
Member

Hey Antoine, it seems there is an issue with the test:

Error: Failures: Error: SpecificStateChangeBuilderTest.shouldProperlySerializeChangeState:68 Unexpected exception thrown: com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Conflicting property-based creators: already had explicit creator [constructor for io.streamthoughts.jikkou.core.models.change.SpecificStateChange(4 args), annotations: {interface java.beans.ConstructorProperties=@java.beans.ConstructorProperties({"name", "op", "before", "after"})}, encountered another: [constructor forio.streamthoughts.jikkou.core.models.change.SpecificStateChange (5 args), annotations: {interface java.beans.ConstructorProperties=@java.beans.ConstructorProperties({"name", "op", "before", "after", "description"})} [INFO] Error: Tests run: 233, Failures: 1, Errors: 0, Skipped: 0

I think you can just drop the @ConstructorProperties from the class as well as the constructor.

@RouxAntoine
Copy link
Author

RouxAntoine commented Jan 15, 2025

Hey Antoine, it seems there is an issue with the test:

Error: Failures: Error: SpecificStateChangeBuilderTest.shouldProperlySerializeChangeState:68 Unexpected exception thrown: com.fasterxml.jackson.databind.exc.InvalidDefinitionException: Conflicting property-based creators: already had explicit creator [constructor for io.streamthoughts.jikkou.core.models.change.SpecificStateChange(4 args), annotations: {interface java.beans.ConstructorProperties=@java.beans.ConstructorProperties({"name", "op", "before", "after"})}, encountered another: [constructor forio.streamthoughts.jikkou.core.models.change.SpecificStateChange (5 args), annotations: {interface java.beans.ConstructorProperties=@java.beans.ConstructorProperties({"name", "op", "before", "after", "description"})} [INFO] Error: Tests run: 233, Failures: 1, Errors: 0, Skipped: 0

I think you can just drop the @ConstructorProperties from the class as well as the constructor.

Yes indeed I test it today and seems to be due to duplicate @ConstructorProperties, I can update my pull request. What do you thing is the best @ConstructorProperties to kept ? I am not sure to understand the usefullness of this annotation.

@RouxAntoine
Copy link
Author

Hi @fhussonnois, sorry but I read again this and I don't understand what you mean by:

I think you can just drop the @ConstructorProperties from the class as well as the constructor.

Both SpecificStateChange constructor are use somewhere else in jikkou so I can't remove it.

@fhussonnois
Copy link
Member

Yes, sorry I was looking at the DefaultChangeResult class for which I think the serialization/deserialization should be OK without :

    @ConstructorProperties({
            "end",
            "errors",
            "status",
            "data",
            "description"
    })
    public DefaultChangeResult {
    }

Now, Jackson has a good support for records.

But, you're totally right! For the SpecificStateChange dropping the @ConstructorProperties on the constructor with the fewest args is enough.

@RouxAntoine RouxAntoine force-pushed the fix/jikkou-513/creator-conflict branch from 9f34d1b to 2d2bec2 Compare January 17, 2025 14:09
final String description) {
super(name, op, before, after, description);
final T after) {
super(name, op, before, after, null);
Copy link
Author

Choose a reason for hiding this comment

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

Change are due to the change order of constructor.

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.

2 participants