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

Support adding marker interface to oneOfs. #273

Merged
merged 6 commits into from Apr 12, 2024

Conversation

cjbooms
Copy link
Owner

@cjbooms cjbooms commented Apr 11, 2024

While the discriminator property is needed to enable full polymorphism with Jackson, it should not be a blocker to applying the sealed interface to classes that deserve it.

This PR allows the interface to be applied, without the Jackson polymorphism annotations:

public sealed interface State

public data class StateA(
    @get:JsonProperty("status")
    @get:NotNull
    public val status: String,
) : State

public data class StateB(
    @get:JsonProperty("mode")
    @get:NotNull
    public val mode: String,
) : State

Jackson won't be able to figure out which object to deserialise to, but it is beneficial to have the marker interface in code

@cjbooms cjbooms marked this pull request as ready for review April 11, 2024 18:33
@cjbooms cjbooms force-pushed the support-oneof-marker-interfaces branch from 25d8a7e to 65495e2 Compare April 11, 2024 18:35
@cjbooms
Copy link
Owner Author

cjbooms commented Apr 11, 2024

@pschichtel I have started using the SEALED_INTERFACES_FOR_ONE_OF feature you added in some work projects recently, and I have to say I am a big fan. 💯

Possibly I should make it the default behaviour in a version soon.

This PR extends support even more for a different use case I have.

@param:JsonProperty("state")
@get:JsonProperty("state")
@get:Valid
public val state: State? = null,
Copy link
Owner Author

@cjbooms cjbooms Apr 11, 2024

Choose a reason for hiding this comment

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

Here I am not sure whether to reference State when the Polymorphic annotations are missing, or whether I should have it set to Any to ensure deserialisation is not broken

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nor I. Well, the model generated is correct but having a model that fails at deserialization defeats the purpose 🤔 I'd slightly lean for Any, but I'm on the fence.

Copy link
Owner Author

Choose a reason for hiding this comment

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

OK, changed to Any. That at least preserves the existing behaviour better. If anyone comes looking for it, we can add a new configuration option to allow them to opt-in

@cjbooms cjbooms requested a review from averabaq April 11, 2024 20:19
Copy link
Collaborator

@averabaq averabaq left a comment

Choose a reason for hiding this comment

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

Gotcha! 👍

@param:JsonProperty("state")
@get:JsonProperty("state")
@get:Valid
public val state: State? = null,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nor I. Well, the model generated is correct but having a model that fails at deserialization defeats the purpose 🤔 I'd slightly lean for Any, but I'm on the fence.

While the discriminator property is needed to enable full polymorphism with Jackson, it should not be a blocker to  applying the sealed interface to classes that deserve it.

This PR makes a first attempt at allowing the interface to be applied, without the Jackson polymorphism annotations
@cjbooms cjbooms force-pushed the support-oneof-marker-interfaces branch from 01b8b5f to 54967df Compare April 12, 2024 10:22
@cjbooms cjbooms merged commit 6645d86 into master Apr 12, 2024
1 check passed
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 this pull request may close these issues.

None yet

2 participants