Skip to content

Commit

Permalink
fix: Stack overflow on multimaps (and other non-trivial generic class…
Browse files Browse the repository at this point in the history 
…es), 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<K,V> implements Map<K,List<V>>`) are generated correctly.

Fixes fabric8io#4357
Fixes fabric8io#4487
  • Loading branch information
@xRodney @manusa
xRodney authored and manusa committed Oct 19, 2022
1 parent c033a82 commit 0156d0f
Show file tree
Hide file tree
Showing 8 changed files with 271 additions and 140 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -15,6 +15,7 @@
* Fix #4473: Fix regression in backoff interval introduced in #4365
* Fix #4478: Removing the resourceVersion bump with null status
* Fix #4482: Fixing blocking behavior of okhttp log watch
* Fix #4487: Schema for multimaps is now generated correctly

#### Improvements
* Fix #4471: Adding KubernetesClientBuilder.withHttpClientBuilderConsumer to further customize the HttpClient for any implementation.
Expand All @@ -30,6 +31,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
Expand Down
122 changes: 33 additions & 89 deletions crd-generator/api/src/main/java/io/fabric8/crd/generator/utils/Types.java
View file
Expand Up @@ -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;

Expand All @@ -33,7 +37,6 @@
import java.util.stream.Collectors;
import java.util.stream.Stream;


public class Types {
private Types() {
throw new IllegalStateException("Utility class");
Expand All @@ -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();
Expand All @@ -62,98 +66,41 @@ public static TypeDef unshallow(TypeDef definition) {
final List<Property> 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) {
return unshallow(GetDefinition.of(classRef));
}

private static TypeDef projectDefinition(ClassRef ref) {
List<TypeRef> arguments = ref.getArguments();
TypeDef definition = GetDefinition.of(ref);
if (arguments.isEmpty()) {
Map<String, TypeRef> 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<TypeParamDef> parameters = definition.getParameters();
Map<String, TypeRef> 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<PropertyBuilder> mapGenericProperties(Map<String, TypeRef> mappings) {
return new TypedVisitor<PropertyBuilder>() {
@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<T>} and a map containing {@code T -> Integer}
* the final reference will be: {@code Shape<Integer> type}.
* @param mappings A map that maps class arguments names to types.
* @return a visitors that performs the actual mapping.
*/
private static TypedVisitor<ClassRefBuilder> mapClassRefArguments(Map<String, TypeRef> mappings) {
return new TypedVisitor<ClassRefBuilder>() {
@Override
public void visit(ClassRefBuilder c) {
List<TypeRef> 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<ClassRef> projectSuperClasses(TypeDef definition) {
List<ClassRef> 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<Property> projectProperties(TypeDef typeDef) {
Expand All @@ -172,23 +119,20 @@ private static List<Property> 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<Property> 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);
Expand Down Expand Up @@ -258,7 +202,7 @@ public boolean isUnreliable() {

public static SpecAndStatus resolveSpecAndStatusTypes(TypeDef definition) {
Optional<ClassRef> 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<TypeRef> arguments = customResourceRef.getArguments();
Expand Down Expand Up @@ -309,14 +253,14 @@ public static boolean isNamespaced(TypeDef definition, Set<TypeDef> visited) {
*/
public static Optional<Property> 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
*/
Expand Down
29 changes: 29 additions & 0 deletions crd-generator/api/src/test/java/io/fabric8/crd/example/map/ContainingMapsSpec.java
View file
Expand Up @@ -15,6 +15,7 @@
*/
package io.fabric8.crd.example.map;

import java.util.HashMap;
import java.util.List;
import java.util.Map;

Expand All @@ -32,4 +33,32 @@ public Map<String, Map<String, List<Boolean>>> getTest2() {
return test2;
}

private MultiHashMap<String, Integer> stringToIntMultiMap1;
private MultiMap<String, Integer> stringToIntMultiMap2;
private SwappedParametersMap<List<Integer>, String> stringToIntMultiMap3;
private RedundantParametersMap<Object, Runnable, String, List<Integer>> stringToIntMultiMap4;
private RedundantParametersStringToIntMultiMap<Integer, Long> stringToIntMultiMap5;
private StringKeyedMultiHashMap<Integer> stringToIntMultiMap6;
private IntValuedMultiMap<String> stringToIntMultiMap7;

static class MultiHashMap<K, V> extends HashMap<K, List<V>> {
}

interface MultiMap<K, V> extends Map<K, List<V>> {
}

interface SwappedParametersMap<V, K> extends Map<K, V> {
}

interface RedundantParametersMap<A, B, K, V> extends Map<K, V> {
}

interface RedundantParametersStringToIntMultiMap<K, V> extends Map<String, List<Integer>> {
}

static class StringKeyedMultiHashMap<V> extends MultiHashMap<String, V> {
}

interface IntValuedMultiMap<K> extends MultiMap<K, Integer> {
}
}

0 comments on commit 0156d0f

Please sign in to comment.