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.

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

UnmatchedFieldTypeModule prevents certain jackson features from working #6342

Closed
fxshlein opened this issue Sep 11, 2024 · 6 comments · Fixed by #6343
Closed

UnmatchedFieldTypeModule prevents certain jackson features from working #6342

fxshlein opened this issue Sep 11, 2024 · 6 comments · Fixed by #6343
Assignees
Milestone

Comments

@fxshlein
Copy link
Contributor

fxshlein commented Sep 11, 2024

Describe the bug

I'm using the jackson-module-kotlin, registered in KubernetesSerialization. When deserializing a kotlin object, the SettableBeanPropertyDelegate that is added here does not properly delegate some functions from jackson (in this case, getCreatorIndex()). These are implemented with a default value by jackson, but they can be overridden by special properties, like kotlin constructor properties.

Fabric8 Kubernetes Client version

6.13.3

Steps to reproduce

  1. Have a CRD represented by a kotlin data class
  2. Try to serialize/deserialize that resource using the KubernetesSerialization

Expected behavior

I think this is a simple oversight, and the delegate is probably supposed implement and delegate all the functions.

Runtime

other (please specify in additional context)

Kubernetes API Server version

other (please specify in additional context)

Environment

other (please specify in additional context)

Fabric8 Kubernetes Client Logs

io.fabric8.kubernetes.client.KubernetesClientException: An error has occurred.
	at io.fabric8.kubernetes.client.KubernetesClientException.launderThrowable(KubernetesClientException.java:129)
	at io.fabric8.kubernetes.client.KubernetesClientException.launderThrowable(KubernetesClientException.java:122)
	at io.fabric8.kubernetes.client.utils.KubernetesSerialization.unmarshal(KubernetesSerialization.java:261)
	at io.fabric8.kubernetes.client.dsl.internal.OperationSupport.lambda$handleResponse$0(OperationSupport.java:551)
	at java.base/java.util.concurrent.CompletableFuture$UniApply.tryFire(CompletableFuture.java:646)
	at java.base/java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:510)
	at java.base/java.util.concurrent.CompletableFuture.complete(CompletableFuture.java:2179)
	at io.fabric8.kubernetes.client.http.StandardHttpClient.lambda$completeOrCancel$10(StandardHttpClient.java:142)
	at java.base/java.util.concurrent.CompletableFuture.uniWhenComplete(CompletableFuture.java:863)
	at java.base/java.util.concurrent.CompletableFuture$UniWhenComplete.tryFire(CompletableFuture.java:841)
	at java.base/java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:510)
	at java.base/java.util.concurrent.CompletableFuture.complete(CompletableFuture.java:2179)
	at io.fabric8.kubernetes.client.http.ByteArrayBodyHandler.onBodyDone(ByteArrayBodyHandler.java:51)
	at java.base/java.util.concurrent.CompletableFuture.uniWhenComplete(CompletableFuture.java:863)
	at java.base/java.util.concurrent.CompletableFuture$UniWhenComplete.tryFire(CompletableFuture.java:841)
	at java.base/java.util.concurrent.CompletableFuture.postComplete(CompletableFuture.java:510)
	at java.base/java.util.concurrent.CompletableFuture.complete(CompletableFuture.java:2179)
	at io.fabric8.kubernetes.client.okhttp.OkHttpClientImpl$OkHttpAsyncBody.doConsume(OkHttpClientImpl.java:136)
	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1144)
	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:642)
	at java.base/java.lang.Thread.run(Thread.java:1583)
