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

fix(engine): don't check for @JsonCreator annotation and only take @JsonValue into account #3097

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

Conversation

cromoteca
Copy link
Contributor

@cromoteca cromoteca commented Dec 30, 2024

As explained in the discussion below, there are use cases for @JsonValue use without @JsonCreator. As the former is needed to create a domain object, it's easier to only check for that one and assume that the user has taken deserialization into account, by using the latter or another means like a constructor.

Closes #3095.

This could be cherry-picked to 24.6 (see forum post linked in the issue).

Copy link

codecov bot commented Dec 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.60%. Comparing base (fede3cd) to head (ba4b063).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3097   +/-   ##
=======================================
  Coverage   92.60%   92.60%           
=======================================
  Files          85       85           
  Lines        3164     3164           
  Branches      775      775           
=======================================
  Hits         2930     2930           
  Misses        183      183           
  Partials       51       51           
Flag Coverage Δ
unittests 92.60% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Legioth
Copy link
Member

Legioth commented Jan 2, 2025

A use case for @JsonCreator without @JsonValue was described in the forum discussion linked from the ticket. But is there any use case for @JsonValue without @JsonCreator or should that combination still be disallowed?

@cromoteca
Copy link
Contributor Author

While testing some possible use cases, I realized that @JsonCreator is actually optional, at least in some cases, as Jackson is smart enough to deserialize correctly a single value passed as constructor parameter for example. Here's the simplest case.

public class Email {
    private final String email;

    public Email(String email) {
        this.email = email;
    }

    @JsonValue
    public String getEmail() {
        return email;
    }

    @Override
    public String toString() {
        return email;
    }
}

So we should probably just look at @JsonValue, as verifying that a corresponding creator (with annotation or not) exists is not easy.

@cromoteca
Copy link
Contributor Author

This version handles both serialization formats correctly (just comment out @JsonValue to verify):

public class Email {
    private String email;

    public Email() {}

    public Email(String email) {
        this.email = email;
    }

    @JsonValue
    public String getEmail() {
        return email;
    }

    public void setEmail(String email) {
        this.email = email;
    }

    @Override
    public String toString() {
        return email;
    }
}

@Legioth
Copy link
Member

Legioth commented Jan 2, 2025

Sounds like it would make sense to only look for @JsonValue since that's the one with an actual semantic meaning but ignoring @JsonCreator which has no semantic meaning but is used as a hint in multiple different cases.

@cromoteca
Copy link
Contributor Author

Exactly. So I propose to:

  • file a new bug
  • remove @JsonCreator support completely from Hilla (does not break existing apps)
  • cherry-pick to 24.6
  • update docs

@Legioth
Copy link
Member

Legioth commented Jan 3, 2025

Makes sense though the cherry pick should be done only if we're over 99.99% certain that it will always be backwards compatible.

@cromoteca
Copy link
Contributor Author

cromoteca commented Jan 3, 2025

I proposed to cherry-pick since the user who reported on the forum cannot upgrade from 24.5 to 24.6 due to this.
I can't see which case could lead to incompatibility: a class which only uses one is currently unsupported. And for those who use both, we get the type from the return type of the method marked with @JsonValue.

@cromoteca cromoteca marked this pull request as draft January 7, 2025 08:55
@cromoteca cromoteca marked this pull request as ready for review January 8, 2025 13:59
@cromoteca cromoteca changed the title feat: allow classes with one of JsonValue and JsonCreator to be generated fix(engine): don't check for @JsonCreator annotation and only take @JsonValue into account Jan 8, 2025
Copy link

sonarqubecloud bot commented Jan 9, 2025

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.

Hilla should tolerate classes using only one of @JsonValue and @JsonCreator
2 participants