From b1998f25465aa6a2c402e1ef6bd676afa5c13f7a 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), schema for multimaps is wrong 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 the same algorithm. A test has been added to validate that multimaps (like `class MultiMap implements Map>`) are generated correctly. Fixes fabric8io/kubernetes-client#4357 Fixes fabric8io/kubernetes-client#4487 --- CHANGELOG.md | 3 + .../io/fabric8/crd/generator/utils/Types.java | 122 +++++------------- .../crd/example/map/ContainingMapsSpec.java | 4 + .../crd/generator/CRDGeneratorTest.java | 18 +-- pom.xml | 2 +- 5 files changed, 51 insertions(+), 98 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c0a31e67b39..46e3751bcfc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ #### 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) +* Fix #4487: Schema for multi maps is now generated correctly * Fix #3733: The authentication command from the .kube/config won't be discarded if no arguments are specified * Fix #4441: corrected patch base handling for the patch methods available from a Resource - resource(item).patch() will be evaluated as resource(latest).patch(item). Also undeprecated patch(item), which is consistent with leaving patch(context, item) undeprecated as well. For consistency with the other operations (such as edit), patch(item) will use the context item as the base when available, or the server side item when not. This means that patch(item) is only the same as resource(item).patch() when the patch(item) is called when the context item is missing or is the same as the latest. * Fix #4442: TokenRefreshInterceptor doesn't overwrite existing OAuth token with empty string @@ -25,6 +27,7 @@ * Fix #4243: Update Tekton triggers model to v0.20.2 * Fix #4383: bump snakeyaml from 1.30 to 1.31 * Fix #4347: Update Kubernetes Model to v1.25.0 +* Fix #4413: update sundrio to 0.93.1 #### New Features * Fix #4398: add annotation @PreserveUnknownFields for marking generated field have `x-kubernetes-preserve-unknown-fields: true` defined 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 a4f9498f323..90909672be3 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.utils.TypeArguments; +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; @@ -33,7 +37,6 @@ import java.util.stream.Collectors; import java.util.stream.Stream; - public class Types { private Types() { throw new IllegalStateException("Utility class"); @@ -45,7 +48,8 @@ private Types() { public static final AdapterContext REFLECTION_CONTEXT = AdapterContext.create(DefinitionRepository.getRepository()); /** - * Make sure the generation context is reset so that types can be properly introspected if classes have changed since the last generation round. + * Make sure the generation context is reset so that types can be properly introspected if classes have changed since the last + * generation round. */ public static void resetGenerationContext() { DefinitionRepository.getRepository().clear(); @@ -62,10 +66,10 @@ public static TypeDef unshallow(TypeDef definition) { final List properties = Types.projectProperties(definition); // re-create TypeDef with all the needed information return new TypeDef(definition.getKind(), definition.getPackageName(), - definition.getName(), definition.getComments(), definition.getAnnotations(), classRefs, - definition.getImplementsList(), definition.getParameters(), properties, - definition.getConstructors(), definition.getMethods(), definition.getOuterTypeName(), - definition.getInnerTypes(), definition.getModifiers(), definition.getAttributes()); + definition.getName(), definition.getComments(), definition.getAnnotations(), classRefs, + definition.getImplementsList(), definition.getParameters(), properties, + definition.getConstructors(), definition.getMethods(), definition.getOuterTypeName(), + definition.getInnerTypes(), definition.getModifiers(), definition.getAttributes()); } public static TypeDef typeDefFrom(ClassRef classRef) { @@ -73,87 +77,30 @@ public static TypeDef typeDefFrom(ClassRef classRef) { } private static TypeDef projectDefinition(ClassRef ref) { - List arguments = ref.getArguments(); TypeDef definition = GetDefinition.of(ref); - if (arguments.isEmpty()) { + Map mappings = TypeArguments.getGenericArgumentsMappings(ref, definition); + if (mappings.isEmpty()) { return definition; + } else { + return new TypeDefBuilder(definition) + .accept(new ApplyTypeParamMappingToTypeArguments(mappings)) // existing type arguments must be handled before methods and properties + .accept(new ApplyTypeParamMappingToProperty(mappings), + new ApplyTypeParamMappingToMethod(mappings)) + .build(); } - - List parameters = definition.getParameters(); - Map mappings = new HashMap<>(); - for (int i = 0; i < arguments.size(); i++) { - String name = parameters.get(i).getName(); - TypeRef typeRef = arguments.get(i); - mappings.put(name, typeRef); - } - - return new TypeDefBuilder(definition) - .accept(mapClassRefArguments(mappings), mapGenericProperties(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() - .flatMap(s -> Stream.concat(Stream.of(s), projectDefinition(s).getExtendsList().stream())) - .collect(Collectors.toSet()); + .flatMap(s -> Stream.concat(Stream.of(s), projectDefinition(s).getExtendsList().stream())) + .collect(Collectors.toSet()); } /** * All non-static properties (including inherited). - * @param typeDef The type. + * + * @param typeDef The type. * @return A list with all properties. */ private static List projectProperties(TypeDef typeDef) { @@ -172,23 +119,20 @@ private static List projectProperties(TypeDef typeDef) { return !p.isStatic(); }), typeDef.getExtendsList().stream() - .filter(e -> !e.getFullyQualifiedName().startsWith("java.")) - .flatMap(e -> projectProperties(projectDefinition(e)) - .stream() - .filter(p -> filterCustomResourceProperties(e).test(p))) - ) + .filter(e -> !e.getFullyQualifiedName().startsWith("java.")) + .flatMap(e -> projectProperties(projectDefinition(e)) + .stream() + .filter(p -> filterCustomResourceProperties(e).test(p)))) - .collect(Collectors.toList()); + .collect(Collectors.toList()); } - private static Predicate filterCustomResourceProperties(ClassRef ref) { return p -> !p.isStatic() && - (!ref.getFullyQualifiedName().equals(CUSTOM_RESOURCE_NAME) || - (p.getName().equals("spec") || p.getName().equals("status"))); + (!ref.getFullyQualifiedName().equals(CUSTOM_RESOURCE_NAME) || + (p.getName().equals("spec") || p.getName().equals("status"))); } - public static void output(TypeDef def) { final StringBuilder builder = new StringBuilder(300); Types.describeType(def, "", new HashSet<>(), builder); @@ -258,7 +202,7 @@ public boolean isUnreliable() { public static SpecAndStatus resolveSpecAndStatusTypes(TypeDef definition) { Optional optionalCustomResourceRef = definition.getExtendsList().stream() - .filter(s -> s.getFullyQualifiedName().equals(CUSTOM_RESOURCE_NAME)).findFirst(); + .filter(s -> s.getFullyQualifiedName().equals(CUSTOM_RESOURCE_NAME)).findFirst(); if (optionalCustomResourceRef.isPresent()) { ClassRef customResourceRef = optionalCustomResourceRef.get(); List arguments = customResourceRef.getArguments(); @@ -309,14 +253,14 @@ public static boolean isNamespaced(TypeDef definition, Set visited) { */ public static Optional findStatusProperty(TypeDef typeDef) { return typeDef.getProperties().stream() - .filter(Types::isStatusProperty) - .findFirst(); + .filter(Types::isStatusProperty) + .findFirst(); } - /** * Returns true if the specified property corresponds to status. * A property qualifies as `status` if it is called `status`. + * * @param property the property we want to check * @return {@code true} if named {@code status}, {@code false} otherwise */ 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 e09a7ee8f02..8e11f1ebd45 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 a1f96364356..ce2ba9b0185 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,25 +275,27 @@ 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(); - assertEquals("string", arrayType); + JSONSchemaProps testSchema = checkMapProp(specProps, "test", "array"); + assertEquals("string", testSchema.getItems().getSchema().getType()); - checkMapProp(specProps, "test2", "object"); - JSONSchemaProps valueSchema = specProps.get("test2").getAdditionalProperties().getSchema().getAdditionalProperties().getSchema(); + JSONSchemaProps test2Schema = checkMapProp(specProps, "test2", "object"); + JSONSchemaProps valueSchema = test2Schema.getAdditionalProperties().getSchema(); String valueType = valueSchema.getType(); assertEquals("array", valueType); - assertEquals("boolean", valueSchema.getItems().getSchema().getType()); + + JSONSchemaProps test3Schema = checkMapProp(specProps, "test", "array"); + assertEquals("string", test3Schema.getItems().getSchema().getType()); }); } - private void checkMapProp(Map specProps, String name, String valueType) { + private JSONSchemaProps checkMapProp(Map specProps, String name, String valueType) { final JSONSchemaProps props = specProps.get(name); assertEquals("object", props.getType()); assertEquals(valueType, props.getAdditionalProperties().getSchema().getType()); + return props.getAdditionalProperties().getSchema(); } @Test diff --git a/pom.xml b/pom.xml index a88edc0b440..592ce1ffab9 100644 --- a/pom.xml +++ b/pom.xml @@ -80,7 +80,7 @@ UTF-8 - 0.93.0 + 0.93.1 3.12.12 3.12.1_1 1.15.0