Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix StackOverflow on cyclic references involving collections. #4512

Merged
merged 1 commit into from
Nov 16, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
#### Bugs
* Fix #4159: ensure the token refresh obeys how the Config was created
* Fix #4491: added a more explicit shutdown exception for okhttp
* Fix #4510: Fix StackOverflow on cyclic references involving collections.
* Fix #4534: Java Generator CLI default handling of skipGeneratedAnnotations
* Fix #4535: The shell command string will now have single quotes sanitized
* Fix #4543: (Java Generator) additionalProperties JsonAny setter method generated as setAdditionalProperty
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ public class ClassDependenciesVisitor extends TypedVisitor<TypeDefBuilder> {
private static final Map<String, Set<String>> traversedClasses = new HashMap<>();
private static final Map<String, Set<String>> crdNameToCrClass = new HashMap<>();
private final Set<String> classesForCR;
private final Set<String> processed = new HashSet<>();
private final Set<ClassRef> processed = new HashSet<>();

public ClassDependenciesVisitor(String crClassName, String crdName) {
// need to record all classes associated with the different versions of the CR (not the CRD spec)
Expand All @@ -49,7 +49,7 @@ public void visit(TypeDefBuilder builder) {
// note that we cannot simply check the traversed class set to know if a class has been processed because classes
// are usually added to the traversed set before they're looked at in depth
final String className = type.getFullyQualifiedName();
if (ignore(className) || processed.contains(className)) {
if (ignore(className)) {
return;
}

Expand All @@ -74,19 +74,21 @@ public void visit(TypeDefBuilder builder) {

// add classes from extends list
type.getExtendsList().forEach(this::processTypeRef);

// add class to the processed classes
processed.add(className);
}

private boolean ignore(String className) {
return (className.startsWith("java.") && !className.startsWith("java.util.")) || className.startsWith("com.fasterxml.jackson") || className.startsWith("jdk.");
return (className.startsWith("java.") && !className.startsWith("java.util."))
|| className.startsWith("com.fasterxml.jackson") || className.startsWith("jdk.");
}

private void processTypeRef(TypeRef t) {
if (t instanceof ClassRef) {
ClassRef classRef = (ClassRef) t;
visit(new TypeDefBuilder(Types.typeDefFrom(classRef)));
// only process the class reference if we haven't already
// note that the references are stored in the set including type arguments, so List<A> and List<B> are not the same
if (processed.add(classRef)) {
visit(new TypeDefBuilder(Types.typeDefFrom(classRef)));
}
}
}

Expand All @@ -101,7 +103,7 @@ public static Set<String> getDependentClasses(String crClassName) {
public static Set<String> getDependentClassesFromCRDName(String crdName) {
// retrieve all dependent classes that might affect any of the CR versions
return crdNameToCrClass.get(crdName).stream()
.flatMap(crClassName -> traversedClasses.get(crClassName).stream())
.collect(Collectors.toSet());
.flatMap(crClassName -> traversedClasses.get(crClassName).stream())
.collect(Collectors.toSet());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
/**
* Copyright (C) 2015 Red Hat, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.fabric8.crd.example.cyclic;

import io.fabric8.kubernetes.api.model.Namespaced;
import io.fabric8.kubernetes.client.CustomResource;
import io.fabric8.kubernetes.model.annotation.Group;
import io.fabric8.kubernetes.model.annotation.Version;

@Group("sample.fabric8.io")
@Version("v1alpha1")
public class CyclicList extends CustomResource<CyclicListSpec, CyclicStatus> implements Namespaced {

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
/**
* Copyright (C) 2015 Red Hat, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.fabric8.crd.example.cyclic;

import java.util.List;

public class CyclicListSpec {
private List<RefList> ref;
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
/**
* Copyright (C) 2015 Red Hat, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.fabric8.crd.example.cyclic;

import java.util.List;

public class RefList {

private List<RefList> ref;

}
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import io.fabric8.crd.example.basic.BasicSpec;
import io.fabric8.crd.example.basic.BasicStatus;
import io.fabric8.crd.example.cyclic.Cyclic;
import io.fabric8.crd.example.cyclic.CyclicList;
import io.fabric8.crd.example.inherited.*;
import io.fabric8.crd.example.joke.Joke;
import io.fabric8.crd.example.joke.JokeRequest;
Expand Down Expand Up @@ -216,6 +217,19 @@ void generatingACycleShouldFail() {
"An IllegalArgument Exception hasn't been thrown when generating a CRD with cyclic references");
}

@Test
void generatingACycleInListShouldFail() {
final CRDGenerator generator = new CRDGenerator()
.customResourceClasses(CyclicList.class)
.forCRDVersions("v1", "v1beta1")
.withOutput(output);

assertThrows(
IllegalArgumentException.class,
() -> generator.detailedGenerate(),
"An IllegalArgument Exception hasn't been thrown when generating a CRD with cyclic references");
}

@Test
void notGeneratingACycleShouldSucceed() {
final CRDGenerator generator = new CRDGenerator()
Expand Down