Skip to content

Commit

Permalink
More CRD generator improvements (#3022)
Browse files Browse the repository at this point in the history 
* feat: make generator more configurable, with better reporting and tests

* fix: only output type def and crd if the test failed

* feat: only project definition / properties once

* fix: simplify status property detection

* fix: properly handle enum types for additional printer columns

Fixes #3011

* chore(tests): add tests for #3020

Fixing this issue will require a new sundrio release.

* chore(docs): update CHANGELOG

* chore(tests): update to sundrio 0.30.4

* chore(docs): update CHANGELOG

* fix: output format

* fix: TypeDef from TypeElement also needs to be "unshallowed"

* chore: clean-up

(cherry picked from commit 91a0078)
  • Loading branch information
@metacosm @manusa
metacosm authored and manusa committed Apr 26, 2021
1 parent 2ad2804 commit dbe50de
Show file tree
Hide file tree
Showing 15 changed files with 174 additions and 120 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,8 @@
* Fix #2994: updating the SharedIndexInformer indexer state for a delete event generated by resync
* Fix #2910: Move crd-generator tests from kubernetes-itests to kubernetes-tests
* Fix #3005: Make it possible to select which CRD version is generated / improve output
* Fix #3011: properly handle enum types for additional printer columns
* Fix #3020: annotations should now properly have their associated values when processing CRDs from the API

### 4.13.3 (2021-04-22)

Expand Down
26 changes: 2 additions & 24 deletions crd-generator/api/src/main/java/io/fabric8/crd/generator/AbstractCustomResourceHandler.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@
*/
package io.fabric8.crd.generator;

import static io.fabric8.crd.generator.utils.Types.findStatusProperty;

import io.fabric8.crd.generator.decorator.Decorator;
import io.fabric8.crd.generator.visitor.AdditionalPrinterColumnDetector;
import io.fabric8.crd.generator.visitor.LabelSelectorPathDetector;
Expand Down Expand Up @@ -55,31 +53,12 @@ public void handle(CustomResourceInfo config) {
LabelSelectorPathDetector labelSelectorPathDetector = new LabelSelectorPathDetector();
AdditionalPrinterColumnDetector additionalPrinterColumnDetector = new AdditionalPrinterColumnDetector();

boolean statusExists = config.statusClassName().isPresent();
if (statusExists) {
TypeDefBuilder builder = new TypeDefBuilder(def);
Optional<Property> statusProperty = findStatusProperty(def);
if (statusProperty.isPresent()) {
Property p = statusProperty.get();
builder.removeFromProperties(p);

def = builder
.addNewProperty()
.withName("status")
.withTypeRef(p.getTypeRef())
.endProperty()
.build();
} else {
statusExists = false;
}
}

TypeDefBuilder builder = new TypeDefBuilder(def);
if (config.specClassName().isPresent()) {
builder.accept(specReplicasPathDetector);
}

if (statusExists) {
if (config.statusClassName().isPresent()) {
builder.accept(statusReplicasPathDetector);
}

Expand All @@ -97,8 +76,7 @@ public void handle(CustomResourceInfo config) {
Map<String, Object> parameters = property.getAnnotations().stream()
.filter(a -> a.getClassRef().getName().equals("PrinterColumn")).map(a -> a.getParameters())
.findFirst().orElse(Collections.emptyMap());
String type = AbstractJsonSchema.COMMON_MAPPINGS
.getOrDefault(property.getTypeRef(), "object");
String type = AbstractJsonSchema.getSchemaTypeFor(property.getTypeRef());
String column = (String) parameters.get("name");
if (Utils.isNullOrEmpty(column)) {
column = property.getName().toUpperCase();
Expand Down
18 changes: 14 additions & 4 deletions crd-generator/api/src/main/java/io/fabric8/crd/generator/AbstractJsonSchema.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public abstract class AbstractJsonSchema<T, B> {
protected static final TypeRef P_BOOLEAN_REF = new PrimitiveRefBuilder().withName(BOOLEAN_MARKER)
.build();

protected static final Map<TypeRef, String> COMMON_MAPPINGS = new HashMap<>();
private static final Map<TypeRef, String> COMMON_MAPPINGS = new HashMap<>();

static {
COMMON_MAPPINGS.put(STRING_REF, STRING_MARKER);
Expand All @@ -99,6 +99,16 @@ public abstract class AbstractJsonSchema<T, B> {
COMMON_MAPPINGS.put(DURATION_REF, STRING_MARKER);
}

public static String getSchemaTypeFor(TypeRef typeRef) {
String type = COMMON_MAPPINGS.get(typeRef);
if (type == null && typeRef instanceof ClassRef) { // Handle complex types
ClassRef classRef = (ClassRef) typeRef;
TypeDef def = Types.typeDefFrom(classRef);
type = def.isEnum() ? STRING_MARKER : "object";
}
return type;
}

/**
* Creates the JSON schema for the particular {@link TypeDef}. This is template method where
* sub-classes are supposed to provide specific implementations of abstract methods.
Expand All @@ -113,8 +123,8 @@ protected T internalFrom(TypeDef definition, String... ignore) {
ignore.length > 0 ? new LinkedHashSet<>(Arrays.asList(ignore)) : Collections
.emptySet();
List<String> required = new ArrayList<>();

for (Property property : Types.projectProperties(definition)) {
for (Property property : definition.getProperties()) {
final String name = property.getName();

if (property.isStatic() || ignores.contains(name)) {
Expand Down Expand Up @@ -185,7 +195,7 @@ public T internalFrom(TypeRef typeRef) {
} else {
if (typeRef instanceof ClassRef) { // Handle complex types
ClassRef classRef = (ClassRef) typeRef;
TypeDef def = classRef.getDefinition();
TypeDef def = Types.typeDefFrom(classRef);

// check if we're dealing with an enum
if(def.isEnum()) {
Expand Down
2 changes: 1 addition & 1 deletion crd-generator/api/src/main/java/io/fabric8/crd/generator/CRDGenerator.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ public int generate() {
"# Generated by Fabric8 CRDGenerator, manual edits might get overwritten!\n"
.getBytes());
YAML_MAPPER.writeValue(outputStream, crd);
builder.append("\t- ").append(version).append(" CRD -> ").append(output.crdURI(outputName));
builder.append("\t- ").append(version).append(" CRD -> ").append(output.crdURI(outputName)).append("\n");
}
} catch (IOException e) {
throw new RuntimeException(e);
Expand Down
3 changes: 1 addition & 2 deletions crd-generator/api/src/main/java/io/fabric8/crd/generator/CustomResourceInfo.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@
import io.fabric8.kubernetes.client.CustomResource;
import io.fabric8.kubernetes.client.KubernetesClientException;
import io.fabric8.kubernetes.model.Scope;
import io.sundr.codegen.functions.ClassTo;
import io.sundr.codegen.model.TypeDef;
import java.lang.reflect.InvocationTargetException;
import java.util.Objects;
Expand Down Expand Up @@ -135,7 +134,7 @@ public static CustomResourceInfo fromClass(Class<? extends CustomResource> custo

final String[] shortNames = CustomResource.getShortNames(customResource);

final TypeDef definition = ClassTo.TYPEDEF.apply(customResource);
final TypeDef definition = Types.typeDefFrom(customResource);
if (DESCRIBE_TYPE_DEFS) {
Types.output(definition);
}
Expand Down
143 changes: 91 additions & 52 deletions crd-generator/api/src/main/java/io/fabric8/crd/generator/utils/Types.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import io.fabric8.kubernetes.api.model.Namespaced;
import io.fabric8.kubernetes.client.CustomResource;
import io.sundr.builder.TypedVisitor;
import io.sundr.codegen.functions.ClassTo;
import io.sundr.codegen.model.ClassRef;
import io.sundr.codegen.model.ClassRefBuilder;
import io.sundr.codegen.model.PrimitiveRef;
Expand Down Expand Up @@ -51,7 +52,28 @@ public class Types {
public static final String JAVA_LANG_VOID = "java.lang.Void";


public static TypeDef projectDefinition(ClassRef ref) {
public static TypeDef typeDefFrom(Class<?> clazz) {
return unshallow(ClassTo.TYPEDEF.apply(clazz));
}

public static TypeDef unshallow(TypeDef definition) {
// resolve hierarchy
final List<ClassRef> classRefs = new ArrayList<>(Types.projectSuperClasses(definition));
// resolve properties
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());
}

public static TypeDef typeDefFrom(ClassRef classRef) {
return unshallow(classRef.getDefinition());
}

private static TypeDef projectDefinition(ClassRef ref) {
List<TypeRef> arguments = ref.getArguments();
TypeDef definition = ref.getDefinition();
if (arguments.isEmpty()) {
Expand All @@ -78,21 +100,21 @@ public static TypeDef projectDefinition(ClassRef ref) {
* @param mappings A map that maps class arguments names to types.
* @return a visitors that performs the actual mapping.
*/
public 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);
}
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);
}
}
};
}
};
}

/**
Expand All @@ -102,28 +124,28 @@ public void visit(PropertyBuilder property) {
* @param mappings A map that maps class arguments names to types.
* @return a visitors that performs the actual mapping.
*/
public 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;
}
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);
arguments.add(mappedRef);
}
};
c.withArguments(arguments);
}
};
}
public static Set<ClassRef> projectSuperClasses(TypeDef definition) {

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()))
Expand All @@ -135,21 +157,36 @@ public static Set<ClassRef> projectSuperClasses(TypeDef definition) {
* @param typeDef The type.
* @return A list with all properties.
*/
public static List<Property> projectProperties(TypeDef typeDef) {
return Stream.concat(typeDef.getProperties().stream(),
typeDef.getExtendsList()
.stream()
.filter(e -> !e.getFullyQualifiedName().equals("java.lang.Object"))
.flatMap(e -> projectProperties(projectDefinition(e))
.stream()
.filter(p -> filterCustomResourceProperties(e).test(p))))
.filter(p -> !p.isStatic())
.collect(Collectors.toList());
private static List<Property> projectProperties(TypeDef typeDef) {
final String fqn = typeDef.getFullyQualifiedName();
return Stream.concat(
typeDef.getProperties().stream().filter(p -> {
// enums have self-referential static properties for each enum case so we cannot ignore them
if(typeDef.isEnum()) {
final TypeRef typeRef = p.getTypeRef();
if (typeRef instanceof ClassRef && fqn.equals(((ClassRef) typeRef).getFullyQualifiedName())) {
// we're dealing with an enum case so keep it
return true;
}
}
// otherwise exclude all static properties
return !p.isStatic();
}),
typeDef.getExtendsList().stream()
.filter(e -> !e.getFullyQualifiedName().startsWith("java."))
.flatMap(e -> projectProperties(projectDefinition(e))
.stream()
.filter(p -> filterCustomResourceProperties(e).test(p)))
)

.collect(Collectors.toList());
}


public static Predicate<Property> filterCustomResourceProperties(ClassRef ref) {
return p -> !ref.getFullyQualifiedName().equals(CUSTOM_RESOURCE_NAME) || (p.getName().equals("spec") || p.getName().equals("status"));
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")));
}


Expand All @@ -165,10 +202,10 @@ public static void describeType(TypeDef def, String indent, Set<String> visited,
}

visited.add(def.getFullyQualifiedName());
for (Property property : projectProperties(def)) {
for (Property property : def.getProperties()) {
TypeRef typeRef = property.getTypeRef();
if (typeRef instanceof ClassRef) {
final TypeDef typeDef = ((ClassRef) typeRef).getDefinition();
final TypeDef typeDef = typeDefFrom((ClassRef) typeRef);
builder.append(indent).append("\t").append(property).append(" - ClassRef [").append(typeDef.getKind()).append("]\n");
if (!visited.contains(typeDef.getFullyQualifiedName())) {
describeType(typeDef, indent + "\t", visited, builder);
Expand All @@ -181,7 +218,7 @@ public static void describeType(TypeDef def, String indent, Set<String> visited,
type = "TypeParamRef";
} else if (typeRef instanceof VoidRef) {
type = "VoidRef";
} else if (typeRef instanceof WildcardRef) {
} else if (typeRef instanceof WildcardRef) {
type = "WildcardRef";
} else {
type = "Unknown";
Expand All @@ -193,6 +230,7 @@ public static void describeType(TypeDef def, String indent, Set<String> visited,
}

public static class SpecAndStatus {

private static final SpecAndStatus UNRESOLVED = new SpecAndStatus(null, null);
private final String specClassName;
private final String statusClassName;
Expand All @@ -216,11 +254,11 @@ public boolean isUnreliable() {
return unreliable;
}
}

private static final String CUSTOM_RESOURCE_NAME = CustomResource.class.getCanonicalName();
public static SpecAndStatus resolveSpecAndStatusTypes(TypeDef definition) {
Set<ClassRef> superClasses = Types.projectSuperClasses(definition);

Optional<ClassRef> optionalCustomResourceRef = superClasses.stream()
public static SpecAndStatus resolveSpecAndStatusTypes(TypeDef definition) {
Optional<ClassRef> optionalCustomResourceRef = definition.getExtendsList().stream()
.filter(s -> s.getFullyQualifiedName().equals(CUSTOM_RESOURCE_NAME)).findFirst();
if (optionalCustomResourceRef.isPresent()) {
ClassRef customResourceRef = optionalCustomResourceRef.get();
Expand Down Expand Up @@ -271,7 +309,7 @@ public static boolean isNamespaced(TypeDef definition, Set<TypeDef> visited) {
* @return the an optional property.
*/
public static Optional<Property> findStatusProperty(TypeDef typeDef) {
return projectProperties(typeDef).stream()
return typeDef.getProperties().stream()
.filter(Types::isStatusProperty)
.findFirst();
}
Expand All @@ -284,7 +322,8 @@ public static Optional<Property> findStatusProperty(TypeDef typeDef) {
* @return {@code true} if named {@code status}, {@code false} otherwise
*/
public static boolean isStatusProperty(Property property) {
return "status".equals(property.getName()) && getClassFQNIfNotVoid(property.getTypeRef()) != null;
return "status".equals(property.getName())
&& getClassFQNIfNotVoid(property.getTypeRef()) != null;
}

private static String getClassFQNIfNotVoid(TypeRef typeRef) {
Expand Down
Loading

0 comments on commit dbe50de

Please sign in to comment.