Allow disabling Integer to String coercion via CoercionConfig

[emilkostadinov]

Describe the bug

I am experiencing issues with the new coercion settings that were released in v2.12.0. I would like to disable coercion between String and Integer.

Version information

v2.12.1

To Reproduce

I have class Example (using Lombok annotations):

@Getter
@Setter
@AllArgsConstructor
@NoArgsConstructor
public class Example {
    private String type;
}

The ObjectMapper setup:

ObjectMapper mapper = new ObjectMapper();
mapper.coercionConfigFor(LogicalType.Textual)
        .setCoercion(CoercionInputShape.Integer, CoercionAction.Fail);

How I try to deserialize:

Example example = mapper.readValue("{\"type\": 123}", Example.class);

Expected behavior

I expect an exception to be thrown saying that Integer cannot be converted to String. But what happens is that example is successfully created with type set to "123" which means there was conversion.

Is there anything wrong I do (maybe with the coercion configuration)?

[cowtowncoder]

@e1-emilkostadinov Thank you for reporting this! Your usage of the feature looks correct so that is not the problem.

Up until now coercions from other scalar types to String have been automatically allowed but with the new coercion-config settings it should be possible to prevent this. This requires some work for StringDeserializer (and perhaps couple of other related deserializers) to add applicable checks.

So this is just a case of missing checks and should be implemented: I mark it for 2.13 as the work would most likely be down when starting to consider features to add in 2.13, but it could probably be backported in 2.12 as well if change seems safe enough.

[stephenvsp]

Is there an ETA for 2.13?

[cowtowncoder]

@stephenvsp Not really; I would not expect it before maybe June-July 2021, ideally it'd be relatively small increase but it will have to compete with 3.0 development work.
I haven't had time to look into this feature (and probably won't any time soon), but I can help others with PRs more easily.

[jiamo]

After it success created.
We can only use (Integer) ((Object) map.get("type")) to use the map.

[WojciechLuczkow]

I use following walk-around for 2.12:

Create custom deserializer

public class CoercionLessStringDeserializer extends StringDeserializer {

  @Override
  public String deserialize(JsonParser p, DeserializationContext ctxt) throws IOException {

    List<JsonToken> forbiddenTypes =
        Arrays.asList(
            JsonToken.VALUE_NUMBER_INT,
            JsonToken.VALUE_NUMBER_FLOAT,
            JsonToken.VALUE_TRUE,
            JsonToken.VALUE_FALSE);

    if (forbiddenTypes.contains(p.getCurrentToken())) {
      String message =
          MessageFormat.format("Cannot coerce {0} to String value", p.getCurrentToken());
      throw MismatchedInputException.from(p, String.class, message);
    }
    return super.deserialize(p, ctxt);
  }
}

And use it with objectMapper

      SimpleModule deserializerModule = new SimpleModule();
      deserializerModule.addDeserializer(
          String.class, new CoercionLessStringDeserializer());
      mapper.registerModule(deserializerModule);

More advanced approach would be to use ctxt.findCoercionAction(logicalType(), handledType(), CoercionInputShape.?) for each possible JsonToken.

[cowtowncoder]

Thank you @WojciechLuczkow for showing so anyone who needs a work-around now can use it.
(there are alternatives, such as specifically looking for just VALUE_STRING but above works).

[Soggywaters]

For those who want to use this on a field of a model, you can also use the @JsonDeserialize(using=CoercionLessStringDeserializer.class) with @WojciechLuczkow's solution. Thank you for the deserializer!

[cowtowncoder]

Thank you for sharing @Soggywaters! I also marked this with "pr-welcome" since I probably won't have time to work on this soon, but think it should be quite doable if anyone else wanted to have a go. And I can help getting PR ready and merged even if not having time to implement it myself.

[Soggywaters]

Hi @cowtowncoder, Thank you for all that you do for this project.

About this issue - I would love to help where I can but does it really make sense to add this as a feature? I think the solution above works well for the problem (creating a deserializer extended from the string deserializer and leveraging the @JsonDeserialize annotation). Unless, you're talking about a simple annotation, like @EnforceStringOnly, where it would just implement a deserializer like above?

[cowtowncoder]

@Soggywaters it would not require any API addition but application of existing CoercionConfig functionality. This is a global (per ObjectMapper) setting, not new annotation.