Caused by: com.fasterxml.jackson.databind.JsonMappingException: Internal error: no creator index for property 'enabled' (of type io.fabric8.kubernetes.model.jackson.SettableBeanPropertyDelegate) (through reference chain: ...)
	at com.fasterxml.jackson.databind.JsonMappingException.wrapWithPath(JsonMappingException.java:402)
	at com.fasterxml.jackson.databind.JsonMappingException.wrapWithPath(JsonMappingException.java:361)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.wrapAndThrow(BeanDeserializerBase.java:1964)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeWithErrorWrapping(BeanDeserializer.java:580)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeUsingPropertyBased(BeanDeserializer.java:447)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromObjectUsingNonDefault(BeanDeserializerBase.java:1497)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserializeFromObject(BeanDeserializer.java:348)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:185)
	at com.fasterxml.jackson.databind.deser.impl.MethodProperty.deserializeAndSet(MethodProperty.java:129)
	at io.fabric8.kubernetes.model.jackson.SettableBeanPropertyDelegate.deserializeAndSet(SettableBeanPropertyDelegate.java:134)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.vanillaDeserialize(BeanDeserializer.java:310)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:177)
	at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:342)
	at com.fasterxml.jackson.databind.ObjectReader._bindAndClose(ObjectReader.java:2125)
	at com.fasterxml.jackson.databind.ObjectReader.readValue(ObjectReader.java:1501)
	at io.fabric8.kubernetes.client.utils.KubernetesSerialization.unmarshal(KubernetesSerialization.java:257)
	... 18 more
Caused by: java.lang.IllegalStateException: Internal error: no creator index for property 'enabled' (of type io.fabric8.kubernetes.model.jackson.SettableBeanPropertyDelegate)
	at com.fasterxml.jackson.databind.deser.SettableBeanProperty.getCreatorIndex(SettableBeanProperty.java:450)
	at com.fasterxml.jackson.databind.deser.impl.PropertyValueBuffer.assignParameter(PropertyValueBuffer.java:327)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeUsingPropertyBased(BeanDeserializer.java:449)
	at com.fasterxml.jackson.databind.deser.BeanDeserializerBase.deserializeFromObjectUsingNonDefault(BeanDeserializerBase.java:1497)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserializeFromObject(BeanDeserializer.java:348)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer.deserialize(BeanDeserializer.java:185)
	at com.fasterxml.jackson.databind.deser.SettableBeanProperty.deserialize(SettableBeanProperty.java:543)
	at com.fasterxml.jackson.databind.deser.BeanDeserializer._deserializeWithErrorWrapping(BeanDeserializer.java:578)
	... 30 more

Additional context

This is happening anywhere and is not really platform-dependent.

Jackson provides SettableBeanProperty.Delegating for reliably delegating all properties, and using it as a supertype I fixed the issue (although registering this was a bit hacky). The delegate I'm using instead of SettableBeanPropertyDelegate looks like this:

open class JacksonSettableBeanPropertyDelegate(
    delegate: SettableBeanProperty,
    private val anySetter: SettableAnyProperty?,
    private val useAnySetter: BooleanSupplier,
) : SettableBeanProperty.Delegating(delegate) {
    override fun withDelegate(d: SettableBeanProperty): SettableBeanProperty {
        return JacksonSettableBeanPropertyDelegate(d, anySetter, useAnySetter)
    }

    @Throws(IOException::class)
    override fun deserializeAndSet(p: JsonParser, ctxt: DeserializationContext?, instance: Any?) {
        try {
            delegate.deserializeAndSet(p, ctxt, instance)
        } catch (ex: MismatchedInputException) {
            if (shouldUseAnySetter()) {
                anySetter!![instance, delegate.name] = p.text
            } else {
                throw ex
            }
        }
    }

    @Throws(IOException::class)
    override fun deserializeSetAndReturn(p: JsonParser?, ctxt: DeserializationContext?, instance: Any?): Any? {
        try {
            return delegate.deserializeSetAndReturn(p, ctxt, instance)
        } catch (ex: MismatchedInputException) {
            deserializeAndSet(p!!, ctxt, instance)
        }
        return null
    }

    private fun shouldUseAnySetter(): Boolean {
        if (anySetter == null) {
            return false
        }
        return useAnySetter.asBoolean
    }
}
fxshlein added a commit to fxshlein/kubernetes-client that referenced this issue Sep 11, 2024
Changes the delegation in SettableBeanPropertyDelegate from
a custom implementation to the standard way of implementing
a delegating property in jackson. This way, if some jackson
module overrides methods that are not delegated explicitely
here, they will continue to work.

Fixes: fabric8io#6342
fxshlein added a commit to fxshlein/kubernetes-client that referenced this issue Sep 11, 2024
Changes the delegation in SettableBeanPropertyDelegate from
a custom implementation to the standard way of implementing
a delegating property in jackson. This way, if some jackson
module overrides methods that are not delegated explicitely
here, they will continue to work.

Fixes: fabric8io#6342
@fxshlein
Copy link
Contributor Author

