KotlinJacksonModule supports JsonProperty annotations on Kotlin data classes #359
Conversation
There was a problem hiding this comment.
Thanks a lot for your contribution.
I believe this will be a great addition. As per inline comments though, I'd prefer to not make this strictly Kotlin-specific.
It's a pity though, that the parameter names are not preserved on the implicit constructor of the Kotlin data classes.
| <dependency> | ||
| <groupId>org.jetbrains.kotlin</groupId> | ||
| <artifactId>kotlin-stdlib</artifactId> | ||
| <version>${kotlin.version}</version> | ||
| </dependency> |
There was a problem hiding this comment.
suggestion: I'd prefer to avoid adding this extra dependency for all (also non-Kotlin) users.
It is only used to detect Kotlin classes, which can just as easily be steered through a JacksonOption and then be applied to all types equally.
At the same time I was just wondering: since the Jackson library supports those annotations on constructor parameters as well (also in Java classes), there is nothing Kotlin-specific about this functionality then.
It just happens to apply to Kotlin data classes too, sure, but the more generic feature to be added here would be to always consider constructor parameters as well as annotations on the field and its getter.
There was a problem hiding this comment.
In that case, would you prefer checking for the annotation in the appropriate ctor parameter in FieldScope.getAnnotationConsideringFieldAndGetter? We can add an Option to enable this extra check
| <plugin> | ||
| <groupId>org.apache.maven.plugins</groupId> | ||
| <artifactId>maven-compiler-plugin</artifactId> | ||
| </plugin> |
There was a problem hiding this comment.
suggestion: The Maven compiler plugin is the first one to be mentioned in this pom.xml, i.e., this repeated mention is redundant, right?
There was a problem hiding this comment.
According to Kotlin docs (https://kotlinlang.org/docs/maven.html#compile-kotlin-and-java-sources):
To compile projects that include Kotlin and Java source code, invoke the Kotlin compiler before the Java compiler. In Maven terms it means that kotlin-maven-plugin should be run before maven-compiler-plugin using the following method, making sure that the kotlin plugin comes before the maven-compiler-plugin in your pom.xml file
That's why the additional maven-compiler-plugin was added (automatically by the IDE, BTW). It makes the first mention redundant though - I'll remove it.
| } | ||
|
|
||
| private static Parameter[] getConstructorParameters(MemberScope<?, ?> member) { | ||
| return member.getDeclaringType().getConstructors().get(0).getRawMember().getParameters(); |
There was a problem hiding this comment.
suggestion (non-blocking): Shouldn't we assume that multiple constructors could also exist? Only considering the first might be sufficient for Kotlin data classes, but perhaps more general support of annotations on constructor parameters would be achieved by taking others into account as well?
| return member.getDeclaringType().getConstructors().get(0).getRawMember().getParameters(); | |
| return member.getDeclaringType().getConstructors().get(0).getRawMember().getParameters(); |
There was a problem hiding this comment.
For Kotlin data classes, we can safely assume that all fields are initialized by the implicit ctor in their order in the field list. I'm afraid that might not be the case for other non-data JVM classes.
e.g.
class CtorTest{
final int bar;
@JsonProperty("my_bar")
final String foo;
public CtorTest(@JsonProperty("my_foo") int foo, String bar){
this.bar=foo;
this.foo=bar;
}
}How would you suggest we associate between fields and ctor parameters in the general case (non-Kotlin data classes)?
There was a problem hiding this comment.
@moranlefler I don't think we need to support such a mix of member and constructor parameter annotations.
First of all: it's probably a good idea to put this new feature behind a dedicated JacksonOption and then we could start with the assumption that there is one constructor with the same number of parameters as there are fields in the class and that those either match by parameter name or position.
I would have preferred matching by parameter name, but if I understood your code comment correctly, for the Kotlin data class that wouldn't work as they are generically called arg0, arg1, etc.
Therefore, we need to do it purely based on the index.
If someone else needs a different behavior in the future, we could easily extend this, but this approach should be sufficient for the Kotlin data classes as a start. No need to over-engineer it.
Perhaps nobody uses those annotations on constructor parameters anymore outside of auto-generated constructors such as these. 😃
| private static boolean isKotlinType(MemberScope<?, ?> member) { | ||
| return member.getDeclaringType().getErasedType().isAnnotationPresent(Metadata.class); | ||
| } |
There was a problem hiding this comment.
note: This check alone doesn't seem worth it to add an extra dependency for all consumers of this library.
| } | ||
| } | ||
| } | ||
| return super.getPropertyNameOverrideBasedOnJsonPropertyAnnotation(member); |
There was a problem hiding this comment.
suggestion: I'd still favor annotations on the field and its getter method before even considering to check the constructor parameters.
| */ | ||
| @Override | ||
| protected String getPropertyNameOverrideBasedOnJsonPropertyAnnotation(MemberScope<?, ?> member) { | ||
| if (isKotlinType(member)) { |
There was a problem hiding this comment.
suggestion: As per another comment, let's not limit this to Kotlin types alone.
| * @return alternative property name or the base class implementation return value | ||
| */ | ||
| @Override | ||
| protected String getPropertyNameOverrideBasedOnJsonPropertyAnnotation(MemberScope<?, ?> member) { |
There was a problem hiding this comment.
suggestion: There are other aspects where Jackson annotations are being considered -- not just for the property name overrides. It'd probably be worth it to generalize this a bit more to extend the coverage.
| import java.util.OptionalInt; | ||
| import kotlin.Metadata; | ||
|
|
||
| public class KotlinJacksonModule extends JacksonModule { |
There was a problem hiding this comment.
suggestion: Let's please not introduce an alternative module just for Kotlin. Instead, we should extend the JacksonModule's capabilities in general and thereby also cover the Kotlin use-case.
| * In order to avoid erroneous overrides, this method verifies that the specified member is indeed of a | ||
| * Kotlin class. |
There was a problem hiding this comment.
note: I understand your motivation but would prefer to not make this Kotlin specific. Perhaps other JVM languages have similar behavior. It'd be a pity to exclude those without any real need.
|
As per comment on #358, it appears one has to merely indicate where the Java annotation gets applied through an explicit "use site target" (e.g., With this in place, no additional change is required within the JSON Schema Generator, i.e., I'm closing this PR. |
[/issues/358]
I have extended
JacksonModuleand createdKotlinJacksonModuleto support Kotlin data classes.I have updated
pom.xmlin order to include a Kotlin test class. Please review these changes to make sure it doesn't break anything.The only functionality currently is to detect
@JsonPropertyannotations on data classes by identifying the appropriate constructor parameter in the compiled class. The functionality only supports the required primary constructor and will ignore any additional constructors.