Skip to content

Add new @JsonTypeInfo.requireTypeIdForSubtypes usage #3891

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

Jump to bottom
Merged
merged 12 commits into from
Jun 6, 2023
Merged

Add new @JsonTypeInfo.requireTypeIdForSubtypes usage #3891

merged 12 commits into from
Jun 6, 2023

Conversation

@JooHyukKim
Copy link
Member

@JooHyukKim JooHyukKim commented Apr 21, 2023 •
edited
Loading

(originated from #2968 regarding per-type configuration of Polymorphic handling)

Description

This PR will be un-drafted after new attribute is added to @JsonTypeInfo by FasterXML/jackson-annotations#223. And this PR resolves #3877

Notes

  • Used attribute name as requireTypeIdForSubtypes to explicitly relate to MapperFeature.REQUIRE_TYPE_ID_FOR_SUBTYPES.

@JooHyukKim JooHyukKim changed the title Allow configuration of per-type configuration of strict type id handling Allow per-type configuration of strict type id handling Apr 21, 2023
@JooHyukKim JooHyukKim changed the title Allow per-type configuration of strict type id handling Add new OptBoolean valued property in @JsonTypeInfo to allow per-type configuration of strict type id handling Apr 21, 2023
@JooHyukKim JooHyukKim changed the title Add new OptBoolean valued property in @JsonTypeInfo to allow per-type configuration of strict type id handling Add new OptBoolean valued property in @JsonTypeInfo to allow per-type configuration of strict type id handling Apr 21, 2023
@JooHyukKim JooHyukKim mentioned this pull request Apr 21, 2023
Merged
@JooHyukKim JooHyukKim force-pushed the 3877-requireTypeIdForSubtype branch from de1dbc2 to 66632c3 Compare April 24, 2023 11:15
@JooHyukKim JooHyukKim changed the base branch from 2.15 to 2.16 April 24, 2023 11:15
cowtowncoder
cowtowncoder reviewed May 16, 2023
View reviewed changes
src/main/java/com/fasterxml/jackson/databind/introspect/JacksonAnnotationIntrospector.java Outdated
}
b = b.typeIdVisibility(info.visible());
// [databind#3877]: allow configuration of per-type strict type handling
b.requireTypeIdForSubtypes(info.requireTypeIdForSubtypes().asBoolean());
Copy link
Member

@cowtowncoder cowtowncoder May 16, 2023

Choose a reason for hiding this comment

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

The point of OptBoolean is to have "third option" of "not specified", so I think this should check that case (skip call if value is OptBoolean.DEFAULT)

Copy link
Member

@cowtowncoder cowtowncoder May 16, 2023

Choose a reason for hiding this comment

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

Oh. Actually, never mind. asBoolean() returns null for the case, so I think this is fine.

Copy link
Member

@yawkat yawkat May 16, 2023

Choose a reason for hiding this comment

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

add a comment though

Copy link
Member Author

@JooHyukKim JooHyukKim May 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

add a comment though

@yawkat Where at?

EDIT: you mean add java toasBoolean() method?
EDIT: added some comment about null-checks, let me know what you think ✌🏼✌🏼 @yawkat

