Skip to content

@JsonValue ignored for enums #1617

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

Closed
thomas-bee opened this issue Nov 29, 2022 · 10 comments · Fixed by #1618
Closed

@JsonValue ignored for enums #1617

thomas-bee opened this issue Nov 29, 2022 · 10 comments · Fixed by #1618
Assignees
Labels
type: enhancement A general enhancement

Comments

@thomas-bee
Copy link

According to the docs of Couchbase's @JsonValue: NOTE: when use for Java enums, one additional feature is that value returned by annotated method is also considered to be the value to deserialize from, not just JSON String to serialize as. This is possible since set of Enum values is constant and it is possible to define mapping, but can not be done in general for POJO types; as such, this is not used for POJO deserialization.

The following enum should be deserialised / serialised using its code property. However, @JsonValue has no effect and Springdata Couch tries to deserialise by enum name. Note however that using the original Jackson annotation @JsonValue, the deserialisation does work !
In both cases, serialisation does not work,

public enum ETurbulenceCategory {
    T10("10%"),
    T20("20%"),
    T30("30%");

    private final String code;

    ETurbulenceCategory(String code) {
        this.code = code;
    }
//  DOES WORK FOR DE-SERIALISATION, BUT NOT FOR SERIALSATION
    @com.fasterxml.jackson.annotation.JsonValue
//  DOES NOT WORK AT ALL
//  @com.couchbase.client.core.deps.com.fasterxml.jackson.annotation.JsonValue
    String getCode() {
        return code;
    }
}
@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 29, 2022
@mikereiche
Copy link
Collaborator

mikereiche commented Nov 29, 2022

spring-data-couchbase does not use the jackson object mapper. Instead it uses its own MappingCouchbaseConverter. Support for this annotation would need to be added.

deserialization does not work for me

org.springframework.core.convert.ConversionFailedException: Failed to convert from type [java.lang.String] to type [org.springframework.data.couchbase.domain.ETurbulenceCategory] for value '10%'

	at org.springframework.core.convert.support.ConversionUtils.invokeConverter(ConversionUtils.java:47)
	at org.springframework.core.convert.support.GenericConversionService.convert(GenericConversionService.java:192)
	at org.springframework.data.couchbase.core.convert.MappingCouchbaseConverter.getPotentiallyConvertedSimpleRead(MappingCouchbaseConverter.java:431)
	at org.springframework.data.couchbase.core.convert.MappingCouchbaseConverter.readValue(MappingCouchbaseConverter.java:967)
	at org.springframework.data.couchbase.core.convert.MappingCouchbaseConverter$CouchbasePropertyValueProvider.getPropertyValue(MappingCouchbaseConverter.java:1096)
	at org.springframework.data.couchbase.core.convert.MappingCouchbaseConverter.getValueInternal(MappingCouchbaseConverter.java:323)
	at org.springframework.data.couchbase.core.convert.MappingCouchbaseConverter$1.doWithPersistentProperty(MappingCouchbaseConverter.java:283)
	at org.springframework.data.couchbase.core.convert.MappingCouchbaseConverter$1.doWithPersistentProperty(MappingCouchbaseConverter.java:274)
	at org.springframework.data.mapping.model.BasicPersistentEntity.doWithProperties(BasicPersistentEntity.java:298)
	at org.springframework.data.couchbase.core.convert.MappingCouchbaseConverter.read(MappingCouchbaseConverter.java:274)
	at org.springframework.data.couchbase.core.convert.MappingCouchbaseConverter.read(MappingCouchbaseConverter.java:253)
	at org.springframework.data.couchbase.core.convert.MappingCouchbaseConverter.read(MappingCouchbaseConverter.java:208)
	at org.springframework.data.couchbase.core.convert.MappingCouchbaseConverter.read(MappingCouchbaseConverter.java:93)
	at org.springframework.data.couchbase.core.AbstractTemplateSupport.decodeEntityBase(AbstractTemplateSupport.java:122)

@thomas-bee
Copy link
Author

thomas-bee commented Nov 29, 2022

spring-data-couchbase does not use the jackson object mapper. Instead it uses its own MappingCouchbaseConverter. Support for this annotation would need to be added.

Thanks a lot @mikereiche
What, then, is the point of having @com.couchbase.client.core.deps.com.fasterxml.jackson.annotation.JsonValue and the quoted documentation that goes with it?

Is there a workaround to achieving the intended serialisation / de-serialisation?

@mikereiche
Copy link
Collaborator

mikereiche commented Nov 29, 2022

What, then, is the point of having @com.couchbase.client.core.deps.com.fasterxml.jackson.annotation.JsonValue and the quoted documentation that goes with it?

That belongs to the Java SDK and works in the Java SDK as described in the documentation when the Java SDK Object Mapper is used. Spring Data Couchbase does not use the Java SDK Object Mapper. The issue with using that Object Mapper is that once anything is serialized/deserialized using it, then Spring Data Couchbase has no control over the serialization/deserialization. See #908. Ideally, I would have duplicated all the functionality of the Java SDK annotations in spring data couchbase, but I have not.

I can add handling of the annotation to MappingCouchbaseConverter.

@vforchi
Copy link

vforchi commented Nov 30, 2022

I had a similar issue when trying to serialize LocalDate and LocalDateTime.

I still don't understand the purpose of duplicating the jackson (not part of the java SDK) annotations in the spring-data-couchbase module, if they are not used or interpreted. Jackson itself won't work with those annotations, because they are in the wrong package.

I see mainly one problem with implementing serialization/deserialization from scratch: jackson is used in other spring projects (spring-web-mvc, to name one), so if you want to write a simple app that exposes a CRUD API in front of couchbase, and you need some custom mappings, you have to define them twice: once to make the jackson serialization work, needed in the controller, and once to make couchbase work, which seems a bit redundant.

@mikereiche
Copy link
Collaborator

mikereiche commented Nov 30, 2022

I still don't understand the purpose of duplicating the jackson (not part of the java SDK) annotations in the spring-data-couchbase module

The repackaged classes are in the java SDK. If the application required a different, conflicting version of jackson, couchbase can still use the (repackaged) jackson that it wants. When the classes are repackaged, all the references to the old package are renamed to the new package. Thus where the original jackson jar looks for com.fasterxml.jackson.annotation.JsonValue annotation, the repackaged jackson jar looks for the com.couchbase.client.core.deps.com.fasterxml.jackson.annotation.JsonValue annotation.

@mikereiche
Copy link
Collaborator

mikereiche commented Nov 30, 2022

I had a similar issue when trying to serialize LocalDate and LocalDateTime.

Can you open a new issue for that? LocalDate and LocalDateTime should be deserialized without issue (unless they have been serialized to a String by the java sdk )
#576

There is an open issue regarding compatibility with the java sdk - #1203

@mikereiche
Copy link
Collaborator

mikereiche commented Nov 30, 2022

so if you want to write a simple app that exposes a CRUD API in front of couchbase, and you need some custom mappings, you have to define them twice: once to make the jackson serialization work, needed in the controller, and once to make couchbase work, which seems a bit redundant.

Correct. Custom customizations must be defined where they are used. There's really no way around that. I would love to use the jackson ObjectMapper, but the reason for not relying on the jackson Object Mapper is described in #908. A good example of having to do two implementations is the Field Level Encryption converter. The java sdk contains a converter for FLE, and spring-data-couchbase contains a converter for FLE.

once to make the jackson serialization work, needed in the controller, and once to make couchbase work

I'm trying to think of a custom annotation/converter that would need to work in both the controller and couchbase (?). For instance - Field Level Encryption only needs to work for spring-data-couchbase - everything else can just ignore it. Something like @JsonValue does need to work in both the controller and spring-data-couchbase, but it's not "custom" (and therefore only needs to be implemented in spring-data-couchbase - which is the purpose of issue #1617).

Note: The java sdk ObjectMapper is exposed in spring-data-couchbase.

Let me know if there are other annotations like @ JsonValue that need to be implemented. It's just a matter of adding the property value converter class, and have the converter factory recognize the annotation.

mikereiche added a commit that referenced this issue Dec 1, 2022
@vforchi
Copy link

vforchi commented Dec 1, 2022

Can you open a new issue for that? LocalDate and LocalDateTime should be deserialized without issue (unless they have been serialized to a String by the java sdk )

It was serialized, but to a number, and I wanted a String.

I understand your point, but I still don't see the reason for duplicating the jackson annotation, which I find very confusing.

Besides @JsonValue, @jsonformat would also be useful.

Also, I think the documentation could be improved, especially with respect to the mapping.

@mikereiche
Copy link
Collaborator

mikereiche commented Dec 1, 2022

Besides @JsonValue, @jsonformat would also be useful.

I'll look at that today. Maybe added to the PR for @JsonValue if possible.

It was serialized, but to a number, and I wanted a String.

As mentioned above - #1203. I looked at it again yesterday and added a note to it. Serializing to a String would break non-upgraded spring-data-couchbase applications as they would not have the StringToLocal... converters. Maybe it's time to bite the bullet and make the change. It would require keeping the NumberToLocal... read converters and all spring-data-couchbase applications that read/write the same documents containing those types to be upgraded at the same time. I don't think that is too much to ask.

Also, I think the documentation could be improved, especially with respect to the mapping.

I have to agree with you here. I'm sorry. We do accept contributions to the project. https://github.com/spring-projects/spring-data-build/blob/main/CONTRIBUTING.adoc

@mikereiche mikereiche added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Dec 1, 2022
mikereiche added a commit that referenced this issue Dec 1, 2022
mikereiche added a commit that referenced this issue Dec 7, 2022
mikereiche added a commit that referenced this issue Dec 7, 2022
mikereiche added a commit that referenced this issue Dec 8, 2022
@mikereiche mikereiche self-assigned this Dec 8, 2022
mikereiche added a commit that referenced this issue Dec 14, 2022
mikereiche added a commit that referenced this issue Dec 14, 2022
mikereiche added a commit that referenced this issue Dec 15, 2022
mikereiche added a commit that referenced this issue Dec 15, 2022
mikereiche added a commit that referenced this issue Dec 15, 2022
mikereiche added a commit that referenced this issue Dec 15, 2022
mikereiche added a commit that referenced this issue Dec 15, 2022
mikereiche added a commit that referenced this issue Dec 15, 2022
@mikereiche mikereiche added this to the 5.0.1 (2022.0.1) milestone Dec 16, 2022
@mikereiche
Copy link
Collaborator

@vforchi @thomas-bee - this should be available in SNAPSHOT if you want to try it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement A general enhancement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants