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

[java-generator] Fix additionalProperties ser/deser #4561

Merged
merged 2 commits into from
Nov 14, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,10 @@
import com.github.javaparser.ast.body.ClassOrInterfaceDeclaration;
import com.github.javaparser.ast.body.EnumDeclaration;
import com.github.javaparser.ast.body.FieldDeclaration;
import com.github.javaparser.ast.body.MethodDeclaration;
import com.github.javaparser.ast.body.VariableDeclarator;
import com.github.javaparser.ast.expr.*;
import com.github.javaparser.ast.stmt.BlockStmt;
import com.github.javaparser.ast.type.ClassOrInterfaceType;
import com.github.javaparser.utils.StringEscapeUtils;
import io.fabric8.java.generator.Config;
Expand Down Expand Up @@ -280,6 +282,13 @@ public GeneratorResult generateJava() {

objField.createGetter().addAnnotation("com.fasterxml.jackson.annotation.JsonAnyGetter");
objField.createSetter().addAnnotation("com.fasterxml.jackson.annotation.JsonAnySetter");

MethodDeclaration additionalSetter = clz.addMethod("setAdditionalProperties", Modifier.Keyword.PUBLIC);
andreaTP marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
MethodDeclaration additionalSetter = clz.addMethod("setAdditionalProperties", Modifier.Keyword.PUBLIC);
MethodDeclaration additionalSetter = clz.addMethod("setAdditionalProperty", Modifier.Keyword.PUBLIC);

Sorry, I may not be being clear. I would expect both the test and this logic to say setAdditionalProperty singular. That lines up with the model generation logic in the main project - Pod.setAdditionalProperty.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, that's what I did, changing both breaks the functionality I'm fixing here as demonstrated by the additional test included in this PR.

If you are still in doubt I propose to have a quick sync on it to clarify somewhere next week ;-)

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, it would probably be best to compare to what's already in use. The core model classes are generated with:

    @JsonIgnore
    private Map<String, Object> additionalProperties = new HashMap<String, Object>();

   @JsonAnyGetter
    public Map<String, Object> getAdditionalProperties() {
        return this.additionalProperties;
    }

    @JsonAnySetter
    public void setAdditionalProperty(String name, Object value) {
        this.additionalProperties.put(name, value);
    }

due to a limitation in the jsonschema2pojo,

@JsonIgnore
void setAdditionalProperties(Map<String, Object> properties)

being generated by the class annotations:

@Setter
@Accessors(prefix = {
    "_",
    ""
})

As far as I know that handles all of the issues we have seen with serdes and builder logic.

Could you try aligning to that?

Copy link
Member Author

Choose a reason for hiding this comment

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

@shawkins sorry for the long forth and back, the integration test failure was due to a different reason ... fixed now 👍

additionalSetter.addAnnotation("com.fasterxml.jackson.annotation.JsonAnySetter");
additionalSetter.addParameter("String", "key");
additionalSetter.addParameter("Object", "value");
additionalSetter
.setBody(new BlockStmt().addStatement(new NameExpr("this." + Keywords.ADDITIONAL_PROPERTIES + ".put(key, value);")));
}

buffer.add(new GeneratorResult.ClassResult(this.className, cu));
Expand Down
5 changes: 5 additions & 0 deletions java-generator/it/src/it/cert-manager/expected/Auth.expected
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,9 @@ public class Auth implements io.fabric8.kubernetes.api.model.KubernetesResource
public void setAdditionalProperties(java.util.Map<String, Object> additionalProperties) {
this.additionalProperties = additionalProperties;
}

@com.fasterxml.jackson.annotation.JsonAnySetter()
public void setAdditionalProperties(String key, Object value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be setAdditionalProperty singular?

Copy link
Member Author

Choose a reason for hiding this comment

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

Surprisingly not, the integration test fails if I make it singular.

Choose a reason for hiding this comment

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

So probably some jackson magic :)

Copy link
Member Author

Choose a reason for hiding this comment

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

yup, at least now is properly tested 🙂

this.additionalProperties.put(key, value);;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#
# Copyright (C) 2015 Red Hat, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

invoker.goals=test
88 changes: 88 additions & 0 deletions java-generator/it/src/it/preserve-unknown-serdeser/pom.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
<?xml version="1.0" encoding="UTF-8"?>
<!--

Copyright (C) 2015 Red Hat, Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.

-->
<project>

<modelVersion>4.0.0</modelVersion>

<artifactId>preserve-unknown-serdeser</artifactId>
<groupId>io.fabric8.it</groupId>
<version>0.0-SNAPSHOT</version>
<packaging>jar</packaging>

<properties>
<maven.compiler.source>@maven.compiler.source@</maven.compiler.source>
<maven.compiler.target>@maven.compiler.target@</maven.compiler.target>
</properties>

