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

KotlinJacksonModule supports JsonProperty annotations on Kotlin data classes #359

Closed
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 26 additions & 1 deletion jsonschema-module-jackson/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

<properties>
<java.module.name>com.github.victools.jsonschema.module.jackson</java.module.name>
<kotlin.version>1.9.0-Beta</kotlin.version>
</properties>

<dependencies>
Expand All @@ -34,6 +35,17 @@
<artifactId>jackson-annotations</artifactId>
<scope>provided</scope>
</dependency>
<dependency>
<groupId>org.jetbrains.kotlin</groupId>
<artifactId>kotlin-stdlib</artifactId>
<version>${kotlin.version}</version>
</dependency>
Comment on lines +38 to +42
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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

<dependency>
<groupId>org.jetbrains.kotlin</groupId>
<artifactId>kotlin-test</artifactId>
<version>${kotlin.version}</version>
<scope>test</scope>
</dependency>
</dependencies>

<build>
Expand All @@ -54,7 +66,20 @@
<groupId>org.moditect</groupId>
<artifactId>moditect-maven-plugin</artifactId>
</plugin>
<plugin>
<groupId>org.jetbrains.kotlin</groupId>
<artifactId>kotlin-maven-plugin</artifactId>
<version>${kotlin.version}</version>
<extensions>true</extensions>
<configuration>
<jvmTarget>1.8</jvmTarget>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-compiler-plugin</artifactId>
</plugin>
Comment on lines +75 to +78
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: The Maven compiler plugin is the first one to be mentioned in this pom.xml, i.e., this repeated mention is redundant, right?

Copy link
Author

Choose a reason for hiding this comment

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

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.

</plugins>
</build>

</project>
</project>
Original file line number Diff line number Diff line change
Expand Up @@ -196,13 +196,24 @@ protected String getPropertyNameOverrideBasedOnJsonPropertyAnnotation(MemberScop
if (annotation != null) {
String nameOverride = annotation.value();
// check for invalid overrides
if (nameOverride != null && !nameOverride.isEmpty() && !nameOverride.equals(member.getDeclaredName())) {
if (isValidNameOverride(member, nameOverride)) {
return nameOverride;
}
}
return null;
}

/**
* Checks whether the potential name override is valid for the specified member.
*
* @param member field/method to override
* @param nameOverride the name that will override the original name of the member
* @return true if the specified override is valid for the member, false otherwise
*/
protected static boolean isValidNameOverride(MemberScope<?, ?> member, String nameOverride) {
return nameOverride != null && !nameOverride.isEmpty() && !nameOverride.equals(member.getDeclaredName());
}

/**
* Alter the declaring name of the given field as per the declaring type's {@link JsonNaming} annotation.
*
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
package com.github.victools.jsonschema.module.jackson;

import com.fasterxml.classmate.members.RawField;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.github.victools.jsonschema.generator.MemberScope;
import java.lang.reflect.Parameter;
import java.util.List;
import java.util.OptionalInt;
import kotlin.Metadata;

public class KotlinJacksonModule extends JacksonModule {
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 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.

/**
* Look up an alternative name for a member in the constructor parameter list.
* When the Kotlin compiler compiles a Kotlin data class, it creates a constructor with a parameter
* for each field in the data class. The parameters have generic name such as "arg0", "arg1", etc.
* In order to find the appropriate constructor parameter, the method first determines the index of
* specified member within its type member list and uses the parameter with the same index.
* In order to avoid erroneous overrides, this method verifies that the specified member is indeed of a
* Kotlin class.
Comment on lines +18 to +19
Copy link
Member

Choose a reason for hiding this comment

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

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.

*
* @param member field/method to look-up alternative property name for
* @return alternative property name or the base class implementation return value
*/
@Override
protected String getPropertyNameOverrideBasedOnJsonPropertyAnnotation(MemberScope<?, ?> member) {
Copy link
Member

Choose a reason for hiding this comment

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

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.

if (isKotlinType(member)) {
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: As per another comment, let's not limit this to Kotlin types alone.

OptionalInt memberIndex = getMemberIndex(member);
if (memberIndex.isPresent()) {
Parameter[] parameters = getConstructorParameters(member);
Parameter parameter = parameters[memberIndex.getAsInt()];
JsonProperty jsonPropertyAnnotation = parameter.getAnnotation(JsonProperty.class);
if (jsonPropertyAnnotation != null) {
String nameOverride = jsonPropertyAnnotation.value();
if (isValidNameOverride(member, nameOverride)) {
return nameOverride;
}
}
}
}
return super.getPropertyNameOverrideBasedOnJsonPropertyAnnotation(member);
Copy link
Member

Choose a reason for hiding this comment

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

suggestion: I'd still favor annotations on the field and its getter method before even considering to check the constructor parameters.


}

private OptionalInt getMemberIndex(MemberScope<?, ?> member) {
List<RawField> memberFields = member.getDeclaringType().getMemberFields();
for (int i = 0; i < memberFields.size(); i++) {
if (memberFields.get(i).getName().equals(member.getName())) {
return OptionalInt.of(i);
}
}
return OptionalInt.empty();
}

private static Parameter[] getConstructorParameters(MemberScope<?, ?> member) {
return member.getDeclaringType().getConstructors().get(0).getRawMember().getParameters();
Copy link
Member

Choose a reason for hiding this comment

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

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?

Suggested change
return member.getDeclaringType().getConstructors().get(0).getRawMember().getParameters();
return member.getDeclaringType().getConstructors().get(0).getRawMember().getParameters();

Copy link
Author

Choose a reason for hiding this comment

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

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)?

Copy link
Member

@CarstenWickner CarstenWickner Jun 10, 2023

Choose a reason for hiding this comment

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

@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);
}
Comment on lines +58 to +60
Copy link
Member

Choose a reason for hiding this comment

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

note: This check alone doesn't seem worth it to add an extra dependency for all consumers of this library.


}
Original file line number Diff line number Diff line change
@@ -0,0 +1,29 @@
package com.github.victools.jsonschema.module.jackson

import com.fasterxml.jackson.annotation.JsonProperty
import com.github.victools.jsonschema.generator.*
import org.junit.jupiter.api.Assertions
import org.junit.jupiter.api.Test
import java.util.*

class KotlinJacksonModuleTest {
data class TestJsonProperty(
@JsonProperty("my_text") val text: String,
@JsonProperty("my_number") val number: Int)

@Test
fun `naming override in kotlin data class with JsonProperty`() {
val config = SchemaGeneratorConfigBuilder(SchemaVersion.DRAFT_2020_12, OptionPreset.PLAIN_JSON)
.with(KotlinJacksonModule())
.build()

val generator = SchemaGenerator(config)
val result = generator.generateSchema(TestJsonProperty::class.java)
val propertiesNode = result[config.getKeyword(SchemaKeyword.TAG_PROPERTIES)]
val propertyNames: MutableSet<String> = TreeSet()
propertiesNode.fieldNames().forEachRemaining { e: String -> propertyNames.add(e) }
Assertions.assertTrue(propertyNames.contains("my_text"))
Assertions.assertTrue(propertyNames.contains("my_number"))
}

}