java-generator doesn't handle correctly numeric enum types

[musabshak]

Describe the bug

When generating a Java POJO from a CRD yaml, if the CRD yaml defines an "enum" property with values of the form

enum:
  - bool:true
  - bool:false
  type: string

, a syntax-incorrect Java enum is generated - something like the following:

public enum MyEnum {
   BOOL:TRUE ("bool:true") 
   BOOL:FALSE ("bool:false")
}

The problem obviously is - colons aren't an accepted character in variable names.

The CRD I tried to convert where the above enum is defined is: https://github.com/emissary-ingress/emissary/blob/master/manifests/emissary/emissary-crds.yaml.in#L3820.

Fabric8 Kubernetes Client version

6.6.2

Steps to reproduce

To replicate, create a maven project with the correct dependencies for fabric8 k8s client, add the java-generator-maven-plugin, and try and generate POJOs from the above CRD file.

Expected behavior

If the enum values are integers i.e.

enum:
  - 301
  - 302
  - 303
  - 307
  - 308
  type: integer

, the plugin does the right thing ie it prepends V__ to the enum value.

There should probably be a general check to parse the enum value specified in the yaml and ensure it translates to a valid variable name?

The original CRD was generated by a go/kubebuilder: https://github.com/emissary-ingress/emissary/blob/f17e1898c44bd77db0986c310a8a34d0ffde17a6/pkg/api/getambassador.io/v3alpha1/common.go#L54

Runtime

Kubernetes (vanilla)

Kubernetes API Server version

other (please specify in additional context)

Environment

macOS

Fabric8 Kubernetes Client Logs

No response

Additional context

k8s API Server Version: v1.22.17

[andreaTP]

Hi @musabshak and thanks again for reporting this one.

This looks like a plain bug, where we are missing some String sanitization, would you feel comfortable attempting a fix?
I expect the change to be similar to #4769 and I can assist with any further questions 🙂 .

Otherwise, I can probably fix it early next week.

Side note: I'm NOT sure that enums with type: integer are working as expected, as I have never seen this use case.
Adding an integration test for it would be best.

[musabshak]

I can try working on it on over the weekend, but no guarantees haha.

What should be the bug fix be? We surely don't want to reject the above CRD/throw an exception. Should we replace enum value "bool:true" with "bool-true" or something?

[andreaTP]

Yup, I would say that the column should become an underscore for the value name.

[andreaTP]

@musabshak I took a peek at this 🙂

