CRD generator ignores `@Accessors` annotations

[metacosm]

Describe the bug

The CRD generator seemed to ignore accessor methods and just used the field name.

Environment

Quarkus operator extension 4.0.7

Possible Solution

The workaround was to add a JsonProperty to the field.

See https://github.com/java-operator-sdk/java-operator-sdk/issues/1792?notification_referrer_id=NT_kwDOAAHU-bE1NzM5NjEzMDI4OjEyMDA1Nw&notifications_query=is%3Aunread#issue-1605209108

Fabric8 Kubernetes Client version

5.12.4

Steps to reproduce

With a class like

@Getter
@Setter
@Accessors(prefix = "_")
public class Foo {
    private boolean _field;
}

The generated crd contained _field, rather than field as the property name.

Expected behavior

Generated CRD should output field instead of _field

Runtime

Kubernetes (vanilla)

Kubernetes API Server version

1.25.3@latest

Environment

Linux

Fabric8 Kubernetes Client Logs

No response

Additional context

No response

[andreaTP]

is the Accessors annotation mentioned here the one coming from lombok? i.e. lombok.experimental.Accessors

[metacosm]

Good question! @shawkins ?

[shawkins]

@andreaTP yes those are lombok annotations

[andreaTP]

I don't think we should support lombok annotations out-of-the-box in the CRD generator. Also, the viable workaround of using JsonProperty seems to cover all the use-cases, am I missing something?

[andreaTP]

here we have the list of all of the supported annotations, anything else is implicitly not supported, as far as I can tell ...

[metacosm]

I don't think we should support lombok annotations out-of-the-box in the CRD generator. Also, the viable workaround of using JsonProperty seems to cover all the use-cases, am I missing something?

Agreed, I thought that this was a Jackson annotation I didn't know about. I'd be in favor of closing this as rejected, then.

[shawkins]

Agreed, I thought that this was a Jackson annotation I didn't know about. I'd be in favor of closing this as rejected, then.

It's just surprising from a usage perspective. Without the JsonProperty everything works as expected with Jackson, or even the sundrio builder generation. So the CRD logic is doing something different.

[andreaTP]

It's just surprising from a usage perspective. ... the CRD logic is doing something different.

This is correct, the CRD logic is not extracting the name using the logic from sundrio and the only supported way to change a field name currently is using JsonProperty, otherwise the "plain" name of the field is going to be used.

IIRC sundrio Builder annotation is considering also the accessor methods, which is not the case in the CRD generator (so far).

[andreaTP]

@shawkins do you still think we should do any action in this context?

The problem here is that, if we start supporting lombok annotations, we are going to be on the hook for supporting all of them and all of the configuration options they expose.

In this specific case, we have to appreciate the fact that Accessors is marked as experimental API ... I also don't think that "equivalence" with other sundrio features is a goal of the CRD generator.

[shawkins]

@shawkins do you still think we should do any action in this context?

Does this boil down to what order the annotation processing is done in? That is if you worked against the already processed source that contained the actual getter and setter would the crd name be as expected?

[andreaTP]

So, here we are challenging the assumption of extracting the property name from the field name as opposed to the getter/setters. On this subject I don't have a super strong opinion, I believe that relying on the original field name is more idiomatic and involves simpler reasoning, on the contrary, extracting the name based on accessors might be a common enough pattern to be considered.

I'll let @metacosm and @manusa break the tie here, whatever the outcome is I propose we document accordingly the decision taken.

[shawkins]

I believe that relying on the original field name is more idiomatic and involves simpler reasoning

It is, it's just different than Jackson.

whatever the outcome is I propose we document accordingly the decision taken.

Sounds good.

[shawkins]

Using explicit accessors results in the field being omitted entirely from the CRD:

@ToString
@EqualsAndHashCode
@JsonInclude(JsonInclude.Include.NON_NULL)
public class NetworkConfiguration {
    private boolean _private;

    public void setPrivate(boolean pvt) {
        this._private = pvt;
    }

    public boolean isPrivate() {
        return _private;
    }
}

[andreaTP]

Using explicit accessors results in the field being omitted entirely from the CRD

this looks like a bug indeed, it should be easily reproducible in the unit tests though.

[metacosm]

The problem with the accessor detection is that it's pretty dumb: the accessors have to be named after the field and prefixed either by is, get or set. In this case, the field is named _private so the CRD generator would expect the getter to be named is_private. To be fair, I'm not sure this case would work with the regular bean property detection mechanism either…

Without an associated field that we can rely on to be matched to the accessor methods and without adding too many specific corner cases, we can't be sure the methods are supposed to be properties or just synthetic fields that should not be in the generated CRD. Looking at the code, a human can easily make that decision, however, without looking at the method implementation, it's a lot less easy.

So the question here becomes whether or not we want to support this corner case where the field is named after a Java keyword and is prefixed by _. Written as such, this starts to look like a stretch already ;)

[metacosm]

To be clear, it's probably easy to implement. The question is whether we want to add this feature or not.

[andreaTP]

So the question here becomes whether or not we want to support this corner case where the field is named after a Java keyword and is prefixed by _. Written as such, this starts to look like a stretch already ;)

I agree this is a stretch, but we should probably emit a warning in this case.

[metacosm]

So the question here becomes whether or not we want to support this corner case where the field is named after a Java keyword and is prefixed by _. Written as such, this starts to look like a stretch already ;)

I agree this is a stretch, but we should probably emit a warning in this case.

Problem is detecting the case to emit a warning is probably equivalent to supporting the "feature" in term of complexity.

[stale[bot]]

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!