Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: @SchemaSwap can only be used once #4354

Merged
merged 2 commits into from
Sep 8, 2022
Merged

fix: @SchemaSwap can only be used once #4354

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
74d533b
fix: @SchemaSwap can only be used once
xRodney Aug 25, 2022
2b44a7f
Merge branch 'master' into bugfix/schema-swap
andreaTP Sep 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 3 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
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
Open in desktop
Original file line number Diff line number Diff line change
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I correctly follow this change, here you cannot mark just yet the SchemaSwap as used, as it will mark the annotation to be "used" even in this condition:

  • Matched Class
  • The Class doesn't have the specified Property (e.g. a matching field)

This operation should be performed only after process has been called on the PropertyFacade (and the SchemaSwap has been matched).

Copy link
Contributor Author

@xRodney xRodney Aug 26, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

schemaSwapt.lookupAndMark accepts two arguments, the second being the property name. (Line 253: String name = property.getName();)
So I'm not sure I understand your concern?
Btw, I thought the case of "matched class, unmatched field" is being tested in JsonSchemaTest.shouldThrowIfSchemaSwapHasUnmatchedField. This test is still passing and I only extended it with assert on the error message. But the error is still thrown.

Copy link
Contributor Author

@xRodney xRodney Aug 26, 2022 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or is your concern that the name could be overriden by @JsonProperty & friends? IMHO it makes more sense for the fieldName parameter to reference the fields as is defined in Java, but that's just my opinion.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it makes more sense for the fieldName parameter to reference the fields as is defined in Java, but that's just my opinion.

This makes sense, but, using name might not be the name extracted by the PropertyFacade constructor:

kubernetes-client/crd-generator/api/src/main/java/io/fabric8/crd/generator/AbstractJsonSchema.java

Lines 414 to 426 in 808084e

propertyOrAccessors.add(PropertyOrAccessor.fromProperty(property));
Method method = potentialAccessors.get("is" + capitalized);
if (method != null) {
propertyOrAccessors.add(PropertyOrAccessor.fromMethod(method, name));
}
method = potentialAccessors.get("get" + capitalized);
if (method != null) {
propertyOrAccessors.add(PropertyOrAccessor.fromMethod(method, name));
}
method = potentialAccessors.get("set" + capitalized);
if (method != null) {
propertyOrAccessors.add(PropertyOrAccessor.fromMethod(method, name));
}

in some cases, the PropertyFacade is extracting the field name from constructor/getter/setter
and this implementation will not respect the extracted name as opposed to the underlying field name.
We can either refactor the logic and use the PropertyFacade name extraction logic everywhere or simply document this different behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, and the only way how that can happen is a @JsonProperty on the field itself or getter (get* or is*) or setter.
I can only say that to me, as the user of the library, would never occur that fieldName could reference the name after transformations.
Maybe it's because the word "field" is used, whose meaning in Java is well defined and is different from the term "property". (Field is a class variable, property is a getter/setter pair - which often is backed by a field). Also the annotation is @JsonProperty, not "JsonField".
Or maybe it's because the reason I am dealing with @SchemaSwaps in the first place is that I cannot place annotations (like @JsonProperty) on the field directly.
So with your permission, I will add Javadocs on @SchemaSwap, describing the currently implemented behaviour.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To me, this "limitation" is totally fine, please just make it explicit in this doc:
https://github.com/fabric8io/kubernetes-client/blob/master/doc/CRD-generator.md#iofabric8crdgeneratorannotationschemaswap

mentioning something along the lines:

the name of the field is restricted to the original fieldName and should be backed by a matching concrete field of the matching class. Getters, setters, and constructors are not taken into consideration.

cc. @metacosm to check if he is on board with this decision.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that should be fine but this should be documented as a breaking change, though…

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it "might" be breaking, but, at the same time, this PR is solving some fundamental issues with the implementation and I doubt anyone else is using it just yet.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can add a line to the Changelog for reference, wdyt?

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