Skip to content

Commit

Permalink
fix #3972: moving template handling to a custom deserializer
Browse files Browse the repository at this point in the history
  • Loading branch information
@shawkins @manusa
shawkins authored and manusa committed Aug 23, 2022
1 parent 0a28e95 commit e19e8cd
Show file tree
Hide file tree
Showing 14 changed files with 298 additions and 134 deletions.
15 changes: 3 additions & 12 deletions kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/utils/Serialization.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@
import io.fabric8.kubernetes.api.model.GenericKubernetesResource;
import io.fabric8.kubernetes.api.model.KubernetesResource;
import io.fabric8.kubernetes.client.KubernetesClientException;
import io.fabric8.kubernetes.client.utils.serialization.UnmatchedFieldTypeModule;
import io.fabric8.kubernetes.model.jackson.UnmatchedFieldTypeModule;
import org.yaml.snakeyaml.DumperOptions;
import org.yaml.snakeyaml.Yaml;
import org.yaml.snakeyaml.constructor.SafeConstructor;
Expand Down Expand Up @@ -419,17 +419,8 @@ public static <T> T clone(T resource) {
// if full serialization seems too expensive, there is also
//return (T) JSON_MAPPER.convertValue(resource, resource.getClass());
try {
return JSON_MAPPER.readValue(
JSON_MAPPER.writeValueAsString(resource), new TypeReference<T>() {
@Override
public Type getType() {
if (resource instanceof KubernetesResource) {
// Force KubernetesResource so that the KubernetesDeserializer takes over any resource configured deserializer
return resource instanceof GenericKubernetesResource ? resource.getClass() : KubernetesResource.class;
}
return resource.getClass();
}
});
return (T) JSON_MAPPER.readValue(
JSON_MAPPER.writeValueAsString(resource), resource.getClass());
} catch (JsonProcessingException e) {
throw new IllegalStateException(e);
}
Expand Down
36 changes: 20 additions & 16 deletions ...ion/SettableBeanPropertyDelegateTest.java → ...son/SettableBeanPropertyDelegateTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.fabric8.kubernetes.client.utils.serialization;
package io.fabric8.kubernetes.model.jackson;

import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.databind.deser.SettableAnyProperty;
Expand All @@ -24,6 +24,7 @@
import org.junit.jupiter.api.Test;

import java.io.IOException;
import java.util.concurrent.atomic.AtomicBoolean;

import static org.assertj.core.api.Assertions.assertThat;
import static org.mockito.ArgumentMatchers.any;
Expand All @@ -41,12 +42,14 @@ class SettableBeanPropertyDelegateTest {
private SettableBeanProperty delegateMock;
private SettableAnyProperty anySetterMock;
private SettableBeanPropertyDelegate settableBeanPropertyDelegate;
private AtomicBoolean useAnySetter = new AtomicBoolean();

@BeforeEach
void setUp() {
delegateMock = mock(SettableBeanProperty.class, RETURNS_DEEP_STUBS);
anySetterMock = mock(SettableAnyProperty.class);
settableBeanPropertyDelegate = new SettableBeanPropertyDelegate(delegateMock, anySetterMock, () -> false);
useAnySetter.set(false);
settableBeanPropertyDelegate = new SettableBeanPropertyDelegate(delegateMock, anySetterMock, () -> useAnySetter.get());
}

@Test
Expand All @@ -58,10 +61,10 @@ void withValueDeserializer() {
final SettableBeanProperty result = settableBeanPropertyDelegate.withValueDeserializer(null);
// Then
assertThat(result)
.isInstanceOf(SettableBeanPropertyDelegate.class)
.isNotSameAs(settableBeanPropertyDelegate)
.hasFieldOrPropertyWithValue("anySetter", anySetterMock)
.hasFieldOrPropertyWithValue("delegate", delegateMock);
.isInstanceOf(SettableBeanPropertyDelegate.class)
.isNotSameAs(settableBeanPropertyDelegate)
.hasFieldOrPropertyWithValue("anySetter", anySetterMock)
.hasFieldOrPropertyWithValue("delegate", delegateMock);
}

@Test
Expand All @@ -73,10 +76,10 @@ void withName() {
final SettableBeanProperty result = settableBeanPropertyDelegate.withName(null);
// Then
assertThat(result)
.isInstanceOf(SettableBeanPropertyDelegate.class)
.isNotSameAs(settableBeanPropertyDelegate)
.hasFieldOrPropertyWithValue("anySetter", anySetterMock)
.hasFieldOrPropertyWithValue("delegate", delegateMock);
.isInstanceOf(SettableBeanPropertyDelegate.class)
.isNotSameAs(settableBeanPropertyDelegate)
.hasFieldOrPropertyWithValue("anySetter", anySetterMock)
.hasFieldOrPropertyWithValue("delegate", delegateMock);
}

@Test
Expand All @@ -88,10 +91,10 @@ void withNullProvider() {
final SettableBeanProperty result = settableBeanPropertyDelegate.withNullProvider(null);
// Then
assertThat(result)
.isInstanceOf(SettableBeanPropertyDelegate.class)
.isNotSameAs(settableBeanPropertyDelegate)
.hasFieldOrPropertyWithValue("anySetter", anySetterMock)
.hasFieldOrPropertyWithValue("delegate", delegateMock);
.isInstanceOf(SettableBeanPropertyDelegate.class)
.isNotSameAs(settableBeanPropertyDelegate)
.hasFieldOrPropertyWithValue("anySetter", anySetterMock)
.hasFieldOrPropertyWithValue("delegate", delegateMock);
}

@Test
Expand Down Expand Up @@ -186,9 +189,10 @@ void deserializeSetAndReturnWithException() throws IOException {
final Object instance = new Object();
when(delegateMock.getName()).thenReturn("the-property");
when(delegateMock.deserializeSetAndReturn(any(), any(), eq(instance)))
.thenThrow(MismatchedInputException.from(null, Integer.class, "The Mocked Exception"));
.thenThrow(MismatchedInputException.from(null, Integer.class, "The Mocked Exception"));
doThrow(MismatchedInputException.from(null, Integer.class, "The Mocked Exception"))
.when(delegateMock).deserializeAndSet(any(), any(), eq(instance));
.when(delegateMock).deserializeAndSet(any(), any(), eq(instance));
useAnySetter.set(true);
// When
final Object result = settableBeanPropertyDelegate.deserializeSetAndReturn(mock(JsonParser.class), null, instance);
// Then
Expand Down
2 changes: 1 addition & 1 deletion ...ization/UnmatchedFieldTypeModuleTest.java → ...jackson/UnmatchedFieldTypeModuleTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.fabric8.kubernetes.client.utils.serialization;
package io.fabric8.kubernetes.model.jackson;

import com.fasterxml.jackson.core.JsonProcessingException;
import com.fasterxml.jackson.databind.ObjectMapper;
Expand Down
4 changes: 4 additions & 0 deletions kubernetes-model-generator/kubernetes-model-common/pom.xml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,10 @@
<groupId>com.fasterxml.jackson.core</groupId>
<artifactId>jackson-databind</artifactId>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-api</artifactId>
</dependency>
</dependencies>

<build>
Expand Down
10 changes: 6 additions & 4 deletions ...alization/BeanPropertyWriterDelegate.java → ...l/jackson/BeanPropertyWriterDelegate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.fabric8.kubernetes.client.utils.serialization;
package io.fabric8.kubernetes.model.jackson;

import com.fasterxml.jackson.core.JsonGenerator;
import com.fasterxml.jackson.databind.SerializerProvider;
Expand All @@ -29,10 +29,12 @@
* Variant of {@link BeanPropertyWriter} which prevents property values present in the {@link AnnotatedMember} anyGetter
* to be serialized twice.
*
* <p> Any property that's present in the anyGetter is ignored upon serialization. The values present in the anyGetter
* <p>
* Any property that's present in the anyGetter is ignored upon serialization. The values present in the anyGetter
* take precedence over those stored in the Bean's fields.
*
* <p> This BeanPropertyWriter implementation is intended to be used in combination with
* <p>
* This BeanPropertyWriter implementation is intended to be used in combination with
* the {@link SettableBeanPropertyDelegate} to allow the propagation of deserialized properties that don't match the
* target field types.
*/
Expand Down Expand Up @@ -64,7 +66,7 @@ public void serializeAsField(Object bean, JsonGenerator gen, SerializerProvider
delegate.serializeAsField(bean, gen, prov);
} else if (Boolean.TRUE.equals(logDuplicateWarning.get())) {
logger.warn("Value in field '{}' ignored in favor of value in additionalProperties ({}) for {}",
delegate.getName(), valueInAnyGetter, bean.getClass().getName());
delegate.getName(), valueInAnyGetter, bean.getClass().getName());
}
}
}
34 changes: 17 additions & 17 deletions ...ization/SettableBeanPropertyDelegate.java → ...jackson/SettableBeanPropertyDelegate.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.fabric8.kubernetes.client.utils.serialization;
package io.fabric8.kubernetes.model.jackson;