Created a PR that changes to SettableBeanProperty.Delegating 🙂

@manusa
Copy link
Member

manusa commented Sep 17, 2024

Hi @fxshlein,

Sorry for the late reply. The core team had a face to face meeting during last week and we were swamped with meetings.

Thanks for looking into this and for your pull request.

When deserializing a kotlin object, the SettableBeanPropertyDelegate that is added here does not properly delegate some functions from jackson (in this case, getCreatorIndex()).
[...]
I think this is a simple oversight, and the delegate is probably supposed implement and delegate all the functions.
[...]
Jackson provides SettableBeanProperty.Delegating for reliably delegating all properties, and using it as a supertype I fixed the issue (although registering this was a bit hacky).

I implemented the problematic module and I'm not sure why I overlooked so many things (the existing delegator superclass + missing implementation overrides).
Best guess is that some of the methods have been created later, although after a first quick glance at the class this doesn't seem likely.

Let me dig a little deeper to see if there's some other reasoning behind this before I merge your fix.

@manusa manusa moved this to In Progress in Eclipse JKube Sep 17, 2024
@manusa manusa self-assigned this Sep 17, 2024
@fxshlein
Copy link
Contributor Author

No worries, hope the meetings were enjoyable!

I'm not sure why I overlooked so many things

To be fair, it's jackson... it has every feature imaginable, but sometimes discovering them is as hard as just writing them from scratch in the first place 😄

@shawkins
Copy link
Contributor

To be fair, it's jackson... it has every feature imaginable, but sometimes discovering them is as hard as just writing them from scratch in the first place

This has come up before #5028 and we raised an upstream issue on some of the design issues FasterXML/jackson-databind#3862 - which confirmed we're basically stuck with a problem for a while.

If it crept in this time via an upgrade we can consider something like adding a unit test to look for the declared method count on the base class to capture an approved number of methods - if it changes with an upgrade of Jackson someone will have to review the new methods to see if new logic is required before updating the expected method count.

@fxshlein
Copy link
Contributor Author

Yeah, sadly the writer doesn't seem to have any delegation mechanism, only SettableBeanProperty.

manusa pushed a commit to fxshlein/kubernetes-client that referenced this issue Sep 20, 2024
Changes the delegation in SettableBeanPropertyDelegate from
a custom implementation to the standard way of implementing
a delegating property in jackson. This way, if some jackson
module overrides methods that are not delegated explicitely
here, they will continue to work.

Fixes: fabric8io#6342
@manusa
Copy link
Member

manusa commented Sep 20, 2024

I added a few extra-commits to #6343

  • Implemented additional methods that weren't covered by SettableBeanProperty.Delegating
  • Added a test to ensure that all methods are implemented in future versions of Jackson

With these changes, I think we should be safe from any future issue related to this component.

@manusa manusa added this to the 7.0.0 milestone Sep 20, 2024 — with automated-tasks
@manusa manusa closed this as completed in d881c0e Sep 23, 2024
@github-project-automation github-project-automation bot moved this from Review to Done in Eclipse JKube Sep 23, 2024
@github-project-automation github-project-automation bot moved this from Review to Done in Eclipse JKube Sep 23, 2024
manusa added a commit that referenced this issue Sep 25, 2024
…n (6343)

fix(model): Use SettableBeanProperty.Delegating for jackson delegation

Changes the delegation in SettableBeanPropertyDelegate from
a custom implementation to the standard way of implementing
a delegating property in jackson. This way, if some jackson
module overrides methods that are not delegated explicitely
here, they will continue to work.

Fixes: #6342
---
fix: SettableBeanProperty.deserializeSetAndReturn should return instance instead of null

Signed-off-by: Marc Nuri <marc@marcnuri.com>
---
test:refactor: SettableBeanPropertyDelegateTest doesn't rely on mocks

Signed-off-by: Marc Nuri <marc@marcnuri.com>
---
fix: SettableBeanPropertyDelegate implements all methods from SettableBeanProperty

Includes tests to ensure all methods are implemented in future
Jackson versions too.

Signed-off-by: Marc Nuri <marc@marcnuri.com>

Co-authored-by: Marc Nuri <marc@marcnuri.com>
Signed-off-by: Marc Nuri <marc@marcnuri.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants