Add enum features into JsonFormat.Feature #3731
Conversation
| @@ -1712,7 +1712,10 @@ public JsonDeserializer<?> createEnumDeserializer(DeserializationContext ctxt, | |||
| if (deser == null) { | |||
| deser = new EnumDeserializer(constructEnumResolver(enumClass, | |||
| config, beanDesc.findJsonValueAccessor()), | |||
| config.isEnabled(MapperFeature.ACCEPT_CASE_INSENSITIVE_ENUMS)); | |||
| config.isEnabled(MapperFeature.ACCEPT_CASE_INSENSITIVE_ENUMS), | |||
| ctxt.isEnabled(DeserializationFeature.READ_UNKNOWN_ENUM_VALUES_USING_DEFAULT_VALUE), | |||
There was a problem hiding this comment.
Hmmmh. This is problematic since DeserializationFeatures are meant to be per-call basis; that is, its value may be changed via ObjectReader so we cannot assume that value at this point will not change.
This is different from MapperFeatures which are per-mapper and cannot be changed.
Put another way, DeserializationFeature.READ_UNKNOWN_ENUM_VALUES_USING_DEFAULT_VALUE needs to be accessed dynamically when needed, not on constructor.
There was a problem hiding this comment.
Thanks for looking into the PR.
I have removed the features from constructor in BasicDeserializerFactory.
|
Sounds good. There's some work needed but in general I like the addition. One practical thing: before merging I need CLA: https://github.com/FasterXML/jackson/blob/master/contributor-agreement.pdf (unless you have sent one in the past -- it only needs to be done once,ever) The usual way is to print the doc, fill & sign, scan/photo, email to Looking forward to getting these PRs merged, included in 2.15! |
Thanks @cowtowncoder , I have sent the Signed CLA document. |
|
@cowtowncoder Can you please guide me if something else needs to be done here? Thanks. |
| @@ -213,4 +224,38 @@ public void testEnumWithAliasAndDefaultForUnknownValueEnabled() throws Exception | |||
| MyEnum2352_3 multipleAliases2 = reader.readValue(q("multipleAliases2")); | |||
| assertEquals(MyEnum2352_3.C, multipleAliases2); | |||
| } | |||
|
|
|||
| public void testEnumWithDefaultForUnknownValueEnabled() throws JsonProcessingException { | |||
There was a problem hiding this comment.
Please just use throws Exception here since JsonProcessingException is renamed in 3.0 (master).
|
@Siwach16 I hope to get all of this reviewed soon; will add some notes but I think it looks good overall. |
| verifyException(e, "[JACKSON, OK, RULES]"); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
In addition to testing single Enum property it'd be good to see if things work for EnumSets?
I don't remember if things might just work due to delegation, but it might be necessary to change EnumSetDeserializer. Adding a test would clarify this.
There was a problem hiding this comment.
Added the test cases for EnumSet. Please have look if behaviour is expected.Thanks.
|
Planning to get this merged today. |
| @@ -77,14 +81,16 @@ public EnumDeserializer(EnumResolver byNameResolver, Boolean caseInsensitive) | |||
| /** | |||
| * @since 2.9 | |||
| */ | |||
| protected EnumDeserializer(EnumDeserializer base, Boolean caseInsensitive) | |||
There was a problem hiding this comment.
Will need to leave old constructor for backwards-compatibility; I can add it after merging.
Add enum features into JsonFormat.Feature
Related PR: FasterXML/jackson-annotations#211
Issue : #3637