import com.fasterxml.jackson.core.JsonParser;
import com.fasterxml.jackson.databind.DeserializationConfig;
Expand All @@ -25,53 +25,53 @@
import com.fasterxml.jackson.databind.deser.SettableBeanProperty;
import com.fasterxml.jackson.databind.exc.MismatchedInputException;
import com.fasterxml.jackson.databind.introspect.AnnotatedMember;
import io.fabric8.kubernetes.internal.KubernetesDeserializer;

import java.io.IOException;
import java.lang.annotation.Annotation;
import java.util.function.Supplier;
import java.util.function.BooleanSupplier;

/**
* This concrete sub-class encapsulates a {@link SettableBeanProperty} delegate that is always tried first.
*
* <p> A fall-back mechanism is implemented in the deserializeAndSet methods to allow field values that don't match the
* <p>
* A fall-back mechanism is implemented in the deserializeAndSet methods to allow field values that don't match the
* target type to be preserved in the anySetter method if exists.
*/
public class SettableBeanPropertyDelegate extends SettableBeanProperty {

private final SettableBeanProperty delegate;
private final SettableAnyProperty anySetter;
private final transient Supplier<Boolean> restrictToTemplates;
private final transient BooleanSupplier useAnySetter;

SettableBeanPropertyDelegate(SettableBeanProperty delegate, SettableAnyProperty anySetter, Supplier<Boolean> restrictToTemplates) {
SettableBeanPropertyDelegate(SettableBeanProperty delegate, SettableAnyProperty anySetter, BooleanSupplier useAnySetter) {
super(delegate);
this.delegate = delegate;
this.anySetter = anySetter;
this.restrictToTemplates = restrictToTemplates;
this.useAnySetter = useAnySetter;
}

/**
* {@inheritDoc}
*/
@Override
public SettableBeanProperty withValueDeserializer(JsonDeserializer<?> deser) {
return new SettableBeanPropertyDelegate(delegate.withValueDeserializer(deser), anySetter, restrictToTemplates);
return new SettableBeanPropertyDelegate(delegate.withValueDeserializer(deser), anySetter, useAnySetter);
}

/**
* {@inheritDoc}
*/
@Override
public SettableBeanProperty withName(PropertyName newName) {
return new SettableBeanPropertyDelegate(delegate.withName(newName), anySetter, restrictToTemplates);
return new SettableBeanPropertyDelegate(delegate.withName(newName), anySetter, useAnySetter);
}

/**
* {@inheritDoc}
*/
@Override
public SettableBeanProperty withNullProvider(NullValueProvider nva) {
return new SettableBeanPropertyDelegate(delegate.withNullProvider(nva), anySetter, restrictToTemplates);
return new SettableBeanPropertyDelegate(delegate.withNullProvider(nva), anySetter, useAnySetter);
}

/**
Expand Down Expand Up @@ -117,13 +117,16 @@ public boolean isIgnorable() {
/**
* Method called to deserialize appropriate value, given parser (and context), and set it using appropriate mechanism.
*
* <p> Deserialization is first tried through the delegate. In case a {@link MismatchedInputException} is caught,
* <p>
* Deserialization is first tried through the delegate. In case a {@link MismatchedInputException} is caught,
* the field is stored in the bean's {@link SettableAnyProperty} anySetter field if it exists.
*
* <p> This allows deserialization processes propagate values that initially don't match the target bean type for the
* <p>
* This allows deserialization processes propagate values that initially don't match the target bean type for the
* applicable field.
*
* <p> An example use-case is the use of placeholders (e.g. {@code ${aValue}}) in a field.
* <p>
* An example use-case is the use of placeholders (e.g. {@code ${aValue}}) in a field.
*/
@Override
public void deserializeAndSet(JsonParser p, DeserializationContext ctxt, Object instance) throws IOException {
Expand Down Expand Up @@ -171,9 +174,6 @@ private boolean shouldUseAnySetter() {
if (anySetter == null) {
return false;
}
if (Boolean.TRUE.equals(restrictToTemplates.get()) ) {
return KubernetesDeserializer.isInTemplate();
}
return true;
return useAnySetter.getAsBoolean();
}
}
40 changes: 31 additions & 9 deletions ...rialization/UnmatchedFieldTypeModule.java → ...del/jackson/UnmatchedFieldTypeModule.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
package io.fabric8.kubernetes.client.utils.serialization;
package io.fabric8.kubernetes.model.jackson;

import com.fasterxml.jackson.databind.BeanDescription;
import com.fasterxml.jackson.databind.DeserializationConfig;
Expand All @@ -40,6 +40,8 @@ public class UnmatchedFieldTypeModule extends SimpleModule {
private boolean logWarnings;
private boolean restrictToTemplates;

private static final ThreadLocal<Boolean> IN_TEMPLATE = ThreadLocal.withInitial(() -> false);

public UnmatchedFieldTypeModule() {
this(true, true);
}
Expand All @@ -50,19 +52,22 @@ public UnmatchedFieldTypeModule(boolean logWarnings, boolean restrictToTemplates
setDeserializerModifier(new BeanDeserializerModifier() {

@Override
public BeanDeserializerBuilder updateBuilder(DeserializationConfig config, BeanDescription beanDesc, BeanDeserializerBuilder builder) {
builder.getProperties().forEachRemaining(p ->
builder.addOrReplaceProperty(new SettableBeanPropertyDelegate(p, builder.getAnySetter(), UnmatchedFieldTypeModule.this::isRestrictToTemplates) {
}, true));
public BeanDeserializerBuilder updateBuilder(DeserializationConfig config, BeanDescription beanDesc,
BeanDeserializerBuilder builder) {
builder.getProperties().forEachRemaining(p -> builder.addOrReplaceProperty(
new SettableBeanPropertyDelegate(p, builder.getAnySetter(), UnmatchedFieldTypeModule.this::useAnySetter) {
}, true));
return builder;
}
});
setSerializerModifier(new BeanSerializerModifier() {
@Override
public BeanSerializerBuilder updateBuilder(SerializationConfig config, BeanDescription beanDesc, BeanSerializerBuilder builder) {
builder.setProperties(builder.getProperties().stream().map(p ->
new BeanPropertyWriterDelegate(p, builder.getBeanDescription().findAnyGetter(), UnmatchedFieldTypeModule.this::isLogWarnings))
.collect(Collectors.toList()));
public BeanSerializerBuilder updateBuilder(SerializationConfig config, BeanDescription beanDesc,
BeanSerializerBuilder builder) {
builder.setProperties(builder.getProperties().stream()
.map(p -> new BeanPropertyWriterDelegate(p, builder.getBeanDescription().findAnyGetter(),
UnmatchedFieldTypeModule.this::isLogWarnings))
.collect(Collectors.toList()));
return builder;
}
});
Expand All @@ -85,6 +90,10 @@ boolean isRestrictToTemplates() {
return restrictToTemplates;
}

boolean useAnySetter() {
return !restrictToTemplates || isInTemplate();
}

/**
* Sets if the DeserializerModifier should only be applied to Templates or object trees contained in Templates.
*
Expand All @@ -93,4 +102,17 @@ boolean isRestrictToTemplates() {
public void setRestrictToTemplates(boolean restrictToTemplates) {
this.restrictToTemplates = restrictToTemplates;
}

public static boolean isInTemplate() {
return Boolean.TRUE.equals(IN_TEMPLATE.get());
}

public static void setInTemplate() {
IN_TEMPLATE.set(true);
}

public static void removeInTemplate() {
IN_TEMPLATE.remove();
}

}
20 changes: 1 addition & 19 deletions ...netes-model-core/src/main/java/io/fabric8/kubernetes/internal/KubernetesDeserializer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -76,18 +76,11 @@ public boolean equals(Object obj) {

}

private static final String TEMPLATE_CLASS_NAME = "io.fabric8.openshift.api.model.Template";
private static final String KIND = "kind";
private static final String API_VERSION = "apiVersion";

private static final Mapping mapping = new Mapping();

private static final ThreadLocal<Boolean> IN_TEMPLATE = ThreadLocal.withInitial(() -> false);

public static boolean isInTemplate() {
return IN_TEMPLATE.get();
}

@Override
public KubernetesResource deserialize(JsonParser jp, DeserializationContext ctxt) throws IOException {
JsonNode node = jp.readValueAsTree();
Expand Down Expand Up @@ -121,18 +114,7 @@ private static KubernetesResource fromObjectNode(JsonParser jp, JsonNode node) t
if (resourceType == null) {
return jp.getCodec().treeToValue(node, GenericKubernetesResource.class);
} else if (KubernetesResource.class.isAssignableFrom(resourceType)) {
boolean inTemplate = false;
if (TEMPLATE_CLASS_NAME.equals(resourceType.getName())) {
inTemplate = true;
IN_TEMPLATE.set(true);
}
try {
return jp.getCodec().treeToValue(node, resourceType);
} finally {
if (inTemplate) {
IN_TEMPLATE.remove();
}
}
return jp.getCodec().treeToValue(node, resourceType);
}
throw new JsonMappingException(jp, String.format(
"There's a class loading issue, %s is registered as a KubernetesResource, but is not an instance of KubernetesResource",
Expand Down

0 comments on commit e19e8cd

Please sign in to comment.