ValueInjector break from 2.8.x to 2.9.x

[kinigitbyday]

The ValueInjector constructor signature changed with 76381c5#diff-dbcd29e987f27d95f964a674963a9066R24. It seems like the old one could have been deprecated instead of the hard break.

I'm using Finatra for my API layer, and it handles a lot of request object de/serialization using Jackson. I'd like to upgrade to 2.9.2, but currently can't because Finatra attempts to create an instance of ValueInjector using the old constructor (https://github.com/twitter/finatra/blob/develop/jackson/src/main/scala/com/twitter/finatra/json/internal/caseclass/utils/FieldInjection.scala#L50). It seemed better to open the issue here because I don't think Finatra should have to lock users to 2.9+.

[cowtowncoder]

Since ValueInjector is considered an internal class (suggested by impl package -- although unfortunately not emphasized in javadocs), such changes are made between minor versions when needed. We try to provide one minor version of deprecation usually (and try not to make incompatible changes unless necessary in generaly). I do not remember why this was not done here (sometimes it is not possible to do so in a way that keeps both old and new methods working). It may have seemed that this should not be something used outside of core databinding functionality.

So... it is unfortunate that such a breakage occurs, but part of this is that ideally this class would not be used as an extension point. Internal classes will be more fragile over time; public API compatibility offers stronger guarantees for compatibility.

I am open to adding something in 2.9.3 that would allow compatibility between versions, if possible.
Alternatively I wonder if public mechanisms for supporting injection (like via Guice module -- in jackson-modules-base under guice) should not be sufficient: that is, is there real need to extend this class.

[kinigitbyday]

Appreciate the quick response. For what it's worth, I also opened an issue against Finatra, because it's their custom deserialization logic that depends on a ValueInjector and I also wonder if they could work around it.

[cowtowncoder]

@kinigitbyday Much appreciated. I hope we can figure out something as these issue can certainly be nasty compatibility problems leading to dependency(-version)-hell.

[cowtowncoder]

Added deprecated (for 2.9.x) constructor back, to hopefully reduce compatibility problems.
Will be removed from 3.0.

[mosesn]

@cowtowncoder I'm trying to upgrade finatra to 2.9.x and I'm getting bitten by this–we relied on being able to pass contextAnnotations. I can work around it by overriding getContextAnnotation, but I'm curious if there's a better way to solve this problem than relying on an internal class. We provide our own annotations, like QueryParam.

[cowtowncoder]

@mosesn There probably should be. Problem is that there is no real extension point for value injection, unlike many other features that may be used via annotations (by default), or by other mechanisms that extension modules can register.

But if you could explain bit more on functionality Finatra is implementing using this mechanism, maybe I could think of alternate existing mechanism(s) that could be usable?

I am open for improvements (additions) both for 2.10 (which has to be compatible with 2.x in general, and hence will still have deprecated constructor), and 3.x (which will not have that constructor, but should have something to support existing uses I think).

[mosesn]

I think we use it with our non-json annotations. In finatra you can annotate a case class with @QueryParam to indicate that you're going to hydrate that field with the content of a query parameter, like /admin/whatever?foo=bar. I think this allows us to hydrate a case class partially with stuff parsed from the json, and partially the stuff parsed from the query parameter.

https://github.com/twitter/finatra/blob/324e77b14ce07914e112bb878326725f187335d9/jackson/src/main/scala/com/twitter/finatra/json/internal/caseclass/utils/FieldInjection.scala#L49

[cacoco]

@cowtowncoder Moses is correct, we're basically trying to provide functionality to parse the JSON body of an incoming request and potentially merge it with other sources into a resultant object.

More information on the field annotations here.

We want to be able to take the content body an incoming request, parse it as JSON and include fields injected from the 5 other supported sources into the resultant object. We use the ValueInjector to be to create a beanProperty over this incoming source to pass to the DeserializationContext #findInjectableValue method (as noted in the code Moses linked).

To be honest, this is some of the oldest code of the framework and is probably due to be re-evaluated -- we just have not yet done that.

[cowtowncoder]

I think I understand why access to configuration is needed, and I think Jackson side unfortunately limits information passed too much. Usually AnnotationIntrospector method findInjectableValue() would have means to handle configuration, but unfortunately that is not true either for contextual values (not available for access) or for actual result value (which is simple value object and not handler).

I think what would make more sense in design is probably allowing registration of handlers/overrides for factory method (BeanDeserializerFactory.addInjectable()). And this probably is the place where all annotations, contextual configuration should be used so that ValueInjector need not worry about it.

I will file a new issue, linking to this one.