fix: crd-gen includes only enum values and discard enum fields

[leoandrea7]

Description

Fix #4225

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change
• Chore (non-breaking change which doesn't affect codebase;
  test, version modification, documentation, etc.)

Checklist

• Code contributed by me aligns with current project license: Apache 2.0
• I Added CHANGELOG entry regarding this change
• I have implemented unit tests to cover my changes
• I have added/updated the javadocs and other documentation accordingly
• No new bugs, code smells, etc. in SonarCloud report
• I tested my code in Kubernetes
• I tested my code in OpenShift

[andreaTP]

Thanks a lot for the fix @leoandrea7 , unfortunately, we need a better check as, extending the test in this way:

  public enum AnnotatedEnum {
    non("N"),
    @JsonProperty("oui") es("O");

    private final String abbreviation;
    private AnnotatedEnum def;

    AnnotatedEnum(String abbreviation) {
      this.abbreviation = abbreviation;
    }

    AnnotatedEnum(AnnotatedEnum def) {
      this.abbreviation = null;
      this.def = def;
    }

    public String getAbbreviation() {
      return abbreviation;
    }

    public AnnotatedEnum getDefault() {
      return non;
    }
  }

breaks the test.

[andreaTP]

I took a look at this issue and cannot find a proper check to correctly include/exclude enum's instances.

Specifically given the test here:
https://github.com/sundrio/sundrio/blob/b7091f8fbe792596258d6bf3a10c05bc3a415bd4/adapters/api/src/test/java/io/sundr/adapter/testing/AbstractAdapterTest.java#L206

We include all the needed properties, but we don't know how to discriminate actual values of the Enum from other methods/accessos/constructors.

Maybe @metacosm or @iocanel can help.

Alternatively, it should be enough to "workaround" this issue by considering the JsonIgnore annotation on enum properties (I can open a separate issue for it if needed).

[leoandrea7]

@andreaTP maybe you should add a filter here like the filter i added with this PR
.filter(property -> def.getFullyQualifiedName().equals(property.getTypeRef().toString()))

[andreaTP]

@leoandrea7 couldn't find any way to discriminate the actual enum values from other properties :-(

[leoandrea7]

@andreaTP As you can see def.getProperties return both enum and other fields. But Property that is enum has TypeRef of the same enum type.

[andreaTP]

@leoandrea7 right, but this is not generic enough to distinguish enum values from the rest as demonstrated in this comment:
#4230 (review)

i.e.
any property that returns as Type the enum type will be picked up anyhow.

Feel free to try out my snippet and you will get the relevant test to fail.

[leoandrea7]

@andreaTP you are absolutely right...The problem is that TypeDef class does not have a method that return just enum values..I tried to fetch enum with Class.forName(def.getFullyQualifiedName()) in order to use .values() directly but there is classloader problems for test..

[leoandrea7]

@andreaTP After some thought, I think your test case with an enum containing a field of the same type as the enum itself is a very infrequent case. For now it would be better to merge this solution which solves most of the use cases and I could add in the docs of the crd-gen a new section (titled limitation or known problems) in which I specify that this particular case is not yet supported. Then if in the future the sundrio library will introduce a specific method to retrieve only the values of an enum through the TypeDef class then we will also solve the use case you have proposed. This merge would solve most of the use cases for enums containing fields. What do you think?

[andreaTP]

I don't think this case is infrequent, and anyhow, unfortunately, it's semantically incorrect.
I would suggest that we move on fixing JsonIgnore behavior in enums so that there will be a viable workaround for your use case(possibly re-using the more generic Properties parser logic).

Meanwhile, we can add the necessary information in Sundrio cc. @iocanel and move on from there.

[andreaTP]

Submitted #4294 should provide a workaround also for this issue.

[manusa]

Hi #4294 has been merged. Is this PR providing something else?

[andreaTP]

@manusa , yes, this PR is providing something different, but, to have a robust implementation we need to bubble up information changing sundrio first.

[andreaTP]

@shawkins do you mind fixing the conflicts?
As discussed in the other issue, I have no objection to moving this forward at this point.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

87.5% Coverage
0.0% Duplication