Skip to content

Commit

Permalink
fix: Schema for multimaps is wrong
Browse files Browse the repository at this point in the history 
Upgraded sundrio and added a test to validate that multimaps (like `class MultiMap<K,V> implements Map<K,List<V>>`) are generated correctly.

Fixes fabric8io#4487
  • Loading branch information
@xRodney
xRodney committed Oct 12, 2022
1 parent eed73e1 commit fec526e
Show file tree
Hide file tree
Showing 4 changed files with 41 additions and 58 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Expand Up @@ -6,6 +6,7 @@
* 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
Expand Down
74 changes: 30 additions & 44 deletions crd-generator/api/src/main/java/io/fabric8/crd/generator/utils/Types.java
View file
Expand Up @@ -25,10 +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 @@ -37,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 @@ -49,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 @@ -66,52 +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();
if (parameters.size() != arguments.size()) {
throw new IllegalStateException("Incompatible reference " + ref + " to " + definition);
}

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(new ApplyTypeParamMappingToTypeArguments(mappings)) // existing type arguments must be handled before methods and properties
.accept(new ApplyTypeParamMappingToProperty(mappings),
new ApplyTypeParamMappingToMethod(mappings))
.build();
}

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 @@ -130,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 @@ -216,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 @@ -267,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
22 changes: 9 additions & 13 deletions crd-generator/api/src/test/java/io/fabric8/crd/generator/CRDGeneratorTest.java
View file
Expand Up @@ -277,29 +277,25 @@ void mapPropertyShouldHaveCorrectValueType() {

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);

// this check is currently failing, because multimaps are incorrectly processed as if they were normal maps
// (class MultiMap<K,V> implements Map<K,List<V>> is treated like just Map<K,V>)
// checkMapProp(specProps, "test3", "object");
final JSONSchemaProps props = specProps.get("test3");
assertEquals("object", props.getType());

assertEquals("boolean", valueSchema.getItems().getSchema().getType());

JSONSchemaProps test3Schema = checkMapProp(specProps, "test", "array");
assertEquals("string", test3Schema.getItems().getSchema().getType());
});
}

private void checkMapProp(Map<String, JSONSchemaProps> specProps, String name, String valueType) {
private JSONSchemaProps checkMapProp(Map<String, JSONSchemaProps> 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
Expand Down
2 changes: 1 addition & 1 deletion pom.xml
View file
Expand Up @@ -80,7 +80,7 @@
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>

<!-- Core versions -->
<sundrio.version>0.94-bugfix-multi-maps3-SNAPSHOT</sundrio.version>
<sundrio.version>0.93-SNAPSHOT</sundrio.version>
<okhttp.version>3.12.12</okhttp.version>
<okhttp.bundle.version>3.12.1_1</okhttp.bundle.version>
<okio.version>1.15.0</okio.version>
Expand Down

0 comments on commit fec526e

Please sign in to comment.