I agree that a custom String deserializer works well too, and although in general writing proper fully feature deserialiazers is non-trivial, String type is simple enough (does not f.ex support polymorphism) that it's not very complicated.

[Tomasito665]

Hi all, I gave it my best shot in this branch: 2.14...Tomasito665:disable-int-to-string-coercion.
I'll wait two days and open the PR next Saturday for it to count towards my Hacktoberfest score 🍻.

[WojciechLuczkow]

@Tomasito665 There is some code regarding VALUE_NUMBER_INT, what about VALUE_NUMBER_FLOAT and VALUE_TRUE/FALSE? Also _nullProvider is a accessible through _findNullProvider field - do you need to pass it to _parseString?

[cowtowncoder]

If we are specifically talking about Integer-to-String coercion, neither VALUE_NUMBER_FLOAT nor VALUE_TRUE/VALUE_FALSE handling should be affected. They are not integers in this context (from JSON they are lexically different).

[Tomasito665]

Thanks for the feedback, both.

@WojciechLuczkow I am not sure whether I can use _findNullProvider. It takes a BeanProperty as an argument, which, as far as I can tell, is only available when ContextualSerializer and ContextualDeserializer context resolution occurs. Let me know if I am missing something.

[WojciechLuczkow]

@cowtowncoder Thats right - this issue is regarding Integer to String coercion, but as far I remember deserializer was also doing unexpected coercion for

mapper.coercionConfigFor(LogicalType.Textual)
        .setCoercion(CoercionInputShape.Float, CoercionAction.Fail);
mapper.readValue("{\"type\": 1.5}", Example.class);

and

mapper.coercionConfigFor(LogicalType.Textual)
        .setCoercion(CoercionInputShape.Boolean, CoercionAction.Fail);

mapper.readValue("{\"type1\": true, \"type2\": false}", Example.class);

@Tomasito665 You are right, sorry for making a fuss :)
That is quite complicated that's why after spending some time trying to propose fix i ended up with work-around, which I posted above.

[cowtowncoder]

@WojciechLuczkow Fair enough. Might be most economical to tackle other issues as well, if @Tomasito665 wants to do that. And if so, update issue title here.
Or if not, file a new issue or issues. Happy to get improvements either way.

EDIT: looking at code, I suggest we get this merged in first, then whoever wants can do PR for floating-point number and/or boolean case.

[Tomasito665]

@cowtowncoder sure! I'll see if I can include your suggestion on my PR to simplify the code tomorrow and file a separate PR for floats and booleans some time later during this weekend. Shall I link those to this issue or would you prefer to create a new one?

[cowtowncoder]

@Tomasito665 On issues, please create new one so PRs link nicely one-to-one. You may combine multiple new types (boolean and float) that's fine, but since I merged and closed this issue let's not link more changes to it.

[JooHyukKim]

@cowtowncoder Why is allowing scalar coercion on by default? It seems that this should be off by default. Obviously, this cannot be done for 2.x since that would break the world. But maybe for 3.x this should be off by default?

I am sure it's uninteded, but the questions sound like way too demanding for the questionee, asking for answers and explanations. 🤔

May I ask you to please file another issue @EarthCitizen and maybe link this issue back? There are "TEMPLATES" for requests, not like "I don't think this should be this way or that". All has been developed as intended.

[JooHyukKim]

Please know that I am not speaking on behalf of anybody, but just making an observation here. Thinking maybe the word "demanding" was not the best choice.

Question in its nature demands answers. What I saw problem is not how it is asked, but the question itself. You are right that the library is used by many people. Imagine that many people come and ask (I am sure politetely)

why feature A is this way? I think this should be that way

No context, no structure, not in the best form that can grow into a something meaningful.

Here we have a similiar question on a closed Github issue, again, no context no structure.

@cowtowncoder Why is allowing scalar coercion on by default? It seems that this should be off by default. Obviously, this cannot be done for 2.x since that would break the world. But maybe for 3.x this should be off by default?

It is great that you are contributing by sharing opinions, but would make a difference if it is more structured. With reasons, explanations, background and etc all in issue templates.

Following is what I wanted to say :

• If you are just asking questions, there are Github disucssion section, or stackoverflow.
• If you are making feature request (I think the case here), you can file a new issue.
• If the question belongs here, enough relavent to reopen the issue, requires more explanation currently we don't have any.