UnmatchedFieldTypeModule to deal with mismatched target types for fields

[manusa]

Description

This is a partial implementation of what's described in #3460 (comment)

The new behavior is as follows (edited after #3574 (comment)):

1. A resource is deserialized into a KubernetesResource instance.
2. The resource can contain unknown fields and/or fields with mismatched types (e.g. DesploymentSpec#replicas with a String instead of an Integer)
3. Properties matching any of the categories mentioned in the previous step can stored in the additionalProperties Map (if present)
   a. By default, this unmatched types deserialization is enabled only for Templates
   b. Behavior can be enabled for all types by Serialization.UNMATCHED_FIELD_TYPE_MODULE.setRestrictToTemplates(false);
4. The resource is deserialized as a valid KubernetesResource instance (if applicable), no need to fallback into a GenericKubernetesResource
5. The object instance can now be processed as usual.
6. The object instance is serialized into a YAML/JSON representation
7. The object fields are persisted unless a property matching the field name is defined in the additionalProperties Map. In that case the field value is ignored.
   a. A warning will be logged if the situation is detected, indicating that a field is being ignored.
   b. Log can be opted-out by: Serialization.UNMATCHED_FIELD_TYPE_MODULE.setLogWarnings(false);
8. All properties defined in the additionalProperties Map are serialized.

All of this logic is implemented in the UnmatchedFieldTypeModule Jackson module that sets a serializer and a deserializer modifier. The second commit enables this module by default.

The only drawback when enabling the module is that Exceptions when deserializing input resources that have properties that don't match the target field type won't be thrown. Any user relying on this to validate an Input will lose this functionality (I'm not sure if this is an extended approach).

I would like to know if we should preserve the second commit (where the module is automatically enabled), or revert it and just provide the module as an opt-in feature. (@shawkins, @metacosm, @rohanKanojia, @oscerd, @iocanel)

The second commit also reverts the fallback implemented in #3526 which should no longer be necessary.

Relates to:

• Loading a template with an Integer parameter value fails deserialisation  #448
• Template doesn't support ${{ }} #959
• Error when load yaml with parameters #1979
• k8s:helm with template for integer values fails deserialization (Similar to https://github.com/fabric8io/kubernetes-client/issues/448) eclipse-jkube/jkube#778

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change
• Chore (non-breaking change which doesn't affect codebase;
  test, version modification, documentation, etc.)

Checklist

• Code contributed by me aligns with current project license: Apache 2.0
• I Added CHANGELOG entry regarding this change
• I have implemented unit tests to cover my changes
• I have added/updated the javadocs and other documentation accordingly
• No new bugs, code smells, etc. in SonarCloud report
• I tested my code in Kubernetes
• I tested my code in OpenShift

[centos-ci]

Can one of the admins verify this patch?

[oscerd]

I would preserve the second commit

[shawkins]

or revert it and just provide the module as an opt-in feature.

For serialization the only practical difference compared to existing behavior is that if you set the field and additional properties it gets serialized twice - which should be invalid yaml. It might be good to throw an exception in this case - that will make it clear there's an ambiguity.

Any user relying on this to validate an Input will lose this functionality (I'm not sure if this is an extended approach).

That's a valid concern for deserialization. Seems like this behavior would be best enabled only for templates.

[manusa]

It might be good to throw an exception in this case - that will make it clear there's an ambiguity.

The custom serializer prevents exactly that. The value in additionalProperties takes precedence over any other and it gets serialized only once.

This circumstance could happen before (in fact we've seen it in some issue reports #3195). The new behavior will at least provide a valid YAML when serializing.

Maybe we can log a warning or an info message if the circumstance is detected (when the field is ignored).

[manusa]

Seems like this behavior would be best enabled only for templates.

The DeserializerModifier is applied per bean. There is no easy way to detect if the current bean (or its parents) is part of a list of Objects within a Template when updating the Deserializer builder.

We can only do something like this at the SettableBeanPropertyDelegate deserializeAndSetMethod and check the JsonParser tree. But it might not look very nice. I'll give it a try.

[manusa]

Hi @shawkins
Both of your suggestions should be addressed by the 3rd (logs) commit and the 4th commit (only enabled for templates).

[shawkins]

Both of your suggestions should be addressed by the 3rd (logs) commit and the 4th commit (only enabled for templates).

For a warning by default should the additional properties be java-doc'd as having primacy over regular properties? - perhaps added to an interface like strimzi defines https://github.com/strimzi/strimzi-kafka-operator/blob/79e28149b6dd8dd7b61e0ed60aae6d26fb93b1b7/api/src/main/java/io/strimzi/api/kafka/model/UnknownPropertyPreserving.java

Also this logic for isInTemplate won't work - it needs to preserve the state top down, if you have a template like:

---
apiVersion: v1
kind: Template
metadata:
  name: foo
parameters:
- name: PORT
  value: 80
objects:
- apiVersion: v1
  kind: Service
  metadata:
    name: foo
  spec:
    ports:
    - port: 8080
    selector:
      cheese: edam
    type: ExternalName
- apiVersion: v1
  kind: Service
  metadata:
    name: bar
  spec:
    ports:
    - port: ${PORT}
    selector:
      cheese: edam
    type: ExternalName

Where you hit another object before the one with the parameters, then the flag will get flipped prematurely.

[manusa]

For a warning by default should the additional properties be java-doc'd as having primacy over regular properties?

Well, the circumstance before was worst because the user ended up with an invalid JSON or YAML with duplicate entries. Now the user gets a warning in case the ambiguity is detected and ends up with a valid resource (but maybe unexpected).

This default is only applicable if the user is actually using the ObjectMappers provided by the Serialization class, so I'm not sure if the Javadoc should include that entry. If it did we should add the conditional.

In any case, I'm not sure how we could add this Javadoc easily to the project, since not all KubernetesResources might include the additional properties. Any ideas?

Where you hit another object before the one with the parameters, then the flag will get flipped prematurely.

Should be fixed now ✔️

[shawkins]

Well, the circumstance before was worst because the user ended up with an invalid JSON or YAML with duplicate entries. Now the user gets a warning in case the ambiguity is detected and ends up with a valid resource (but maybe unexpected).

The two concerns are that it's a behavioral change and it's subtle. Historically there's a lot of warnings generated by fabric8, so some people will have that logging turned off entirely.

Just in case it should be documented somehow and in the release notes.

In any case, I'm not sure how we could add this Javadoc easily to the project, since not all KubernetesResources might include the additional properties. Any ideas?

It would be up to the generation logic right? The interface would have to be added into the javaInterfaces list when additionalProperties is true. But that does seem like a lot of work.

[manusa]

It would be up to the generation logic right? The interface would have to be added into the javaInterfaces list when additionalProperties is true. But that does seem like a lot of work.

This is the only way I can think of, besides something similar to #3570 (comment). But both approaches imply extra work, that I really don't think adds any value.

I'll add an entry to the changelog.

Maybe some notes on the marshal/unmarhsal methods of the Serializer make sense too.

[sonarcloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

97.8% Coverage
0.0% Duplication

[shawkins]

LGTM