From 1a3960ba2177186b7105d7d2636f4c27646fef58 Mon Sep 17 00:00:00 2001 From: Dusan Jakub Date: Tue, 13 Sep 2022 11:43:26 +0200 Subject: [PATCH] fix: Stack overflow on multi maps (and other non-trivial generic classes) Logic in sundrio that is responsible for replacing generic type arguments on methods and properties with their concrete instantiations from subclasses contained a bug, in which it looped infinitely if the same type argument name was used at multiple classes in the hierarchy. A common occurence were multimaps. A very similar code was also present in crd-generator, also suffering from the same bug. Sundrio has been updated to account for this situation, and updated utilities from sundrio have been introduced to crd-generator, which replace the previous implementation of teh same algorithm. Fixes fabric8io/kubernetes-client#4357 --- CHANGELOG.md | 2 + .../io/fabric8/crd/generator/utils/Types.java | 64 ++++--------------- .../crd/example/map/ContainingMapsSpec.java | 4 ++ .../crd/generator/CRDGeneratorTest.java | 8 ++- pom.xml | 2 +- 5 files changed, 25 insertions(+), 55 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e2aad570fc..dbd954bce8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ #### Bugs * Fix #4369: Informers will retry with a backoff on list/watch failure as they did in 5.12 and prior. * Fix #4350: SchemaSwap annotation is now repeatable and is applied multiple times if classes are used more than once in the class hierarchy. +* Fix #4413: Stack overflow on multi maps (and other non-trivial generic classes) #### Improvements * Fix #4348: Introduce specific annotations for the generators @@ -15,6 +16,7 @@ * Fix #4243: Update Tekton pipeline model to v0.39.0 * Fix #4243: Update Tekton triggers model to v0.20.2 * Fix #4383: bump snakeyaml from 1.30 to 1.31 +* Fix #4413: update sundrio to 0.94 #### New Features diff --git a/crd-generator/api/src/main/java/io/fabric8/crd/generator/utils/Types.java b/crd-generator/api/src/main/java/io/fabric8/crd/generator/utils/Types.java index a4f9498f32..8cf88bb4cf 100644 --- a/crd-generator/api/src/main/java/io/fabric8/crd/generator/utils/Types.java +++ b/crd-generator/api/src/main/java/io/fabric8/crd/generator/utils/Types.java @@ -25,6 +25,10 @@ import io.sundr.model.*; import io.sundr.model.functions.GetDefinition; import io.sundr.model.repo.DefinitionRepository; +import io.sundr.model.visitors.ApplyTypeParamMappingToMethod; +import io.sundr.model.visitors.ApplyTypeParamMappingToProperty; +import io.sundr.model.visitors.ApplyTypeParamMappingToTypeArguments; + import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -80,6 +84,10 @@ private static TypeDef projectDefinition(ClassRef ref) { } List parameters = definition.getParameters(); + if (parameters.size() != arguments.size()) { + throw new IllegalStateException("Incompatible reference " + ref + " to " + definition); + } + Map mappings = new HashMap<>(); for (int i = 0; i < arguments.size(); i++) { String name = parameters.get(i).getName(); @@ -88,62 +96,12 @@ private static TypeDef projectDefinition(ClassRef ref) { } return new TypeDefBuilder(definition) - .accept(mapClassRefArguments(mappings), mapGenericProperties(mappings)) + .accept(new ApplyTypeParamMappingToTypeArguments(mappings)) // existing type arguments must be handled before methods and properties + .accept(new ApplyTypeParamMappingToProperty(mappings), + new ApplyTypeParamMappingToMethod(mappings)) .build(); } - /** - * Map generic properties to known {@link TypeRef} based on the specified mappings. - * Example: Given a property {@code T size} and a map containing {@code T -> Integer} the final - * property will be: {@code Integer type}. - * @param mappings A map that maps class arguments names to types. - * @return a visitors that performs the actual mapping. - */ - private static TypedVisitor mapGenericProperties(Map mappings) { - return new TypedVisitor() { - @Override - public void visit(PropertyBuilder property) { - TypeRef typeRef = property.buildTypeRef(); - if (typeRef instanceof TypeParamRef) { - TypeParamRef typeParamRef = (TypeParamRef) typeRef; - String key = typeParamRef.getName(); - TypeRef paramRef = mappings.get(key); - if (paramRef != null) { - property.withTypeRef(paramRef); - } - } - } - }; - } - - /** - * Map arguments, of {@link ClassRef} instances to known {@link TypeRef} based on the specified mappings. - * Example: Given a class reference to {@code Shape} and a map containing {@code T -> Integer} - * the final reference will be: {@code Shape type}. - * @param mappings A map that maps class arguments names to types. - * @return a visitors that performs the actual mapping. - */ - private static TypedVisitor mapClassRefArguments(Map mappings) { - return new TypedVisitor() { - @Override - public void visit(ClassRefBuilder c) { - List arguments = new ArrayList<>(); - for (TypeRef arg : c.buildArguments()) { - TypeRef mappedRef = arg; - if (arg instanceof TypeParamRef) { - TypeParamRef typeParamRef = (TypeParamRef) arg; - TypeRef mapping = mappings.get(typeParamRef.getName()); - if (mapping != null) { - mappedRef = mapping; - } - } - arguments.add(mappedRef); - } - c.withArguments(arguments); - } - }; - } - private static Set projectSuperClasses(TypeDef definition) { List extendsList = definition.getExtendsList(); return extendsList.stream() diff --git a/crd-generator/api/src/test/java/io/fabric8/crd/example/map/ContainingMapsSpec.java b/crd-generator/api/src/test/java/io/fabric8/crd/example/map/ContainingMapsSpec.java index e09a7ee8f0..8e11f1ebd4 100644 --- a/crd-generator/api/src/test/java/io/fabric8/crd/example/map/ContainingMapsSpec.java +++ b/crd-generator/api/src/test/java/io/fabric8/crd/example/map/ContainingMapsSpec.java @@ -15,6 +15,7 @@ */ package io.fabric8.crd.example.map; +import java.util.HashMap; import java.util.List; import java.util.Map; @@ -32,4 +33,7 @@ public Map>> getTest2() { return test2; } + private MyMultiMap test3; + + class MyMultiMap extends HashMap> {} } diff --git a/crd-generator/api/src/test/java/io/fabric8/crd/generator/CRDGeneratorTest.java b/crd-generator/api/src/test/java/io/fabric8/crd/generator/CRDGeneratorTest.java index a1f9636435..47868cd3a5 100644 --- a/crd-generator/api/src/test/java/io/fabric8/crd/generator/CRDGeneratorTest.java +++ b/crd-generator/api/src/test/java/io/fabric8/crd/generator/CRDGeneratorTest.java @@ -275,7 +275,7 @@ void mapPropertyShouldHaveCorrectValueType() { final Map specProps = version.getSchema().getOpenAPIV3Schema() .getProperties().get("spec").getProperties(); - assertEquals(2, specProps.size()); + assertEquals(3, specProps.size()); checkMapProp(specProps, "test", "array"); String arrayType = specProps.get("test").getAdditionalProperties().getSchema().getItems().getSchema().getType(); @@ -286,6 +286,12 @@ void mapPropertyShouldHaveCorrectValueType() { String valueType = valueSchema.getType(); assertEquals("array", valueType); + // this check is currently failing, because multimaps are incorrectly processed as if they were normal maps + // (class MultiMap implements Map> is treated like just Map) + // checkMapProp(specProps, "test3", "object"); + final JSONSchemaProps props = specProps.get("test3"); + assertEquals("object", props.getType()); + assertEquals("boolean", valueSchema.getItems().getSchema().getType()); }); } diff --git a/pom.xml b/pom.xml index 0ec6b9c84c..cc11a9c4cf 100644 --- a/pom.xml +++ b/pom.xml @@ -80,7 +80,7 @@ UTF-8 - 0.93.0 + 0.94-bugfix-multi-maps3-SNAPSHOT 3.12.12 3.12.1_1 1.15.0