@JooHyukKim JooHyukKim force-pushed the 3877-requireTypeIdForSubtype branch from 3df1268 to 3b10205 Compare May 16, 2023 09:39
JooHyukKim
JooHyukKim commented May 16, 2023
View reviewed changes
src/main/java/com/fasterxml/jackson/databind/jsontype/TypeResolverBuilder.java Outdated
* This is per-type override of {@link com.fasterxml.jackson.databind.MapperFeature#REQUIRE_TYPE_ID_FOR_SUBTYPES}.
* Sets {@link Boolean} value provided by {@link OptBoolean#asBoolean()} of configured {@link JsonTypeInfo#requireTypeIdForSubtypes}.
* <p>
* WARNING: This method will be abstract in Jackson 3.0.
Copy link
Member Author

@JooHyukKim JooHyukKim May 16, 2023

Choose a reason for hiding this comment

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

Suggested change
* WARNING: This method will be abstract in Jackson 3.0.

Will remove this line, because.... who knows 🤔🤔

@JooHyukKim JooHyukKim marked this pull request as ready for review May 16, 2023 10:11
@JooHyukKim JooHyukKim requested review from cowtowncoder and yawkat May 16, 2023 10:11
@JooHyukKim JooHyukKim force-pushed the 3877-requireTypeIdForSubtype branch from a4a272c to 6968ab3 Compare May 17, 2023 09:23
This was referenced May 18, 2023
Closed
Closed
JooHyukKim
JooHyukKim commented May 23, 2023
View reviewed changes
src/main/java/com/fasterxml/jackson/databind/jsontype/TypeResolverBuilder.java Outdated
public default void requireTypeIdForSubtypes(Boolean requireTypeId) {
// no-op
}

Copy link
Member Author

@JooHyukKim JooHyukKim May 23, 2023

Choose a reason for hiding this comment

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

There shouldn't be mutator here, but instead use @JsonTypeInfo.Value

JooHyukKim
JooHyukKim commented May 23, 2023
View reviewed changes
src/main/java/com/fasterxml/jackson/databind/jsontype/impl/StdTypeResolverBuilder.java Outdated
*
* @since 2.16
*/
protected Boolean _requireTypeIdForSubtypes;
Copy link
Member Author

@JooHyukKim JooHyukKim May 23, 2023

Choose a reason for hiding this comment

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

There shouldn't be mutator here, but instead use @JsonTypeInfo.Value

same

@cowtowncoder
Copy link
Member

cowtowncoder commented May 26, 2023

@JooHyukKim Is this particular PR still active, or is there a replacement wrt JsonTypeInfo.Value changes? (just want to make sure which ones are open etc)

@JooHyukKim
Copy link
Member Author

JooHyukKim commented May 26, 2023 •
edited
Loading

@cowtowncoder Ah, yes this PR is still active, originated from #2968 regarding per-type configuration of Polymorphic handling. I will update the PR message to indicate that accordingly 👍🏻

And fix conflicts 😄

@JooHyukKim JooHyukKim marked this pull request as draft May 26, 2023 22:12
@JooHyukKim JooHyukKim changed the title Add new OptBoolean valued property in @JsonTypeInfo to allow per-type configuration of strict type id handling Use @JsonTypeInfo. requireTypeIdForSubtypes to allow per-type configuration of strict type id handling May 27, 2023
@JooHyukKim JooHyukKim marked this pull request as ready for review May 27, 2023 04:02
@JooHyukKim JooHyukKim changed the title Use @JsonTypeInfo. requireTypeIdForSubtypes to allow per-type configuration of strict type id handling Allow per-type configuration of polymorphic type handling using @JsonTypeInfo.requireTypeIdForSubtypes May 28, 2023
@JooHyukKim
Copy link
Member Author

JooHyukKim commented May 29, 2023

@JooHyukKim Is this particular PR still active, or is there a replacement wrt JsonTypeInfo.Value changes? (just want to make sure which ones are open etc)

This PR is ready for a review now! 👍🏻 After improvements such as #3953 and FasterXML/jackson-annotations#223, changes have been simplified alot.

@JooHyukKim JooHyukKim changed the title Allow per-type configuration of polymorphic type handling using @JsonTypeInfo.requireTypeIdForSubtypes Add new @JsonTypeInfo.requireTypeIdForSubtypes usage May 31, 2023
@cowtowncoder cowtowncoder merged commit c5eabf8 into FasterXML:2.16 Jun 6, 2023
@cowtowncoder cowtowncoder added this to the 2.16.0 milestone Jun 6, 2023
@JooHyukKim JooHyukKim mentioned this pull request Jun 7, 2023
Closed
@JooHyukKim JooHyukKim deleted the 3877-requireTypeIdForSubtype branch June 16, 2023 11:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cowtowncoder cowtowncoder cowtowncoder approved these changes

@yawkat yawkat Awaiting requested review from yawkat

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.16.0

Development

Successfully merging this pull request may close these issues.

Add new OptBoolean valued property in @JsonTypeInfo, handling, to allow per-polymorphic type loose Type Id handling

3 participants

@JooHyukKim @cowtowncoder @yawkat