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

fix(crd-generator): default values for CRD fields can be numeric or boolean #6666

Merged
merged 1 commit into from
Nov 27, 2024
Merged

fix(crd-generator): default values for CRD fields can be numeric or boolean #6666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
* Fix #6214: Java generator does not recognize fields in CRDs other than metadata, spec, and status
* Fix #6459: Pod log request sinceTime param correctly encoded
* Fix #6632: Mock server creationTimestamp and deletionTimestamp formatted consistently (ISO 8601)
* Fix #6654: (crd-generator) default values for CRD fields can be numeric or boolean

#### Improvements
* Fix #3069: (crd-generator) Add `@AdditionalPrinterColumn` to specify a printer column by JSON path.
Expand Down
7 changes: 6 additions & 1 deletion crd-generator/api-v2/pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@
<artifactId>kubernetes-client-api</artifactId>
<scope>compile</scope>
</dependency>

<dependency>
<groupId>com.fasterxml.jackson.module</groupId>
<artifactId>jackson-module-jsonSchema</artifactId>
Expand All @@ -62,6 +62,11 @@
<artifactId>junit-jupiter-engine</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.assertj</groupId>
<artifactId>assertj-core</artifactId>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-simple</artifactId>
Expand Down
28 changes: 18 additions & 10 deletions crd-generator/api-v2/src/main/java/io/fabric8/crdv2/generator/AbstractJsonSchema.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static io.fabric8.crdv2.generator.CRDUtils.toTargetType;
import static java.util.Optional.ofNullable;

