Skip to content

Commit

Permalink
fix: @SchemaSwap can only be used once
Browse files Browse the repository at this point in the history 
@SchemaSwap is now repeatable, so multiple modifications can be applied from the same resource root.
Fixed a bug when the swap was "used up" after just one usage, meaning that if the referenced type was used multiple times in object hierarchy, only the first one was swapped.

Fixes fabric8io#4350
  • Loading branch information
@xRodney
xRodney committed Sep 6, 2022
1 parent 9d10af9 commit 74d533b
Show file tree
Hide file tree
Showing 13 changed files with 586 additions and 124 deletions.
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Expand Up @@ -3,6 +3,7 @@
### 6.2-SNAPSHOT

#### Bugs
* Fix #4350: SchemaSwap annotation is now repeatable and is applied multiple times if classes are used more than once in the class hierarchy.

#### Improvements
* Fix #4348: Introduce specific annotations for the generators
Expand All @@ -12,6 +13,7 @@
#### New Features

#### _**Note**_: Breaking changes in the API
* Fix #4350: SchemaSwap's fieldName parameter now expects a field name only, not a method or a constructor.

### 6.1.1 (2022-09-01)

Expand All @@ -29,6 +31,7 @@ fix #4373: NO_PROXY should allow URIs with hyphens ("circleci-internal-outer-bui
* Fix #4320: corrected leader transitions field on leader election leases
* Fix #4360: JUnit dependencies aren't leaked in child modules


#### Improvements
* Fix #887: added KubernetesClient.visitResources to search and perform other operations across all resources.
* Fix #3960: adding a KubernetesMockServer.expectCustomResource helper method and additional mock crd support
Expand Down
153 changes: 47 additions & 106 deletions crd-generator/api/src/main/java/io/fabric8/crd/generator/AbstractJsonSchema.java
View file
Expand Up @@ -17,6 +17,7 @@

import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.node.JsonNodeFactory;
import io.fabric8.crd.generator.annotation.SchemaSwap;
import io.fabric8.crd.generator.utils.Types;
import io.fabric8.kubernetes.api.model.Duration;
import io.fabric8.kubernetes.api.model.IntOrString;
Expand All @@ -28,13 +29,13 @@
import org.slf4j.LoggerFactory;

import java.util.*;
import java.util.stream.Collectors;

import static io.sundr.model.utils.Types.BOOLEAN_REF;
import static io.sundr.model.utils.Types.DOUBLE_REF;
import static io.sundr.model.utils.Types.INT_REF;
import static io.sundr.model.utils.Types.LONG_REF;
import static io.sundr.model.utils.Types.STRING_REF;
import static io.sundr.model.utils.Types.VOID;

/**
* Encapsulates the common logic supporting OpenAPI schema generation for CRD generation.
Expand Down Expand Up @@ -87,6 +88,7 @@ public abstract class AbstractJsonSchema<T, B> {
public static final String ANNOTATION_NOT_NULL = "javax.validation.constraints.NotNull";
public static final String ANNOTATION_SCHEMA_FROM = "io.fabric8.crd.generator.annotation.SchemaFrom";
public static final String ANNOTATION_SCHEMA_SWAP = "io.fabric8.crd.generator.annotation.SchemaSwap";
public static final String ANNOTATION_SCHEMA_SWAPS = "io.fabric8.crd.generator.annotation.SchemaSwaps";

public static final String JSON_NODE_TYPE = "com.fasterxml.jackson.databind.JsonNode";

Expand Down Expand Up @@ -170,60 +172,12 @@ public boolean getRequired() {
* @return The schema.
*/
protected T internalFrom(TypeDef definition, String... ignore) {
List<InternalSchemaSwap> schemaSwaps = new ArrayList<>();
InternalSchemaSwaps schemaSwaps = new InternalSchemaSwaps();
T ret = internalFromImpl(definition, new HashSet<>(), schemaSwaps, ignore);
validateRemainingSchemaSwaps("unmatched class", schemaSwaps);
schemaSwaps.throwIfUnmatchedSwaps();
return ret;
}

private static class InternalSchemaSwap {
final ClassRef targetType;
final ClassRef originalType;
final String fieldName;

public InternalSchemaSwap(ClassRef originalType, String fieldName, ClassRef targetType) {
this.originalType = originalType;
this.fieldName = fieldName;
this.targetType = targetType;
}

public ClassRef getTargetType() {
return targetType;
}

public ClassRef getOriginalType() {
return originalType;
}

public String getFieldName() {
return fieldName;
}

@Override
public String toString() {
return "{" +
"targetType=" + targetType +
", originalType=" + originalType +
", fieldName='" + fieldName + '\'' +
'}';
}

@Override
public boolean equals(Object o) {
if (this == o)
return true;
if (o == null || getClass() != o.getClass())
return false;
InternalSchemaSwap that = (InternalSchemaSwap) o;
return targetType.equals(that.targetType) && originalType.equals(that.originalType) && fieldName.equals(that.fieldName);
}

@Override
public int hashCode() {
return Objects.hash(targetType, originalType, fieldName);
}
}

private static ClassRef extractClassRef(Object type) {
if (type != null) {
if (type instanceof ClassRef) {
Expand All @@ -238,25 +192,44 @@ private static ClassRef extractClassRef(Object type) {
}
}

private InternalSchemaSwap extractSchemaSwap(AnnotationRef annotation) {
Map<String, Object> params = annotation.getParameters();
return new InternalSchemaSwap(
extractClassRef(params.get("originalType")),
(String) params.get("fieldName"),
extractClassRef(params.get("targetType")));
private void extractSchemaSwaps(ClassRef definitionType, AnnotationRef annotation, InternalSchemaSwaps schemaSwaps) {
String fullyQualifiedName = annotation.getClassRef().getFullyQualifiedName();
switch (fullyQualifiedName) {
case ANNOTATION_SCHEMA_SWAP:
extractSchemaSwap(definitionType, annotation, schemaSwaps);
break;
case ANNOTATION_SCHEMA_SWAPS:
Map<String, Object> params = annotation.getParameters();
Object[] values = (Object[]) params.get("value");
for (Object value : values) {
extractSchemaSwap(definitionType, value, schemaSwaps);
}
break;
}
}

private void validateRemainingSchemaSwaps(String error, List<InternalSchemaSwap> schemaSwaps) {
if (!schemaSwaps.isEmpty()) {
String umatchedSchemaSwaps = schemaSwaps
.stream()
.map(InternalSchemaSwap::toString)
.collect(Collectors.joining(",", "[", "]"));
throw new IllegalArgumentException("SchemaSwap annotation error " + error + ": " + umatchedSchemaSwaps);
private void extractSchemaSwap(ClassRef definitionType, Object annotation, InternalSchemaSwaps schemaSwaps) {
if (annotation instanceof SchemaSwap) {
SchemaSwap schemaSwap = (SchemaSwap) annotation;
schemaSwaps.registerSwap(definitionType,
extractClassRef(schemaSwap.originalType()),
schemaSwap.fieldName(),
extractClassRef(schemaSwap.targetType()));

} else if (annotation instanceof AnnotationRef
&& ((AnnotationRef) annotation).getClassRef().getFullyQualifiedName().equals(ANNOTATION_SCHEMA_SWAP)) {
Map<String, Object> params = ((AnnotationRef) annotation).getParameters();
schemaSwaps.registerSwap(definitionType,
extractClassRef(params.get("originalType")),
(String) params.get("fieldName"),
extractClassRef(params.getOrDefault("targetType", void.class)));

} else {
throw new IllegalArgumentException("Unmanaged annotation type passed to the SchemaSwaps: " + annotation);
}
}

private T internalFromImpl(TypeDef definition, Set<String> visited, List<InternalSchemaSwap> schemaSwaps, String... ignore) {
private T internalFromImpl(TypeDef definition, Set<String> visited, InternalSchemaSwaps schemaSwaps, String... ignore) {
final B builder = newBuilder();
Set<String> ignores = ignore.length > 0 ? new LinkedHashSet<>(Arrays.asList(ignore))
: Collections
Expand All @@ -266,19 +239,7 @@ private T internalFromImpl(TypeDef definition, Set<String> visited, List<Interna
boolean preserveUnknownFields = (definition.getFullyQualifiedName() != null &&
definition.getFullyQualifiedName().equals(JSON_NODE_TYPE));

List<InternalSchemaSwap> newSchemaSwaps = definition
.getAnnotations()
.stream()
.filter(a -> a.getClassRef().getFullyQualifiedName().equals(ANNOTATION_SCHEMA_SWAP))
.map(this::extractSchemaSwap)
.collect(Collectors.toList());

schemaSwaps.addAll(newSchemaSwaps);

final Set<InternalSchemaSwap> currentSchemaSwaps = schemaSwaps
.stream()
.filter(iss -> iss.getOriginalType().getFullyQualifiedName().equals(definition.getFullyQualifiedName()))
.collect(Collectors.toSet());
definition.getAnnotations().forEach(annotation -> extractSchemaSwaps(definition.toReference(), annotation, schemaSwaps));

// index potential accessors by name for faster lookup
final Map<String, Method> accessors = indexPotentialAccessors(definition);
Expand All @@ -290,11 +251,9 @@ private T internalFromImpl(TypeDef definition, Set<String> visited, List<Interna
continue;
}

final PropertyFacade facade = new PropertyFacade(property, accessors, currentSchemaSwaps);
ClassRef potentialSchemaSwap = schemaSwaps.lookupAndMark(definition.toReference(), name).orElse(null);
final PropertyFacade facade = new PropertyFacade(property, accessors, potentialSchemaSwap);
final Property possiblyRenamedProperty = facade.process();
final Set<InternalSchemaSwap> matchedSchemaSwaps = facade.getMatchedSchemaSwaps();
currentSchemaSwaps.removeAll(matchedSchemaSwaps);
schemaSwaps.removeAll(matchedSchemaSwaps);
name = possiblyRenamedProperty.getName();

if (facade.required) {
Expand Down Expand Up @@ -326,7 +285,6 @@ private T internalFromImpl(TypeDef definition, Set<String> visited, List<Interna
addProperty(possiblyRenamedProperty, builder, possiblyUpdatedSchema, options);
}

validateRemainingSchemaSwaps("unmatched field", currentSchemaSwaps.stream().collect(Collectors.toList()));
return build(builder, required, preserveUnknownFields);
}

Expand Down Expand Up @@ -483,8 +441,6 @@ public String toString() {

private static class PropertyFacade {
private final List<PropertyOrAccessor> propertyOrAccessors = new ArrayList<>(4);
private final Set<InternalSchemaSwap> schemaSwaps;
private final Set<InternalSchemaSwap> matchedSchemaSwaps;
private String renamedTo;
private String description;
private Optional<Double> min;
Expand All @@ -499,10 +455,8 @@ private static class PropertyFacade {
private String descriptionContributedBy;
private TypeRef schemaFrom;

public PropertyFacade(Property property, Map<String, Method> potentialAccessors, Set<InternalSchemaSwap> schemaSwaps) {
public PropertyFacade(Property property, Map<String, Method> potentialAccessors, ClassRef schemaSwap) {
original = property;
this.schemaSwaps = schemaSwaps;
this.matchedSchemaSwaps = new HashSet<>();
final String capitalized = property.getNameCapitalized();
final String name = property.getName();
propertyOrAccessors.add(PropertyOrAccessor.fromProperty(property));
Expand All @@ -518,6 +472,7 @@ public PropertyFacade(Property property, Map<String, Method> potentialAccessors,
if (method != null) {
propertyOrAccessors.add(PropertyOrAccessor.fromMethod(method, name));
}
schemaFrom = schemaSwap;
min = Optional.empty();
max = Optional.empty();
pattern = Optional.empty();
Expand All @@ -526,16 +481,6 @@ public PropertyFacade(Property property, Map<String, Method> potentialAccessors,
public Property process() {
final String name = original.getName();

Optional<InternalSchemaSwap> currentSchemaSwap = schemaSwaps
.stream()
.filter(iss -> iss.getFieldName().equals(name))
.findFirst();

currentSchemaSwap.ifPresent(iss -> {
schemaFrom = iss.targetType;
matchedSchemaSwaps.add(iss);
});

propertyOrAccessors.forEach(p -> {
p.process();
final String contributorName = p.toString();
Expand Down Expand Up @@ -594,10 +539,6 @@ public Property process() {
return new Property(original.getAnnotations(), typeRef, finalName,
original.getComments(), original.getModifiers(), original.getAttributes());
}

public Set<InternalSchemaSwap> getMatchedSchemaSwaps() {
return this.matchedSchemaSwaps;
}
}

private boolean isPotentialAccessor(Method method) {
Expand Down Expand Up @@ -670,10 +611,10 @@ private String extractUpdatedNameFromJacksonPropertyIfPresent(Property property)
* @return the structural schema associated with the specified property
*/
public T internalFrom(String name, TypeRef typeRef) {
return internalFromImpl(name, typeRef, new HashSet<>(), new ArrayList<>());
return internalFromImpl(name, typeRef, new HashSet<>(), new InternalSchemaSwaps());
}

private T internalFromImpl(String name, TypeRef typeRef, Set<String> visited, List<InternalSchemaSwap> schemaSwaps) {
private T internalFromImpl(String name, TypeRef typeRef, Set<String> visited, InternalSchemaSwaps schemaSwaps) {
// Note that ordering of the checks here is meaningful: we need to check for complex types last
// in case some "complex" types are handled specifically
if (typeRef.getDimensions() > 0 || io.sundr.model.utils.Collections.isCollection(typeRef)) { // Handle Collections & Arrays
Expand Down Expand Up @@ -721,7 +662,7 @@ private T internalFromImpl(String name, TypeRef typeRef, Set<String> visited, Li
.map(JsonNodeFactory.instance::textNode)
.toArray(JsonNode[]::new);
return enumProperty(enumValues);
} else {
} else if (!classRef.getFullyQualifiedName().equals(VOID.getName())) {
return resolveNestedClass(name, def, visited, schemaSwaps);
}

Expand All @@ -734,7 +675,7 @@ private T internalFromImpl(String name, TypeRef typeRef, Set<String> visited, Li
// Flag to detect cycles
private boolean resolving = false;

private T resolveNestedClass(String name, TypeDef def, Set<String> visited, List<InternalSchemaSwap> schemaSwaps) {
private T resolveNestedClass(String name, TypeDef def, Set<String> visited, InternalSchemaSwaps schemaSwaps) {
if (!resolving) {
visited.clear();
resolving = true;
Expand Down

0 comments on commit 74d533b

Please sign in to comment.