I haven't noticed but you are using an old version of the java-generator, with 6.8.1 the generated code is already correctly escaped:

    public enum Tls {
        ...
        @com.fasterxml.jackson.annotation.JsonProperty("bool:true")
        BOOL_TRUE("bool:true"),
        @com.fasterxml.jackson.annotation.JsonProperty("bool:false")
        BOOL_FALSE("bool:false")
        ...

the emissary-crd generates code that correctly compiles already (added to the tests to prevent regressions).

Still, I'm following up on my comment, and re-purposing this PR to fix the behavior when there are numeric enums.

[musabshak]

Oh, good catch. Apologies, should have checked bug against latest version myself before opening issue.

The upstream project I'm working on uses fabric8 kubernetes-client version 6.6.2 so will need to get project to move to 6.8.1.

Is it possible to use the java-generator-maven-plugin version 6.8.1 with kubernetes-client version 6.6.2? So far, I've been using the same version for the plugins as the fabric8 client. Not sure how the plugins are built/organized/if everything needs to be the same version as the underlying fabric8 kubernetes-client.

[musabshak]

As for the integer enum type, the following CRD

redirect_response_code:
    description: The response code to use when generating an HTTP redirect.
      Defaults to 301. Used with `host_redirect`.
    enum:
    - 301
    - 302
    - 303
    - 307
    - 308
    type: integer

currently generates the following Java enum:

public enum Redirect_response_code {

        @com.fasterxml.jackson.annotation.JsonProperty("301")
        V__301("301"), @com.fasterxml.jackson.annotation.JsonProperty("302")
        V__302("302"), @com.fasterxml.jackson.annotation.JsonProperty("303")
        V__303("303"), @com.fasterxml.jackson.annotation.JsonProperty("307")
        V__307("307"), @com.fasterxml.jackson.annotation.JsonProperty("308")
        V__308("308");

        java.lang.String value;

        Redirect_response_code(java.lang.String value) {
            this.value = value;
        }

        @com.fasterxml.jackson.annotation.JsonValue()
        public java.lang.String getValue() {
            return value;
        }
    }

This is with version 6.6.2. This compiles fine but I guess the correct enum definition should be:

public enum Redirect_response_code {

        @com.fasterxml.jackson.annotation.JsonProperty("301")
        V__301(301), @com.fasterxml.jackson.annotation.JsonProperty("302")
        V__302(302), @com.fasterxml.jackson.annotation.JsonProperty("303")
        V__303(303), @com.fasterxml.jackson.annotation.JsonProperty("307")
        V__307(307), @com.fasterxml.jackson.annotation.JsonProperty("308")
        V__308(308);

        java.lang.Integer value;

        Redirect_response_code(java.lang.Integer value) {
            this.value = value;
        }

        @com.fasterxml.jackson.annotation.JsonValue()
        public java.lang.Integer getValue() {
            return value;
        }
    }

where the associated value with the enum constant is an integer. Right?

[andreaTP]

Thanks for reverting back @musabshak !

Let's start with the easy one 🙂 #5462 fully fixes the handling of numeric enums and generates the expected code, it might not be "evident" as we are not committing a lot of generated code but this is the culprit of it, and it reflects your expectations in the integration tests.

Regarding mixing and matching kubernetes-client versions we strictly have one official answer:

Mixing and matching of kubernetes-client (and related) libraries and tools is NOT supported

I need to ask you which are the reasons preventing you from upgrading the entire project to the latest and greatest.

And now space for nuances 😛 :

• at the best of my understanding there are no fundamental changes affecting the generator from 6.6 -> 6.8
• removing the generatedAnnotations (especially sundrio dependencies) is going to improve stability
• you can try to use the java-generator at a different version than the client, you'll be working on an untested combination but is very likely gonna work

[musabshak]

Thanks for opening the PR, much appreciated!

Mixing and matching not supported - got it.

Re 6.6 -> 6.8 and upgrading to latest and greatest - it just creates one more PR I need to get approved in the upstream project haha. Working through upgrading right now. Off the bat, seems like #4662 removes some static methods so I need to refactor the project code at least a little bit. Hopefully that's all!

EDIT: we're limited by the java operator sdk version; need to updated josdk before updating fabric8: https://github.com/operator-framework/java-operator-sdk/releases

removing the generatedAnnotations (especially sundrio dependencies) is going to improve stability

Could you elaborate on this? Define "stability"? What does "unstable" POJO generation from CRD entail? POJOs are either generated or not; does generatedAnnotations affect the correctness of the generated code somehow?

[andreaTP]

Could you elaborate on this? Define "stability"? What does "unstable" POJO generation from CRD entail? POJOs are either generated or not; does generatedAnnotations affect the correctness of the generated code somehow?

I do not mean "unstable" POJOs as they are completely stable.
I mean that it can be challenging (depending on the project setup) to keep sundrio versions aligned and work in larger projects with transitive dependencies.
That said, for any given configuration that produces a result that compiles, there is no correctness issue I'm aware of.

[manusa]

If you didn't have the Sundrio annotations, it would be more than likely that you could generate your Java types using the Maven Plugin with the latest 6.8.1 version and that those types worked correctly with earlier 6.x versions of the client.
The last couple of weeks/months, there has been some movement in the Sundrio area. The builders and intermediate types have some changes that make builders produced from different Sundrio versions work well together.