<dependencies>
<dependency>
<groupId>io.fabric8</groupId>
<artifactId>java-generator-integration-tests</artifactId>
<version>@project.version@</version>
</dependency>
<dependency>
<groupId>io.fabric8</groupId>
<artifactId>generator-annotations</artifactId>
<version>@project.version@</version>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
<version>@junit.version@</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-engine</artifactId>
<version>@junit.version@</version>
<scope>test</scope>
</dependency>
</dependencies>

<build>
<plugins>
<plugin>
<groupId>io.fabric8</groupId>
<artifactId>java-generator-maven-plugin</artifactId>
<version>@project.version@</version>
<executions>
<execution>
<goals>
<goal>generate</goal>
</goals>
</execution>
</executions>
<configuration>
<source>src/test/resources/dapr-components-crd.yaml</source>
<generatedAnnotations>false</generatedAnnotations>
</configuration>
</plugin>
<plugin>
<groupId>org.apache.maven.plugins</groupId>
<artifactId>maven-surefire-plugin</artifactId>
<version>3.0.0-M7</version>
<configuration>
<useFile>false</useFile>
<trimStackTrace>false</trimStackTrace>
</configuration>
</plugin>
</plugins>
</build>

</project>
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/**
* Copyright (C) 2015 Red Hat, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.fabric8.it.certmanager;

import com.fasterxml.jackson.databind.JsonNode;
import io.dapr.v1alpha1.Component;
import io.dapr.v1alpha1.ComponentSpec;
import io.fabric8.kubernetes.api.model.AnyType;
import io.fabric8.kubernetes.api.model.ObjectMeta;
import io.fabric8.kubernetes.client.utils.Serialization;
import org.junit.jupiter.api.Test;
import io.fabric8.java.generator.testing.KubernetesResourceDiff;

import java.nio.file.Path;
import java.nio.file.Paths;
import java.nio.file.Files;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.Map;
import java.util.List;

import static org.junit.jupiter.api.Assertions.assertEquals;

class TestPreserveUnknownSerDeser {

@Test
void testDeserialization() {
// Arrange
Component sample =
Serialization.unmarshal(getClass().getResourceAsStream("/sample.yaml"), Component.class);

// Act
Map<String, Object> testObj = sample.getSpec().getTest().getAdditionalProperties();

// Assert
assertEquals(2, testObj.size());
// TODO check content
}

private Component createSampleComponent() throws Exception {
Component component = new Component();
ComponentSpec spec = new ComponentSpec();

io.dapr.v1alpha1.componentspec.Test t = new io.dapr.v1alpha1.componentspec.Test();

t.setAdditionalProperties("one", "ONE");

HashMap<String, Object> twoContent = new HashMap();
twoContent.put("more", 1);
twoContent.put("complex", true);
t.setAdditionalProperties("two", twoContent);

spec.setTest(t);

component.setSpec(spec);
ObjectMeta om = new ObjectMeta();
om.setName("messagebus");
component.setMetadata(om);

return component;
}

@Test
void testAgainstSample() throws Exception {
// Arrange
Path resPath = Paths.get(getClass().getResource("/sample.yaml").toURI());
String yamlContent = new String(Files.readAllBytes(resPath), "UTF8");
Component sample = createSampleComponent();
KubernetesResourceDiff diff = new KubernetesResourceDiff(yamlContent, Serialization.asYaml(sample));

// Act
List<JsonNode> aggregatedDiffs = diff.getListOfDiffs();

// Assert
assertEquals(0, aggregatedDiffs.size());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
#
# Copyright (C) 2015 Red Hat, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

apiVersion: apiextensions.k8s.io/v1
kind: CustomResourceDefinition
metadata:
name: components.dapr.io
spec:
group: dapr.io
versions:
- name: v1alpha1
schema:
openAPIV3Schema:
description: Component describes an Dapr component type
properties:
apiVersion:
description: 'APIVersion defines the versioned schema of this representation
of an object. Servers should convert recognized schemas to the latest
internal value, and may reject unrecognized values. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#resources'
type: string
auth:
description: Auth represents authentication details for the component
properties:
secretStore:
type: string
required:
- secretStore
type: object
kind:
description: 'Kind is a string value representing the REST resource this
object represents. Servers may infer this from the endpoint the client
submits requests to. Cannot be updated. In CamelCase. More info: https://git.k8s.io/community/contributors/devel/sig-architecture/api-conventions.md#types-kinds'
type: string
metadata:
type: object
scopes:
items:
type: string
type: array
spec:
description: ComponentSpec is the spec for a component
type: object
properties:
test:
type: object
x-kubernetes-preserve-unknown-fields: true
type: object
served: true
storage: true
names:
kind: Component
plural: components
singular: component
categories:
- all
- dapr
scope: Namespaced
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
#
# Copyright (C) 2015 Red Hat, Inc.
#
# Licensed under the Apache License, Version 2.0 (the "License");
# you may not use this file except in compliance with the License.
# You may obtain a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS,
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
# See the License for the specific language governing permissions and
# limitations under the License.
#

apiVersion: dapr.io/v1alpha1
kind: Component
metadata:
name: messagebus
spec:
test:
one: "ONE"
two:
more: 1
complex: true