FasterXML/java-classmate

TypeResolver.resolve(Type type, Type... typeParameters)

cowwoc opened this issue · 23 comments

cowwoc commented

This will allow users to specify multiple type parameters, as well it will allow them to easily mix simple (URL) and compound type (List<URL>) parameters with ease. Finally, by having ResolvedType extend java.lang.reflect.Type we will be able to tunnel ResolvedType through Jersey's Client.get(Type) method.

See FasterXML/jackson-core#41 (comment) for more information.

cowwoc commented

This issue is related to FasterXML/jackson-core#44

Hey. It looks like we got lucky! They might fix GenericType in JAX-RS 2.0 after all. See http://java.net/jira/browse/JAX_RS_SPEC-313

In light of this, is it possible for you to resolve this issue as soon as possible so we can point them at a ready-to-integrate fix? Do you feel that ClassMate is mature enough to integrate into such a project? Does it have any major outstanding issues?

cowtowncoder commented

It would be great if this could be improved -- one of my long-time wishes.

ClassMate has been stable for longest time, and perhaps I should just call it 1.0 at this point. I am not aware of any bugs it has. The only concern I have is whether JAX-RS specification itself can use it. But if they can use interface type(s) needed to define methods to implement (from JAX-RS perspective), I'm sure ClassMate could be changed to implement that (or simple ClassMate wrapper project that does that).

As to Jackson: I can make this happen for 2.2, but as with all API changes, it can't go in 2.1. But I don't think JAX-RS specification itself cares too much. And if necessary, it is possible to make sure 2.2 gets released early enough to work with Jersey, and Jersey release candidates can use 2.2.0-SNAPSHOT in the meantime.

One thing I am still unsure about, however, is just this: while we can make anything implement java.lang.reflect.Type, it does not solve the bigger problem: how to avoid close coupling between libraries? Jackson does not know ClassMate; nor do I want to add a dependency from the core. Same for ClassMate (in reverse direction).
My plan for using ClassMate from Jackson was to use Maven Shade plug-in, which basically creates a private copy; and its types would not be usable outside of Jackson (or vice versa).

Above might not be a problem, if all that is needed is ability to pass JavaType from client side, and then Jackson can typecast is just fine. I am just trying to figure out how ClassMate would fit in here, as external library.

cowtowncoder commented

One more thing: I am not on any JAX-RS spec list. But perhaps some details of this could be discussed on Jersey mailing lists? (I did see a note on voting for the issue, which is good start).

cowwoc commented

Help me understand, what is the problem with Jackson (or Jersey) depending on ClassMate? How is this any different than Jersey depending on a private copy of GenericType which (I believe) was copied from Guice?

Re: Jersey mailing list, you're welcome to join the conversation there or point me to an alternate post you create on the JAX-RS mailing list and I will join along.

cowtowncoder commented

I do not want or plan to add more external dependencies to Jackson. Jersey may not have any problems with this but JAX-RS API will not be able to rely on a 3rd party library.

As to private copy: it is not a problem for internal usage; but may be problem with external API. It is not a problem if the other library can have a dependency to GenericType. It would be a problem if no such dependency exists: for example, Jackson JAX-RS provider does not have dependency to any Jersey component (except for unit tests that need an implementation).

cowwoc commented

JAX-RS can depend on ClassMate, Jackson's JAX-RS provider will depend on JAX-RS which will implicitly depend on ClassMate. Everyone is happy, no? This mean Jackson doesn't have a dependency on JAX-RS or ClassMate, only its JAX-RS-specific provider does.

How do you currently provide compatibility between Jersey's GenericType and Jackson's JavaType? You're going to need to integrate these two somehow in JAX-RS 2.0 aren't you? Like I said, I think your JAX-RS provider is fine. The question is how the JAX-RS provider and core modules interact.

cowtowncoder commented

Yes, that would work. Bit of glue code would be needed to convert from ResolvedType to JavaType, but that should be easy.

As to GenericType, there is no handling for it. Is that something introduced in JAX-RS 2.0? JAX-RS provider definitely should use it; but so far only java.lang.reflect.Type has been exposed, as far as I know.
But if GenericType is similar to Jackson's TypeReference (uses "super type token" pattern), it should be very simple to support

cowtowncoder commented

On GenericType... maybe you meant GenericEntity? If so, it's all done by Jersey, by just unwrapping it to get Type from inside, passing that. Which is what Jackson does with TypeReference too FWIW.

cowwoc commented

Weird. Both GenericEntity and GenericType exist in Jersey 1.x. See http://jersey.java.net/nonav/apidocs/1.5/jersey/com/sun/jersey/api/client/GenericType.html -- I wonder why we need both.

Did we resolve all outstanding questions to do with integrating ClassMate into JAX-RS 2.0? Or is there anything else to discuss?

cowtowncoder commented

I don't know why there are those two. But since GenericType is a client package, should it matter wrt JAX-RS JSON provider?

I think I know what to do wrt ClassMate and Jackson JavaType -- make both implement java.lang.reflect.Type, and recognize this wrapping in factory methods (which only need to do upcast).
And after this is done, Jackson JAX-RS providers could just pass Type as is, in case of JavaType; or convert into JavaType if it gets ClassMate ResolvedType.

But on the other hand, I could start with JavaType only handling; and add ClassMate if and when Jersey starts passing it. The reason for this is just that I want to keep dependencies minimal, so add it when it as actually needed.

cowwoc commented
  1. Yes, the JSON provider should support both client and server APIs since it'll get used in both context.
  2. If you're going to add ClassMate support why not replace JavaType outright in favor of ResolvedType?
cowtowncoder commented

Oh. I did not know providers are used there -- never used Jersey client actually. Good to know.
I assume JAX-RS 2.0 will specify client API, and if that contains GenericType, that'd work. I know that 1.x did not include client support so impls prototyped with variety of approaches.

As to JavaType: I would love to switch it, but the problem is that JavaType is exposed all throughout the API, and the only way to use ResolvedType is to either make it implement JavaType (not practical) or convert between the two. I don't think either is practical unfortunately -- but would love to be proven wrong! -- which is why I would first prototype by trying to rewrite TypeFactory to use ResolvedType, then convert to JavaType.

The reason I would love to use ClassMate is that there are couple of type resolution bugs that are hard to solve without significant rewrite of type resolution. And ClassMate was written to do things right, based on all I learnt with Jackson.

cowtowncoder commented

One more note: above relates to Jackson databind -- using ClassMate with JAX-RS providers would be different matter. And it might be good idea to try indeed converting its API to use ClassMate.

cowwoc commented
  1. I'm not sure I understand. I've used Jackson from Jersey Client 1.x, with GenericType no less. This feature isn't specific to Jersey 2.x.
  2. I was thinking more along the lines of removing JavaType from the API and replacing it with ResolvedType in Jackson 3.0 or the next major release. You could add converters into version 2.x as you mentioned and @deprecate JavaType.
cowtowncoder commented

What I mean is that Jackson JAX-RS provider has no handling for this type. My guess is that Jersey client simply takes the java.lang.reflect.Type, and passes that to Jackson. This is fine, since I think that is the whole point of the type (ability to "wrap" generic type, to retain it despite type erasure).

I did understand the suggestion on wholesale replacement: and you are right in that this would be possible for 3.0.
But even there the question is whether benefits exceed the cost. It is a big change, and users do not generally like such changes.

Timeline for 3.0 would also be quite a bit out in the future. Users do not exactly like major updates, and there must be some significant benefits to reap to do those. So question, then, is whether to wait out for that, or try to work in making use of actual functionality that ClassMate uses.

At the same time, if ClassMate will get adopted widely, one could definitely argue that there are benefits from Jackson also adopting it. At this point ClassMate is being used, but not exposed through APIs of other frameworks or libraries, as far as I know.

cowwoc commented

One thing is for sure: we need to make sure ClassMate is ready for integration into JAX-RS. To that end, I encourage you to update the Javadoc of TypeResolver and ResolvedType to make sure that all members are well documented and the class description contained a brief example of the typical usage.

cowtowncoder commented

Implemented this: FasterXML/jackson-databind#116 so JavaType implements Type for 2.2; and TypeFactory "unwraps" it as expected.

Related question of modifying TypeFactory methods is trickier one, since it would like be source-compatible but binary incompatible, worst kind of change (because it can be easily re-compiled to work; but trying to use different version without compilation fails). And while overload might work, it could cause compilation failure ("ambiguous method").
Maybe it could be made to work, but I'll defer that question.

cowtowncoder commented

Will create separate issue here for "implements Type".

cowtowncoder commented

Implemented as requested; also made GenericType implement java.lang.reflect.Type; and combined various methods in TypeResolver. The only backwards incompatibility introduced was (I hope) switching of argument order to get:

TypeResolver.resolveType(TypeBindings, Type)

(instead of vice versa) which is needed to avoid/reduce problems with overloading. Hopefully does not cause too many issues with existing users.

cowtowncoder commented

Also: releasing 0.8.0 (produced some intermediate versions during updates), should flow down to Maven Central in due time.

cowwoc commented
  1. I re-read the Javadoc for TypeBindings multiple times and I still don't understand what it's for. It doesn't seem to hold any state, so why do you need to pass it in as an argument?
  2. Why do you need to reverse the parameter order? TypeBindings does not implement `Typeso how could it conflict with theType...`` vararg?
  3. The new TypeResolver is much cleaner. Good job! Question: Why do we need arrayType? Couldn't users simply invoke TypeResolver.resolve(int[].class)?
  4. I believe you could improve usability by moving TypeResolver.resolveSubType() and TypeResolver.isSelfReference() to the ResolvedType class. The factory class should only be used to construct the initial ResolvedType.
  5. The ResolvedType Javadoc looks extremely "busy". Consider converting some of the protected methods to package-private (I believe this will remove them from the Javadoc and hide them in IDEs). As usual, document all methods and consider adding examples to some of the descriptions. For example, it's not obvious what http://cowtowncoder.github.com/java-classmate/javadoc/0.8.0/com/fasterxml/classmate/ResolvedType.html#typeParametersFor(java.lang.Class) means. If the documentation provided an example it would probably be easier to understand.
cowtowncoder commented
  1. TypeBindings is needed for resolving bound type variables, and it does indeed have state (passed via 'create' factory method). It mostly makes sense for recursive resolution, and less commonly from main method. And it is extensively used when resolving members (ResolvedType itself keeps track of it). But I don't think method(s) in TypeResolver that take it are commonly used outside of ClassMate code.
  2. For method calls that pass nulls (unit tests had some), and perhaps for auto-completion. While not absolutely required (i.e. it could have been retained), I think change makes sense at this point before finalizing 1.0. Especially since this alternative method is rarely used from outside package itself.
  3. This is for programmatically constructing arrays of types; usually resolved types. Like Map<String,Integer>[] or such. Arrays are odd types; they are not generic, but have actual non-type-erased element type information.
  4. I was wondering about this one; but if so, ResolvedType would need to contain a back-reference to TypeResolver. This is possible, but did not seem quite right. At the same time, I agree in that current call patterns is not optimal either. I am open to change.
  5. I'm sure documentation could be improved. I don't have too much time to spend here, but will keep this in mind. Note that there's a bit of usage documentation on README of the project itself, contributed by @blangel (who also much improved unit tests).
cowwoc commented
  1. Can you provide a short code sniplet demonstrating how this is meant to be used?
  2. I would actually disallow passing in null and throw NullPointerException. Null is not the same as the lack of type parameters, nor an array of length 0.
  3. So this method converts an existing type to an array of that type? Like List<String> into List<String>[]? If so, I believe it should be moved to ResolvedType.
  4. Instead of storing a back-reference to the factory, you could pass the object cache forward. I assume that's the reason you wanted the back-reference in the first place...