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

Add new @JsonTypeInfo.requireTypeIdForSubtypes usage #3891

Merged
merged 12 commits into from
Jun 6, 2023
Original file line number Diff line number Diff line change
Expand Up @@ -1547,6 +1547,9 @@ protected TypeResolverBuilder<?> _findTypeResolver(MapperConfig<?> config,
b = b.defaultImpl(defaultImpl);
}
b = b.typeIdVisibility(info.visible());
// [databind#3877]: per-type strict type handling. No need for null-checking of {@code OptBoolean.asBoolean()}
// value, because it will be done during construction of deserializer.
b.requireTypeIdForSubtypes(info.requireTypeIdForSubtypes().asBoolean());
Copy link
Member

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

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

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

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

return b;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.annotation.JsonTypeInfo.As;
import com.fasterxml.jackson.annotation.OptBoolean;
import com.fasterxml.jackson.databind.DeserializationConfig;
import com.fasterxml.jackson.databind.JavaType;
import com.fasterxml.jackson.databind.SerializationConfig;
Expand Down Expand Up @@ -153,6 +154,21 @@ public TypeDeserializer buildTypeDeserializer(DeserializationConfig config,
*/
public T typeIdVisibility(boolean isVisible);

/**
* Specifies whether strict type ID handling should be used for this type.
* Parameter {@code Boolean requireTypeId} is provided by {@link JsonTypeInfo#requireTypeIdForSubtypes()}.
* This configuration overrides the global setting defined by
* {@link com.fasterxml.jackson.databind.MapperFeature#REQUIRE_TYPE_ID_FOR_SUBTYPES}.
*
* @param requireTypeId {@code true} to enforce type ID handling, {@code false} otherwise.
* If {@code null}, the global setting will be used.
*
* @since 2.16
*/
public default void requireTypeIdForSubtypes(Boolean requireTypeId) {
// no-op
}

Copy link
Member Author

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

/*
/**********************************************************************
/* Mutant factories (2.13+)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,15 @@ public class StdTypeResolverBuilder

protected TypeIdResolver _customIdResolver;

/**
* Boolean value configured through {@link JsonTypeInfo#requireTypeIdForSubtypes}.
* If this value is not {@code null}, this value overrides the global configuration of
* {@link com.fasterxml.jackson.databind.MapperFeature#REQUIRE_TYPE_ID_FOR_SUBTYPES}.
*
* @since 2.16
*/
protected Boolean _requireTypeIdForSubtypes;
Copy link
Member Author

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


/*
/**********************************************************
/* Construction, initialization, actual building
Expand Down Expand Up @@ -275,6 +284,11 @@ public StdTypeResolverBuilder withDefaultImpl(Class<?> defaultImpl) {
return new StdTypeResolverBuilder(this, defaultImpl);
}

@Override
public void requireTypeIdForSubtypes(Boolean requireTypeId) {
_requireTypeIdForSubtypes = requireTypeId;
}

/*
/**********************************************************
/* Accessors
Expand Down Expand Up @@ -405,7 +419,10 @@ protected boolean allowPrimitiveTypes(MapperConfig<?> config,

/**
* Determines whether strict type ID handling should be used for this type or not.
* This will be enabled when either the type has type resolver annotations or if
* This will be enabld as configured by {@link JsonTypeInfo#requireTypeIdForSubtypes()}
* unless its value is {@link com.fasterxml.jackson.annotation.OptBoolean#DEFAULT}.
* In case the value of {@link JsonTypeInfo#requireTypeIdForSubtypes()} is {@code OptBoolean.DEFAULT},
* this will be enabled when either the type has type resolver annotations or if
* {@link com.fasterxml.jackson.databind.MapperFeature#REQUIRE_TYPE_ID_FOR_SUBTYPES}
* is enabled.
*
Expand All @@ -418,6 +435,10 @@ protected boolean allowPrimitiveTypes(MapperConfig<?> config,
* @since 2.15
*/
protected boolean _strictTypeIdHandling(DeserializationConfig config, JavaType baseType) {
// [databind#3877]: per-type strict type handling, since 2.16
if (_requireTypeIdForSubtypes != null && baseType.isConcrete()) {
return _requireTypeIdForSubtypes;
}
if (config.isEnabled(MapperFeature.REQUIRE_TYPE_ID_FOR_SUBTYPES)) {
return true;
}
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,116 @@
package com.fasterxml.jackson.databind.jsontype;

import com.fasterxml.jackson.annotation.JsonSubTypes;
import com.fasterxml.jackson.annotation.JsonTypeInfo;
import com.fasterxml.jackson.annotation.JsonTypeInfo.Id;
import com.fasterxml.jackson.annotation.OptBoolean;
import com.fasterxml.jackson.databind.BaseMapTest;
import com.fasterxml.jackson.databind.MapperFeature;
import com.fasterxml.jackson.databind.ObjectMapper;
import com.fasterxml.jackson.databind.exc.InvalidTypeIdException;
import com.fasterxml.jackson.databind.json.JsonMapper;

// [databind#3877]: allow configuration of per-type strict type handling
public class OverrideStrictTypeInfoHandling3877Test extends BaseMapTest {

/*
/**********************************************************
/* Set Up
/**********************************************************
*/

@JsonTypeInfo(use = Id.NAME, requireTypeIdForSubtypes = OptBoolean.DEFAULT)
@JsonSubTypes({
@JsonSubTypes.Type(value = DoDefaultCommand.class, name = "do-default")})
interface DefaultCommand {}

static class DoDefaultCommand implements DefaultCommand {}

@JsonTypeInfo(use = Id.NAME, requireTypeIdForSubtypes = OptBoolean.TRUE)
@JsonSubTypes({
@JsonSubTypes.Type(value = DoTrueCommand.class, name = "do-true")})
interface TrueCommand {}

static class DoTrueCommand implements TrueCommand {}

@JsonTypeInfo(use = Id.NAME, requireTypeIdForSubtypes = OptBoolean.FALSE)
@JsonSubTypes({
@JsonSubTypes.Type(value = DoFalseCommand.class, name = "do-false")})
interface FalseCommand {}

static class DoFalseCommand implements FalseCommand {}

/*
/**********************************************************
/* Tests
/**********************************************************
*/

private final ObjectMapper ENABLED_MAPPER = JsonMapper.builder().enable(MapperFeature.REQUIRE_TYPE_ID_FOR_SUBTYPES).build();
private final ObjectMapper DISABLED_MAPPER = JsonMapper.builder().disable(MapperFeature.REQUIRE_TYPE_ID_FOR_SUBTYPES).build();
private final ObjectMapper DEFAULT_MAPPER = JsonMapper.builder().build();

public void testMissingTypeId() throws Exception {
// super types fail on missing-id no matter what
verifyFailureMissingTypeId("{}", FalseCommand.class, ENABLED_MAPPER);
verifyFailureMissingTypeId("{}", FalseCommand.class, DEFAULT_MAPPER);
verifyFailureMissingTypeId("{}", FalseCommand.class, DISABLED_MAPPER);
verifyFailureMissingTypeId("{}", TrueCommand.class, ENABLED_MAPPER);
verifyFailureMissingTypeId("{}", TrueCommand.class, DEFAULT_MAPPER);
verifyFailureMissingTypeId("{}", TrueCommand.class, DISABLED_MAPPER);
verifyFailureMissingTypeId("{}", DefaultCommand.class, ENABLED_MAPPER);
verifyFailureMissingTypeId("{}", DefaultCommand.class, DEFAULT_MAPPER);
verifyFailureMissingTypeId("{}", DefaultCommand.class, DISABLED_MAPPER);

// overrides : to require type id
verifySuccessWithNonNullAndType("{}", DoFalseCommand.class, ENABLED_MAPPER);
verifySuccessWithNonNullAndType("{}", DoFalseCommand.class, DEFAULT_MAPPER);
verifySuccessWithNonNullAndType("{}", DoFalseCommand.class, DISABLED_MAPPER);
// overrides : do not require type id
verifyFailureMissingTypeId("{}", DoTrueCommand.class, ENABLED_MAPPER);
verifyFailureMissingTypeId("{}", DoTrueCommand.class, DEFAULT_MAPPER);
verifyFailureMissingTypeId("{}", DoTrueCommand.class, DISABLED_MAPPER);
// overrides : defaults
verifyFailureMissingTypeId("{}", DoDefaultCommand.class, ENABLED_MAPPER);
verifyFailureMissingTypeId("{}", DoDefaultCommand.class, DEFAULT_MAPPER);
verifySuccessWithNonNullAndType("{}", DoDefaultCommand.class, DISABLED_MAPPER);
}

public void testSuccessWhenTypeIdIsProvided() throws Exception {
verifySuccessWithNonNullAndType(a2q("{'@type': 'do-false'}"), FalseCommand.class, ENABLED_MAPPER);
verifySuccessWithNonNullAndType(a2q("{'@type': 'do-false'}"), FalseCommand.class, DEFAULT_MAPPER);
verifySuccessWithNonNullAndType(a2q("{'@type': 'do-false'}"), FalseCommand.class, DISABLED_MAPPER);
verifySuccessWithNonNullAndType(a2q("{'@type': 'do-false'}"), DoFalseCommand.class, ENABLED_MAPPER);
verifySuccessWithNonNullAndType(a2q("{'@type': 'do-false'}"), DoFalseCommand.class, DEFAULT_MAPPER);
verifySuccessWithNonNullAndType(a2q("{'@type': 'do-false'}"), DoFalseCommand.class, DISABLED_MAPPER);

verifySuccessWithNonNullAndType(a2q("{'@type': 'do-true'}"), TrueCommand.class, ENABLED_MAPPER);
verifySuccessWithNonNullAndType(a2q("{'@type': 'do-true'}"), TrueCommand.class, DEFAULT_MAPPER);
verifySuccessWithNonNullAndType(a2q("{'@type': 'do-true'}"), TrueCommand.class, DISABLED_MAPPER);
verifySuccessWithNonNullAndType(a2q("{'@type': 'do-true'}"), DoTrueCommand.class, ENABLED_MAPPER);
verifySuccessWithNonNullAndType(a2q("{'@type': 'do-true'}"), DoTrueCommand.class, DEFAULT_MAPPER);
verifySuccessWithNonNullAndType(a2q("{'@type': 'do-true'}"), DoTrueCommand.class, DISABLED_MAPPER);

verifySuccessWithNonNullAndType(a2q("{'@type': 'do-default'}"), DefaultCommand.class, ENABLED_MAPPER);
verifySuccessWithNonNullAndType(a2q("{'@type': 'do-default'}"), DefaultCommand.class, DEFAULT_MAPPER);
verifySuccessWithNonNullAndType(a2q("{'@type': 'do-default'}"), DefaultCommand.class, DISABLED_MAPPER);
verifySuccessWithNonNullAndType(a2q("{'@type': 'do-default'}"), DoDefaultCommand.class, ENABLED_MAPPER);
verifySuccessWithNonNullAndType(a2q("{'@type': 'do-default'}"), DoDefaultCommand.class, DEFAULT_MAPPER);
verifySuccessWithNonNullAndType(a2q("{'@type': 'do-default'}"), DoDefaultCommand.class, DISABLED_MAPPER);
}

private <T> void verifySuccessWithNonNullAndType(String json, Class<T> clazz, ObjectMapper om) throws Exception {
T bean = om.readValue(json, clazz);
assertNotNull(bean);
assertType(bean, clazz);
}

private void verifyFailureMissingTypeId(String json, Class<?> clazz, ObjectMapper om) throws Exception {
try {
om.readValue(json, clazz);
fail("Should not pass");
} catch (InvalidTypeIdException e) {
verifyException(e, "missing type id property '@type'");
}
}
}