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

Afterburner does not support the new CoercionConfig #120

Closed
carterkozak opened this issue Dec 2, 2020 · 7 comments
Closed

Afterburner does not support the new CoercionConfig #120

carterkozak opened this issue Dec 2, 2020 · 7 comments

Comments

@carterkozak
Copy link

@carterkozak carterkozak commented Dec 2, 2020

I have a project which disables scalar coercion, however allows serialization and deserialization of java long, java.lang.Long, and java.util.OptionalLong values as strings to avoid javascript number truncation. Upgrading to 2.12.0 I replaced custom deserializers with CoercionConfig, however optimized accessors fail with:

com.fasterxml.jackson.databind.exc.MismatchedInputException: Cannot coerce JSON VALUE_STRING value ("1") into long (enable `MapperFeature.ALLOW_COERCION_OF_SCALARS` to allow)
 at [Source: (String)"{"value":"1"}"; line: 1, column: 10]

	at com.fasterxml.jackson.databind.exc.MismatchedInputException.from(MismatchedInputException.java:59)
	at com.fasterxml.jackson.databind.DeserializationContext.reportInputMismatch(DeserializationContext.java:1603)
	at com.fasterxml.jackson.module.afterburner.deser.OptimizedSettableBeanProperty._verifyScalarCoercion(OptimizedSettableBeanProperty.java:405)
	at com.fasterxml.jackson.module.afterburner.deser.OptimizedSettableBeanProperty._deserializeLong(OptimizedSettableBeanProperty.java:305)
	at com.fasterxml.jackson.module.afterburner.deser.SettableLongMethodProperty.deserializeAndSet(SettableLongMethodProperty.java:40)
	at com.fasterxml.jackson.module.afterburner.deser.SuperSonicBeanDeserializer.deserialize(SuperSonicBeanDeserializer.java:159)
	at com.fasterxml.jackson.databind.deser.DefaultDeserializationContext.readRootValue(DefaultDeserializationContext.java:322)
	at com.fasterxml.jackson.databind.ObjectMapper._readMapAndClose(ObjectMapper.java:4591)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3546)
	at com.fasterxml.jackson.databind.ObjectMapper.readValue(ObjectMapper.java:3514)

The afterburner module should respect the CoercionConfig, currently it only checks MapperFeature.ALLOW_COERCION_OF_SCALARS:

private void _verifyScalarCoercion(DeserializationContext ctxt, JsonParser parser, String type) throws IOException {
MapperFeature feat = MapperFeature.ALLOW_COERCION_OF_SCALARS;
if (!ctxt.isEnabled(feat)) {
ctxt.reportInputMismatch(getType(),
"Cannot coerce JSON %s value (%s) into %s (enable `%s.%s` to allow)",
parser.currentToken().name(),
parser.readValueAsTree(),
type,
feat.getClass().getSimpleName(),
feat.name());
}
}

Blackbird appears to be impacted as well (at a glance reading the code, I have not validated with a test yet):

private void _verifyScalarCoercion(DeserializationContext ctxt, JsonParser parser, String type) throws IOException {
MapperFeature feat = MapperFeature.ALLOW_COERCION_OF_SCALARS;
if (!ctxt.isEnabled(feat)) {
ctxt.reportInputMismatch(getType(),
"Cannot coerce JSON %s value (%s) into %s (enable `%s.%s` to allow)",
parser.currentToken().name(),
parser.readValueAsTree(),
type,
feat.getClass().getSimpleName(),
feat.name());
}
}

carterkozak added a commit to palantir/conjure-java-runtime that referenced this issue Dec 2, 2020
Reported upstream:
FasterXML/jackson-modules-base#120

The CoercionConfig is fantastic, unfortunately it is not yet
supported by the AfterBurner module. Separately we may consider
moving to the new Blackbird module on java 11 runtimes.

For now I've updated our existing long deserializers for 2.12.0
which has the benefit of rejecting coercion from string to SafeLong.
@cowtowncoder
Copy link
Member

@cowtowncoder cowtowncoder commented Dec 2, 2020

Yes, it is very likely that this also affects Blackbird; and yes, improvements are needed to get behavior in line with core databind.

@carterkozak
Copy link
Author

@carterkozak carterkozak commented Dec 2, 2020

I should be able to contribute something in the next week or so, no rush to review as you take a well deserved break :-)
The 2.12 upgrade has been incredibly smooth so far!

@cowtowncoder
Copy link
Member

@cowtowncoder cowtowncoder commented Dec 2, 2020

@carterkozak cool, glad to hear it has been at least somewhat smooth process! It's good to get a sense of various regressions after release, .1 patch often follows within a month. Many users really only test the .0 and not the rcs anyway. :)

I do have my todo-list on

https://github.com/FasterXML/jackson-future-ideas/wiki/Jackson-Work-in-Progress

and this helps track things that should make it in 2.12.1.

cowtowncoder added a commit that referenced this issue Dec 17, 2020
@cowtowncoder
Copy link
Member

@cowtowncoder cowtowncoder commented Dec 17, 2020

Starting work on this, bigger than I thought but important to get parity.

@carterkozak
Copy link
Author

@carterkozak carterkozak commented Dec 17, 2020

Thanks, I really appreciate it! Sorry I hadn't had a chance to put something together myself.

@cowtowncoder
Copy link
Member

@cowtowncoder cowtowncoder commented Dec 17, 2020

@carterkozak no problem at all, this is quite sizable thing. Also beefing up CoercionConfig testing in databind itself, noticing some gaps.

Adding tests to Afterburner, Blackbird is actually not super difficult as I can mostly copy-paste from databind (although ideally could refactor create reusable "jackson-base-test" jar or something).

@cowtowncoder
Copy link
Member

@cowtowncoder cowtowncoder commented Dec 18, 2020

Ok I think this should work now. Did a sizable detour by first copying quite a bit of handling code from StdDeserializer, before realizing that I can delegate non-default input shapes and keep code not only much tighter but also more compliant (since delegating uses default databind handling in that case).

@carterkozak Would appreciate if you could see if snapshot from 2.12 works better at this point.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants