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

Use JsonValue method type when present. #449

Closed
wants to merge 1 commit into from
Closed

Use JsonValue method type when present. #449

wants to merge 1 commit into from

Conversation

SimY4
Copy link

@SimY4 SimY4 commented May 28, 2024

JsonValue can be set not only on enums but on regular types. And the return type of this method will be used as a serialization target type.

This PR makes Jackson module aware of JsonValue annotations on regular types.

Verified

This commit was signed with the committer’s verified signature.
wwills2 William "Zan" Wills
Copy link
Member

@CarstenWickner CarstenWickner left a comment

Choose a reason for hiding this comment

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

Hi @SimY4,

Thanks a lot for your contribution!
It looks already pretty good.
However, I'd suggest trying out the SubtypeResolver instead of the CustomDefinitionProvider as it avoids handling certain things.

If that works (as I hope it does), it's only a matter of updating the documentation and changelog to include your great addition. 😃

@@ -0,0 +1,65 @@
/*
* Copyright 2020 VicTools.
Copy link
Member

Choose a reason for hiding this comment

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

nitpick: Time traveler 😉

Suggested change
* Copyright 2020 VicTools.
* Copyright 2024 VicTools.


/**
* Implementation of the {@link CustomDefinitionProviderV2} interface for treating object types based on a {@link JsonValue} annotation
* being present with {@code value = true} on exactly one argument-free method. If no such annotations exist, no custom definition will be returned;
Copy link
Member

Choose a reason for hiding this comment

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

typo

Suggested change
* being present with {@code value = true} on exactly one argument-free method. If no such annotations exist, no custom definition will be returned;
* being present with {@code value = true} on exactly one argument-free method. If no such annotation exists, no custom definition will be returned;

Comment on lines +136 to +142
static class TestTypeWithJsonValue {
String value;

@JsonValue
String getJsonValue() {
return value;
}
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: Let's also consider generic type parameters here.
In that case, it is important what is declared on the specific field/method then, e.g.,

public TestTypeWithJsonValue<String> typeWithJsonValue;
Suggested change
static class TestTypeWithJsonValue {
String value;
@JsonValue
String getJsonValue() {
return value;
}
static class TestTypeWithJsonValue<T> {
T jsonValue;
@JsonValue
T getJsonValue() {
return this.jsonValue;
}

For that to be respected, we'd also need to consider fields and methods explicitly (as only there, the necessary type parameter may be defined) – on top of the one for the type in general.
A generally registered SubtypeResolver may take care of this though (I think, but haven't tested it now).

Comment on lines +35 to +43
public class JsonValueDefinitionProvider implements CustomDefinitionProviderV2 {

@Override
public CustomDefinition provideCustomSchemaDefinition(ResolvedType javaType, SchemaGenerationContext context) {
ResolvedMethod jsonValueAnnotatedMethod = getJsonValueAnnotatedMethod(javaType, context);
if (jsonValueAnnotatedMethod == null) {
return null;
}
return new CustomDefinition(context.createDefinition(jsonValueAnnotatedMethod.getType()));
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: A custom definition provider by design should be the last resort when realizing some requirement.
In this case, I'd avoid it to not get into the messy realm of "when to inline and when to declare in the central $defs".
Fortunately, there is already an alternative for exactly this kind of "replacing one type with another in the schema": a SubtypeResolver (even if this is not necessarily an actual subtype here).

Suggested change
public class JsonValueDefinitionProvider implements CustomDefinitionProviderV2 {
@Override
public CustomDefinition provideCustomSchemaDefinition(ResolvedType javaType, SchemaGenerationContext context) {
ResolvedMethod jsonValueAnnotatedMethod = getJsonValueAnnotatedMethod(javaType, context);
if (jsonValueAnnotatedMethod == null) {
return null;
}
return new CustomDefinition(context.createDefinition(jsonValueAnnotatedMethod.getType()));
public class JsonValueTypeResolver implements SubtypeResolver {
@Override
public List<ResolvedType> findSubtypes(ResolvedType javaType, SchemaGenerationContext context) {
ResolvedMethod jsonValueAnnotatedMethod = this.getJsonValueAnnotatedMethod(javaType, context);
if (jsonValueAnnotatedMethod == null) {
return null;
}
return Arrays.asList(jsonValueAnnotatedMethod.getType());

@SimY4
Copy link
Author

SimY4 commented Jul 11, 2024

Hi @CarstenWickner . Sorry for taking so long to reply.

I'm having hard time to see how I can solve this problem with SubtypeResolver

@JsonValue doesn't really add a subtype relationship to it's class. It overrides the definition i.e. I can have an Email type in Java that gets converted into a string on an API level. There's no subtype relationship here.

This PR is essentially the way I addressed this issue locally and decided to contribute it back. If you think this is not the right approach, I'm happy to leave it as a ticket in the backlog instead. 👍

@SimY4 SimY4 closed this Jul 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants