-
Notifications
You must be signed in to change notification settings - Fork 59
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
JsonEnumDefaultValue support #447
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @tfactor2,
First of all: Thank you for your contribution!
Generally, I don't mind adding support for this Jackson annotation.
A challenge for such additions, like your proposed JsonEnumDefaultValueResolver
, is that it is probably working fine in your particular use-case but doesn't cover several other scenarios (namely: the variety of representations for a given enum value – for which there have been surprisingly many questions raised in the context of this project in the past).
Now I'm curious how widely used it is to declare a default enum value on the enum itself and not in the context of a particular field/method where it is used.
I'm wondering because I see two options here:
- We add some complexity to the
JsonEnumDefaultValueResolver
, accepting the fact that this would never cover all scenarios, but would at least cover those already supported by the schema generator today. - Or you leave this piece of straightforward configuration in your own code base, where you can make simplifying assumptions such as: "enum values in JSON match the Java/Kotlin enum values".
Is it worth tackling all that complexity or do you rather keep this on your side and we only add this as another example somewhere in the documentation, where others will still be able to benefit from your contribution?
What do you reckon?
public static String apply(TypeScope typeScope) { | ||
if (typeScope.getType().getErasedType().isEnum()) { | ||
return Arrays.stream(typeScope.getType().getErasedType().getDeclaredFields()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: Since MethodScope.getType()
can return null
for void
methods and you're only applying this to fields, I'd suggest making the declared parameter type FieldScope
instead.
Also getErasedType()
may return null
, I believe, if the given type is merely a generic type placeholder.
public static String apply(TypeScope typeScope) { | |
if (typeScope.getType().getErasedType().isEnum()) { | |
return Arrays.stream(typeScope.getType().getErasedType().getDeclaredFields()) | |
public static String apply(FieldScope scope) { | |
Class<?> erasedType = scope.getType().getErasedType(); | |
if (erasedType != null && erasedType.isEnum()) { | |
return Arrays.stream(erasedType.getDeclaredFields()) |
package com.github.victools.jsonschema.module.jackson; | ||
|
||
import com.fasterxml.jackson.annotation.JsonEnumDefaultValue; | ||
import com.github.victools.jsonschema.generator.TypeScope; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As per below suggestion, declaring the more specific FieldScope
instead of TypeScope
as method parameter type.
import com.github.victools.jsonschema.generator.TypeScope; | |
import com.github.victools.jsonschema.generator.FieldScope; |
return Arrays.stream(typeScope.getType().getErasedType().getDeclaredFields()) | ||
.filter(enumValue -> enumValue.isAnnotationPresent(JsonEnumDefaultValue.class)) | ||
.findFirst() | ||
.map(Field::getName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (blocking): This is too simplistic.
There are several ways to represent an enum value in the JSON (Schema). The declared name in the code may therefore not be the appropriate one to declare as default here.
We should at least attempt to cover the handful of standard options already supported by the main schema generator and this JacksonModule
(e.g., considering @JsonValue
annotations).
"enumValueWithJsonEnumDefaultValueAnnotation": { | ||
"type": "string", | ||
"enum": ["A", "B", "C"], | ||
"default": "B" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: The alignment seems a bit off. 😉
We check the backward compatibility of payloads based on schemas. There is a case that we want to mark as backward compatible, particularly when the enum set is reduced but there was a default enum value set. This will allow developers to fallback to it even if some enum is deleted. Here is the PR to support it in the Jackson module.
->
PS: Not sure about using DefaultResolver in the module.