/**
Expand All @@ -93,11 +94,11 @@ public abstract class AbstractJsonSchema<T extends KubernetesJSONSchemaProps, V

private static final Logger LOGGER = LoggerFactory.getLogger(AbstractJsonSchema.class);

private ResolvingContext resolvingContext;
private T root;
private Set<String> dependentClasses = new HashSet<>();
private Set<AdditionalPrinterColumn> additionalPrinterColumns = new HashSet<>();
private Set<AdditionalSelectableField> additionalSelectableFields = new HashSet<>();
private final ResolvingContext resolvingContext;
private final T root;
private final Set<String> dependentClasses = new HashSet<>();
private final Set<AdditionalPrinterColumn> additionalPrinterColumns = new HashSet<>();
private final Set<AdditionalSelectableField> additionalSelectableFields = new HashSet<>();

public static class AnnotationMetadata {
public final Annotation annotation;
Expand All @@ -109,7 +110,7 @@ public AnnotationMetadata(Annotation annotation, KubernetesJSONSchemaProps schem
}
}

private Map<Class<? extends Annotation>, LinkedHashMap<String, AnnotationMetadata>> pathMetadata = new HashMap<>();
private final Map<Class<? extends Annotation>, LinkedHashMap<String, AnnotationMetadata>> pathMetadata = new HashMap<>();

public AbstractJsonSchema(ResolvingContext resolvingContext, Class<?> def) {
this.resolvingContext = resolvingContext;
Expand Down Expand Up @@ -226,8 +227,8 @@ void collectValidationRules(BeanProperty beanProperty, List<V> validationRules)
class PropertyMetadata {

private boolean required;
private String description;
private String defaultValue;
private final String description;
private final Object defaultValue;
private Double min;
private Double max;
private String pattern;
Expand All @@ -241,7 +242,6 @@ public PropertyMetadata(JsonSchema value, BeanProperty beanProperty) {
required = Boolean.TRUE.equals(value.getRequired());

description = beanProperty.getMetadata().getDescription();
defaultValue = beanProperty.getMetadata().getDefaultValue();

schemaFrom = ofNullable(beanProperty.getAnnotation(SchemaFrom.class)).map(SchemaFrom::type).orElse(null);
preserveUnknownFields = beanProperty.getAnnotation(PreserveUnknownFields.class) != null;
Expand All @@ -263,6 +263,15 @@ public PropertyMetadata(JsonSchema value, BeanProperty beanProperty) {

collectValidationRules(beanProperty, validationRules);

if (beanProperty.getMetadata().getDefaultValue() != null) {
defaultValue = toTargetType(beanProperty.getType(), beanProperty.getMetadata().getDefaultValue());
} else if (ofNullable(beanProperty.getAnnotation(Default.class)).map(Default::value).isPresent()) {
defaultValue = toTargetType(beanProperty.getType(),
ofNullable(beanProperty.getAnnotation(Default.class)).map(Default::value).get());
} else {
defaultValue = null;
}

// TODO: should probably move to a standard annotations
// see ValidationSchemaFactoryWrapper
nullable = beanProperty.getAnnotation(Nullable.class) != null;
Expand All @@ -271,7 +280,6 @@ public PropertyMetadata(JsonSchema value, BeanProperty beanProperty) {

// TODO: should the following be deprecated?
required = beanProperty.getAnnotation(Required.class) != null;
defaultValue = ofNullable(beanProperty.getAnnotation(Default.class)).map(Default::value).orElse(defaultValue);
pattern = ofNullable(beanProperty.getAnnotation(Pattern.class)).map(Pattern::value).orElse(pattern);
}

Expand Down
24 changes: 23 additions & 1 deletion crd-generator/api-v2/src/main/java/io/fabric8/crdv2/generator/CRDUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,10 +16,12 @@
package io.fabric8.crdv2.generator;

import com.fasterxml.jackson.databind.BeanDescription;
import com.fasterxml.jackson.databind.JavaType;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.SerializationConfig;
import com.fasterxml.jackson.databind.introspect.BeanPropertyDefinition;

import java.text.NumberFormat;
import java.util.HashMap;
import java.util.Map;

Expand All @@ -28,6 +30,7 @@ private CRDUtils() {
throw new IllegalStateException("Utility class");
}

@SuppressWarnings("LombokGetterMayBeUsed")
public static class SpecAndStatus {

private final String specClassName;
Expand Down Expand Up @@ -79,7 +82,7 @@ public static Map<String, String> toMap(String[] arr) {
Map<String, String> res = new HashMap<>();
if (arr != null) {
for (String e : arr) {
String[] splitted = e.split("\\=");
String[] splitted = e.split("=");
if (splitted.length >= 2) {
res.put(splitted[0], e.substring(splitted[0].length() + 1));
} else {
Expand All @@ -91,4 +94,23 @@ public static Map<String, String> toMap(String[] arr) {
return res;
}

static Object toTargetType(JavaType type, String value) {
if (type == null || value == null) {
return null;
}
try {
if (Number.class.isAssignableFrom(type.getRawClass()) || int.class.isAssignableFrom(type.getRawClass())
|| long.class.isAssignableFrom(type.getRawClass()) || float.class.isAssignableFrom(type.getRawClass())
|| double.class.isAssignableFrom(type.getRawClass())) {
return NumberFormat.getInstance().parse(value);
}
if (Boolean.class.isAssignableFrom(type.getRawClass()) || boolean.class.isAssignableFrom(type.getRawClass())) {
return Boolean.valueOf(value);
}
} catch (Exception ex) {
// NO OP
}
return value;

}
}
175 changes: 175 additions & 0 deletions ...erator/api-v2/src/test/java/io/fabric8/crdv2/generator/v1/JsonSchemaDefaultValueTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,175 @@
/*
* Copyright (C) 2015 Red Hat, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.fabric8.crdv2.generator.v1;

import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.node.BooleanNode;
import com.fasterxml.jackson.databind.node.DoubleNode;
import com.fasterxml.jackson.databind.node.LongNode;
import io.fabric8.generator.annotation.Default;
import org.assertj.core.api.InstanceOfAssertFactories;
import org.junit.jupiter.api.DisplayName;
import org.junit.jupiter.api.Test;

import static org.assertj.core.api.Assertions.assertThat;

class JsonSchemaDefaultValueTest {

private static final class ClassInTest {
@JsonProperty(defaultValue = "precedence-from-json-property")
@Default("precedence-from-default-annotation")
String precedence;

@JsonProperty
@Default("string-from-default-annotation")
String defaultAnnotationForString;

@JsonProperty(defaultValue = "true")
Boolean defaultValueForBoxedBoolean;

@JsonProperty
@Default("true")
Boolean defaultAnnotationForBoxedBoolean;

@JsonProperty(defaultValue = "true")
boolean defaultValueForBoolean;

@JsonProperty
@Default("true")
boolean defaultAnnotationForBoolean;

@JsonProperty(defaultValue = "1337")
int defaultValueForInt;

@JsonProperty(defaultValue = "1337L")
long defaultValueForLong;

@JsonProperty(defaultValue = "13.37")
float defaultValueForFloat;

@JsonProperty(defaultValue = "13.37d")
double defaultValueForDouble;
}

@Test
@DisplayName("JsonProperty default value should take precedence over Default annotation")
void precedence() {
assertThat(JsonSchema.from(ClassInTest.class).getProperties())
.extracting("precedence._default")
.asInstanceOf(InstanceOfAssertFactories.type(JsonNode.class))
.extracting(JsonNode::asText)
.isEqualTo("precedence-from-json-property");
}

@Test
@DisplayName("Default annotation for String")
void stringFromDefaultAnnotation() {
assertThat(JsonSchema.from(ClassInTest.class).getProperties())
.extracting("defaultAnnotationForString._default")
.asInstanceOf(InstanceOfAssertFactories.type(JsonNode.class))
.extracting(JsonNode::asText)
.isEqualTo("string-from-default-annotation");
}

@Test
@DisplayName("JsonProperty defaultValue annotation for boxed Boolean")
void booleanBoxedFromJsonPropertyAnnotation() {
assertThat(JsonSchema.from(ClassInTest.class).getProperties())
.extracting("defaultValueForBoxedBoolean._default")
.asInstanceOf(InstanceOfAssertFactories.type(JsonNode.class))
.isInstanceOf(BooleanNode.class)
.extracting(JsonNode::asBoolean)
.isEqualTo(true);
}

@Test
@DisplayName("Default annotation for boxed Boolean")
void booleanBoxedFromDefaultAnnotation() {
assertThat(JsonSchema.from(ClassInTest.class).getProperties())
.extracting("defaultAnnotationForBoxedBoolean._default")
.asInstanceOf(InstanceOfAssertFactories.type(JsonNode.class))
.isInstanceOf(BooleanNode.class)
.extracting(JsonNode::asBoolean)
.isEqualTo(true);
}

@Test
@DisplayName("JsonProperty defaultValue annotation for boolean")
void booleanFromJsonPropertyAnnotation() {
assertThat(JsonSchema.from(ClassInTest.class).getProperties())
.extracting("defaultValueForBoolean._default")
.asInstanceOf(InstanceOfAssertFactories.type(JsonNode.class))
.isInstanceOf(BooleanNode.class)
.extracting(JsonNode::asBoolean)
.isEqualTo(true);
}

@Test
@DisplayName("Default annotation for boolean")
void booleanFromDefaultAnnotation() {
assertThat(JsonSchema.from(ClassInTest.class).getProperties())
.extracting("defaultAnnotationForBoolean._default")
.asInstanceOf(InstanceOfAssertFactories.type(JsonNode.class))
.isInstanceOf(BooleanNode.class)
.extracting(JsonNode::asBoolean)
.isEqualTo(true);
}

@Test
@DisplayName("JsonProperty defaultValue annotation for int")
void intFromJsonPropertyAnnotation() {
assertThat(JsonSchema.from(ClassInTest.class).getProperties())
.extracting("defaultValueForInt._default")
.asInstanceOf(InstanceOfAssertFactories.type(JsonNode.class))
.isInstanceOf(LongNode.class)
Copy link
Member

Choose a reason for hiding this comment

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

A little surprised this is a LongNode

Copy link
Member Author

Choose a reason for hiding this comment

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

This has to do with the Number parser which considers this to be of long precision.
It really doesn't matter, since what we want is the number to be considered a number and not a String.
Of course, there must be corner cases such as users declaring a field of type in and setting a default value of 999999999999999999999. My expectation is that the cluster won't accept that CRD.
We could try and consider those corner cases, but if we introduce validations for the default values too, the complexity will be kind of crazy for such a simple feature.

Fixing this is just a matter of using the specific parser for the target type. i.e. adding more if-else clauses.

Copy link
Member

Choose a reason for hiding this comment

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

Good enough for me, if we get any complaints over this we should remember to add checks for the boundaries (i.e. Integer.MAX_VALUE) etc.

.extracting(JsonNode::asInt)
.isEqualTo(1337);
}

@Test
@DisplayName("JsonProperty defaultValue annotation for long")
void longFromJsonPropertyAnnotation() {
assertThat(JsonSchema.from(ClassInTest.class).getProperties())
.extracting("defaultValueForLong._default")
.asInstanceOf(InstanceOfAssertFactories.type(JsonNode.class))
.isInstanceOf(LongNode.class)
.extracting(JsonNode::asLong)
.isEqualTo(1337L);
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to also generate the resulting YAML file?

I'm interested in checking the L and d suffixes

Copy link
Member Author

Choose a reason for hiding this comment

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

The suffixes here I added them to highlight that these values are equal to the expected type, to make it clear that a long is a long and a double is not a double.
How it renders in the CRD is not about how the value is stored in a JsonNode, but how the serializer deals with those. Basically they'll be rendered as any other number in a JSON->YAML document.

There's also an approval test modified that you can check:

kubernetes-client/crd-generator/test/src/test/resources/io/fabric8/crd/generator/approvaltests/CRDGeneratorApprovalTest.approvalTest.annotateds.samples.fabric8.io.v1.approved.yml

Lines 25 to 36 in bee2f01

defaultBoolean:
default: true
type: "boolean"
defaultInt:
default: 1337
type: "integer"
defaultInteger:
default: 1337
type: "integer"
defaultValue:
default: "my-value"
type: "string"

I noticed that I didn't add a specific test for doubles, but the expected value should be an unquoted number with a . in case it has decimals.

Copy link
Member Author

Choose a reason for hiding this comment

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

#6670

}

@Test
@DisplayName("JsonProperty defaultValue annotation for float")
void floatFromJsonPropertyAnnotation() {
assertThat(JsonSchema.from(ClassInTest.class).getProperties())
.extracting("defaultValueForFloat._default")
.asInstanceOf(InstanceOfAssertFactories.type(JsonNode.class))
.isInstanceOf(DoubleNode.class)
.extracting(JsonNode::asDouble)
.isEqualTo(13.37);
}

@Test
@DisplayName("JsonProperty defaultValue annotation for double")
void doubleFromJsonPropertyAnnotation() {
assertThat(JsonSchema.from(ClassInTest.class).getProperties())
.extracting("defaultValueForDouble._default")
.asInstanceOf(InstanceOfAssertFactories.type(JsonNode.class))
.isInstanceOf(DoubleNode.class)
.extracting(JsonNode::asDouble)
.isEqualTo(13.37);
}
}
9 changes: 9 additions & 0 deletions ...or/test/src/test/java/io/fabric8/crd/generator/approvaltests/annotated/AnnotatedSpec.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,15 @@ public class AnnotatedSpec {
private String defaultValue;
@Default("my-value2")
private String defaultValue2;
@JsonProperty(defaultValue = "true")
Copy link
Member

Choose a reason for hiding this comment

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

What happens when the two annotations contain different values?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's a precedence test for this, you can check it out ;)
JsonProperty takes precedence since AFAIR the other annotation will eventually be removed or deprecated.

Copy link
Member

Choose a reason for hiding this comment

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

AFAIR the other annotation will eventually be removed or deprecated

oh, really? I'm surprised!

Should this behavior be documented? somewhere

Copy link
Member Author

Choose a reason for hiding this comment

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

I have really no idea, I deduced this from this TODO comment:

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

Line 281 in bee2f01

// TODO: should the following be deprecated?

I thought this had been discussed, but maybe not.

TBH this is the first time I'm actually looking into the CRD generator, up until now I had been delegating any task related to the CRD generator to you folks. So I might be disconnected.

In any case, a deprecation would happen in 7.x and not in 7.0.0

@Default("true") // compatibility with CRDGenerator-v1
private boolean defaultBoolean;
@JsonProperty(defaultValue = "1337")
@Default("1337") // compatibility with CRDGenerator-v1
private Integer defaultInteger;
@JsonProperty(defaultValue = "1337")
@Default("1337") // compatibility with CRDGenerator-v1
private int defaultInt;
@Required
private boolean emptySetter;
@Required
Expand Down
9 changes: 9 additions & 0 deletions ...tests/CRDGeneratorApprovalTest.approvalTest.annotateds.samples.fabric8.io.v1.approved.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,15 @@ spec:
- "non"
- "oui"
type: "string"
defaultBoolean:
default: true
type: "boolean"
defaultInt:
default: 1337
type: "integer"
defaultInteger:
default: 1337
type: "integer"
defaultValue:
default: "my-value"
type: "string"
Expand Down
9 changes: 9 additions & 0 deletions .../CRDGeneratorApprovalTest.approvalTest.annotateds.samples.fabric8.io.v1beta1.approved.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,15 @@ spec:
- "non"
- "oui"
type: "string"
defaultBoolean:
default: true
type: "boolean"
defaultInt:
default: 1337
type: "integer"
defaultInteger:
default: 1337
type: "integer"
defaultValue:
default: "my-value"
type: "string"
Expand Down
Loading