Fix StackOverflow on cyclic references involving collections.

[xRodney]

Description

The logic in ClassDependenciesVisitor was prone to SO when there were cyclic references in the schema. This should fix it, and the tests are still passing, so hopefully it is safe.

Fixes #4510

TBH, I do not fully understand the purpose of ClassDependenciesVisitor, I realize it it used in tests and maybe when someone would want to include the generator in their own code? I would be grateful for some pointers.

It appears that it exists mostly to facilitate conditional hot reload when used with quarkus, as implemented here https://github.com/quarkiverse/quarkus-operator-sdk/blob/main/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/OperatorSDKProcessor.java#L187

This still works with my changes, see my comment below.

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]

Hey @xRodney ! Thanks for this follow-up!

Arrived at this point do you think it's worth adding an integration test using the RealmRepresentation from the published jar keycloak-models?

Regarding your question about ClassDependenciesVisitor @metacosm should be the best person to answer.

[xRodney]

@andreaTP, I have added keycloak test in the latest commit, just to see how that would look like.
I'll leave it up to you and the team to decide if it is a good idea.

[andreaTP]

Thanks, a lot @xRodney ! At least, this guarantees us that we are at the end of the tunnel 🙂 super glad you picked it up!

Let @manusa and @rohanKanojia decide whether they think adding this hard dependency to test is a good idea or not.
Meanwhile, this guarantees us some kind of confidence this is going to work 🥳

[xRodney]

Hi, @manusa and @rohanKanojia, do you have any opinion on this?
If you feel this needs a deeper discussion, I would be happy to extract the Keycloak tests to a separate PR and just fix the bug here. It's up to you.

[xRodney]

Okay, so regarding my original uncertainty regarding the purpose of ClassDependenciesVisitor:
Iit appears that it exists mostly to facilitate conditional hot reload when used with quarkus, as implemented here https://github.com/quarkiverse/quarkus-operator-sdk/blob/main/core/deployment/src/main/java/io/quarkiverse/operatorsdk/deployment/OperatorSDKProcessor.java#L187
Originally, I was testing it as a raw java operator, but with quarkus it really shines (when it does not SO, I mean :D )
So I booted up quarkus-operator-sdk and replaced dependencies with my changes from this PR, and hot reload seems to work as expected. So I think we should be fine.

About Keycloak dependency:
Since there seems to be not much interest, I propose to exclude it to a separate PR. I really wish that the bugfix makes it to the 6.3 release, and I feel that the (lack of) discussion about a test would unnecessarily delay the merge.

[manusa]

Hi, sorry for the delayed response, it's been 4 long hectic weeks and I'm now catching up with everything

About Keycloak dependency:

Yes, it's better we discuss this separately. In general it's better to proceed this way since otherwise things get delayed

[manusa]

code-wise looks good.
🙏 Andrea/Chris check if feature-wise it's OK.

[andreaTP]

LGTM, thanks for the separate PR on the Keycloak CR 👍

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

94.4% Coverage
0.0% Duplication