fabric8io / kubernetes-client

Java client for Kubernetes & OpenShift
http://fabric8.io
Apache License 2.0
3.42k stars 1.47k forks source link

Java Generator:Generated Java Classes Contain Incorrect Field Types and Missing Fields #6079

Open fengyinqiao opened 5 months ago

fengyinqiao commented 5 months ago

Describe the bug

Description:

When generating Java classes from CRDs using fabric8 Kubernetes client, there are issues with incorrect field types and missing fields in the generated classes.

Issues:

  1. Field Type Error:

    • CRD Specification:
      replicas:
      description: The number of pods in the cluster.
      minimum: 1
      type: integer
    • Generated Java Class:

      @com.fasterxml.jackson.annotation.JsonProperty("replicas")
      @io.fabric8.generator.annotation.Required()
      @io.fabric8.generator.annotation.Min(1.0)
      @com.fasterxml.jackson.annotation.JsonPropertyDescription("The number of pods in the cluster.")
      @com.fasterxml.jackson.annotation.JsonSetter(nulls = com.fasterxml.jackson.annotation.Nulls.SKIP)
      private Long replicas;

      The replicas field should be generated as an Integer instead of Long.

  2. Missing Fields:

    • CRD Specification:

      volumes:
      description: List of volumes as Storage objects representing the JBOD disks array.
      items:
       properties:
         class:
           description: The storage class to use for dynamic volume allocation.
           type: string
         # Other fields omitted for brevity
       type: object
      type: array
    • Generated Java Class (Volumes.java):

      @com.fasterxml.jackson.annotation.JsonProperty("class")
      @com.fasterxml.jackson.annotation.JsonPropertyDescription("The storage class to use for dynamic volume allocation.")
      @com.fasterxml.jackson.annotation.JsonSetter(nulls = com.fasterxml.jackson.annotation.Nulls.SKIP)
      private String _class;
      
      public String get_class() {
       return _class;
      }
      
      public void set_class(String _class) {
       this._class = _class;
      }
    • Generated Builder Class (VolumesBuilder.java):

      public Volumes build() {
       Volumes buildable = new Volumes();
       buildable.setDeleteClaim(fluent.getDeleteClaim());
       buildable.setId(fluent.getId());
       buildable.setOverrides(fluent.buildOverrides());
       buildable.setSelector(fluent.buildSelector());
       buildable.setSize(fluent.getSize());
       buildable.setSizeLimit(fluent.getSizeLimit());
       buildable.setType(fluent.getType());
       buildable.setAdditionalProperties(fluent.getAdditionalProperties());
       // Missing class field
       return buildable;
      }
    • Generated Fluent Class (VolumesFluent.java):

      public class VolumesFluent<A extends VolumesFluent<A>> extends BaseFluent<A>{
      public VolumesFluent() {
      }
      
      public VolumesFluent(Volumes instance) {
       this.copyInstance(instance);
      }
      private Boolean deleteClaim;
      private Long id;
      private ArrayList<OverridesBuilder> overrides;
      private SelectorBuilder selector;
      private String size;
      private String sizeLimit;
      private Volumes.Type type;
      private Map<String,Object> additionalProperties;
      
      protected void copyInstance(Volumes instance) {
       instance = (instance != null ? instance : new Volumes());
       if (instance != null) {
             this.withDeleteClaim(instance.getDeleteClaim());
             this.withId(instance.getId());
             this.withOverrides(instance.getOverrides());
             this.withSelector(instance.getSelector());
             this.withSize(instance.getSize());
             this.withSizeLimit(instance.getSizeLimit());
             this.withType(instance.getType());
             this.withAdditionalProperties(instance.getAdditionalProperties());
             // Missing class field
           }
      }
      }

Dependencies:

<dependencies>
    <dependency>
        <groupId>io.fabric8</groupId>
        <artifactId>kubernetes-client</artifactId>
    </dependency>
    <dependency>
        <groupId>io.fabric8</groupId>
        <artifactId>generator-annotations</artifactId>
    </dependency>
    <dependency>
        <groupId>io.sundr</groupId>
        <artifactId>builder-annotations</artifactId>
        <version>0.103.1</version>
        <scope>provided</scope>
    </dependency>
</dependencies>
<build>
    <plugins>
        <plugin>
            <groupId>io.fabric8</groupId>
            <artifactId>java-generator-maven-plugin</artifactId>
            <version>6.12.0</version> 
            <executions>
                <execution>
                    <goals>
                        <goal>generate</goal>
                    </goals>
                    <configuration>
                        <source>src/main/resources/kubernetes</source>
                        <alwaysPreserveUnknown>true</alwaysPreserveUnknown>
                        <generatedAnnotations>true</generatedAnnotations>
                        <extraAnnotations>true</extraAnnotations>
                    </configuration>
                </execution>
            </executions>
        </plugin>
    </plugins>
</build>

Please investigate and provide a fix for these issues.

Thank you!

Fabric8 Kubernetes Client version

6.12.0

Steps to reproduce

  1. Define the CRD as shown above.
  2. Use the java-generator-maven-plugin to generate the Java classes.
  3. Observe the generated Java classes and note the discrepancies in field types and missing fields.

Expected behavior

Actual Behavior:

Runtime

Kubernetes (vanilla)

Kubernetes API Server version

1.25.3@latest

Environment

macOS 13.5

Fabric8 Kubernetes Client Logs

No response

Additional context

No response

stale[bot] commented 2 months ago

This issue has been automatically marked as stale because it has not had any activity since 90 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions!

andreaTP commented 2 months ago

Hi @fengyinqiao thanks for the interest and sorry for the delay, I missed this issue!

We are talking about two things here:

shawkins commented 1 month ago

missing fields in the generated Builder: I encourage you to open an issue to the relevant project: sundrio

There's several things that don't work well here with sundrio. It doesn't understand get/set_ where the next letter is not capitalized. If you try to use get_Class / set_Class sundrio will normalize the name to Class and you end up with compilation failures because getClass conflicts with the built-in java method.

Also any simple non-alpha sufix seems to get stripped by sundrio normalization, so doing something like class_ doesn't work either.

The only thing that will work with sundrio currently is not use the root word class without additional alpha characters - stringClass or sClass will work just fine for example.

andreaTP commented 1 month ago

Thanks for the additional context @shawkins :+1: In this case, I'd suggest avoiding <extraAnnotations>true</extraAnnotations> for this CRD definition.

fengyinqiao commented 1 month ago

stringClass or sClass will work just fine for example.

@shawkins This field was named by the Strimzi-Kafka community, not by us. Changing the name of a field involves compatibility issues, because field in the production environment already has this name.

fengyinqiao commented 1 month ago

In this case, I'd suggest avoiding <extraAnnotations>true</extraAnnotations> for this CRD definition.

@andreaTP why ?

fengyinqiao commented 1 month ago
  • The class field in volumes is missing in the builder and fluent classes.

If you try to use get_Class / set_Class sundrio will normalize the name to Class and you end up with compilation failures because getClass conflicts with the built-in java method.

The get_Class and set_Class methods are already generated in the pojo, and theoretically should be used directly in the builder and fluent, otherwise why generate the set and get methods? @shawkins

shawkins commented 1 month ago

@fengyinqiao I understand that the field name is not determined by you, my comments are more about what will currently work with sundrio. For this case to work without changes to sundrio, then changes are needed to the java generator. Alternattively opening an issue against sundrio as @andreaTP suggested may eventually address this - but it will proabably take a while before that is picked up.

fengyinqiao commented 1 month ago

@fengyinqiao I understand that the field name is not determined by you, my comments are more about what will currently work with sundrio. For this case to work without changes to sundrio, then changes are needed to the java generator. Alternattively opening an issue against sundrio as @andreaTP suggested may eventually address this - but it will proabably take a while before that is picked up.

@shawkins ok. Anyway, thanks for your reply.

andreaTP commented 1 month ago

Thinking about this twice, one possible viable workaround would be to make the field names configurable through configuration. The result is not going to be extremely nice to configure, but it will be effective.

from the user perspective, the result should be something like this:

Maven configuration:

<fieldNameMappings>
  <com.example.v1.fieldnames.Volume.class>
    clazz
  </com.example.v1.fieldnames.Volume.class>
</fieldNameMappings>

Java generator result Volume.java:

@com.fasterxml.jackson.annotation.JsonProperty("class")
@com.fasterxml.jackson.annotation.JsonPropertyDescription("The storage class to use for dynamic volume allocation.")
@com.fasterxml.jackson.annotation.JsonSetter(nulls = com.fasterxml.jackson.annotation.Nulls.SKIP)
private String clazz;

public String getClazz() {
    return clazz;
}

public void setClazz(String clazz) {
    this.clazz = clazz;
}

and even sundrio should be happy with it. I'm completely swamped those days but would be happy to review an implementation if someone can help (maybe @matteriben is interested?).

fengyinqiao commented 1 month ago

@andreaTP nice trick from the user's perspective, hhh.

andreaTP commented 1 month ago

@fengyinqiao would you like to step up? :slightly_smiling_face:

fengyinqiao commented 1 month ago

@fengyinqiao would you like to step up? 🙂

I can give it a try, but I have to wait until I'm free. I'm a little busy right now, my company has a lot of things to do. Maybe two months later? @andreaTP

andreaTP commented 1 month ago

There is no rush at all from my point of view, disabling sundrio should work for most of the users for the time being as a workaround.