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
xRodney committed Oct 18, 2022
1 parent 177de1c commit bf5df02
Show file tree
Hide file tree
Showing 5 changed files with 100 additions and 138 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -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 multimaps (and other non-trivial generic classes)
* Fix #4487: Schema for multimaps 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
Expand All @@ -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
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
5 changes: 5 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,8 @@ public Map<String, Map<String, List<Boolean>>> getTest2() {
return test2;
}

private MyMultiMap<String, String> test3;

class MyMultiMap<K, V> extends HashMap<K, List<V>> {
}
}

0 comments on commit bf5df02

Please sign in to comment.