HV-534 Removing shading plugin

[hferentschik]

I made the changes for removing the shading plugin and updated the assembly and docs as well (including another minor change in how we package everything).

To merge or not to merge is the question ;-)

[gunnarmorling]

One thing I really like about HV is that we have that less dependencies. From a user's POV this is a good thing(TM) as it reduces hassle with transitive dependencies.

Therefore I think it would be best if we could get rid of the JType dependency altogether. I had a short look, we're using only 4 methods of it with ~100 LOC. If we implemented this ourselves/copied it we would have one dependency less and also wouldn't need the shading plug-in. WDYT?

[hferentschik]

One thing I really like about HV is that we have that less dependencies. From a user's POV this is a good thing(TM) as > it reduces hassle with transitive dependencies.

I agree. Having only a few dependencies is a nice thing. jtype itself does not have any other dependencies which is good and made us use the shading plugin in the first place.

Therefore I think it would be best if we could get rid of the JType dependency altogether. I had a short look, we're using > only 4 methods of it with ~100 LOC. If we implemented this ourselves/copied it we would have one dependency less
and also wouldn't need the shading plug-in. WDYT?

Are you volunteering here? You are right that we only use 4 methods, I think they are:

TypeUtils.isArray
TypeUtils.getComponentType
TypeUtils.getErasedType
TypeUtils.isAssignable

I am not so sure about the 100 LOC though. Sure enough the called methods look short, but they in turn call other methods (recursively) and I have the feeling you would have to pull in more than expected of the existing code. Also, once you start in pulling other classes you start wondering whether it makes sense to only try to get in only the functionality needed or whether you should internalize the whole library. And if you do the latter you start wondering why internalizing a whole library if you could just have it as a dependency. This is very much the same discussion we had before when we decided for the shading plugin. It seemed to offer the best tradeoff.

Another point is that if we implement the missing functionality ourselves in a few hundred lines we also have to think about tests and of course correctness of the code. This type erasure stuff is not trivial (at least not imo). I was quite happy that I did not have to implement this myself.

On a side note, there is now another library I like a lot which does the same a jtype - https://github.com/cowtowncoder/java-classmate. In the end they all work around several shortcomings in the Java reflection API.

[hferentschik]

That said, seems in bval they have done exactly that (taken what s needed from jtype) - https://svn.apache.org/repos/asf/incubator/bval/tags/0.3-incubating/bval-jsr303/src/main/java/org/apache/bval/jsr303/util/TypeUtils.java

Funny enough this version has exactly the same bug we fixed two days ago with List<? extends Foo> and on the current trunk TypeUtils from commons-lang is used.

@gunnarmorling You seem to be able to pull out the core of jtype into a few hundred lines after all ;-)

[gunnarmorling]

@hferentschik Give me Ruby and I'll do it in 1 line ;-)

All joking aside, you're of course right about the 4 methods using other private methods in turn. I hadn't considered that, doh.

I just tried to copy the 4 required methods and everything which they invoke, this is altogether ~800 lines (TypeUtils and some stuff from other classes). Do you think it would be feasible to copy that into HV?

If we decide for that approach we also might consider copying the entire lib as it isn't that much more altogether and would probably make handling simpler for us. In either way we should relocate the package IMO in order to avoid conflicts with a user-provided JType dependency.

Do you have a feeling how much it would be using the other library you're mentioning?

Altogether it's really as discussed in HV-90: making things simpler/better for users or for us. Personally I'd slightly prefer the copying approach to spare users an additional dependency, although 800 lines is quite a lot. I think I'm fine with either way :)

@kevinpollet do you have any thoughts on this?

[hferentschik]

I just tried to copy the 4 required methods and everything which they invoke, this is altogether ~800 lines (TypeUtils and > some stuff from other classes). Do you think it would be feasible to copy that into HV?

Possible sure :-) As the link I posted above shows, we probably can condense this all down to a single TypeUtils class witch a few hundred lines.

If we decide for that approach we also might consider copying the entire lib as it isn't that much more altogether and
would probably make handling simpler for us

If we talk about including the whole library, I think we are better of using a normal dependency. Again, it is the same discussion all over again.

To throw something new into the discussion, one could imagine creating a TypeUtils interface with the required methods we need in HV. We could then make the whole thing configurable. You could use jtype or maybe even classmate (or a built-in implementation we provide). Configuration could be via a property we pass. Is this overkill? Maybe :-)

Do you have a feeling how much it would be using the other library you're mentioning?

Not exactly sure what you mean. If you ask how much of the classmate API will be used by HV, I would say that classmate offers even more than jtype.
On the other hand classmate will be used in Hibernate Core together with the new metamodel. In this case it would maybe nice to use the same library between the two (here comes the interface idea into play again)

[gunnarmorling]

In order to bring the discussion to a conclusion I tried to sum up all alternatives with their respective pros and cons. Feel free to change/add where you feel like:

1 Use JType as normal dependency

Pro: Simple handling, no shade plugin
Pro: It's the standard Maven way of doing things
Con: Forces a new transitive dependency onto users, potentially conflicting with other user provided versions
Con: Shipping with a < 1.0 dependency might look dubious?

2 Keep the shade plug-in

Pro: No new dependency for users, no conflicts with other JType versions possible added by users as dependency
Pro: Relatively easy to handle for us
Con: Users can't override the JType version with a newer (compatible) one
Con: Causes trouble with the JBoss EAP version? (I've not yet completely understood what that means)
Con: Complicates debugging as per HV-551

3 Pull JType (or another library) into HV

Pro: No dependency trouble for users
Con: Some effort for us (have to get sources and relocate them in order to avoid conflicts). Repeated effort, whenever a new version of JType comes out which we want to upgrade to (though this happens rarely)

3a: Pull in only the required parts

Pro: Less code to pull in
Pro: Proven to work by BVAL
Con: More efforts for us (have to rip out the things we need)
Con: Possible testing efforts for us (if we're changing the pulled in code)

3b: Pull in complete library

Pro: Less effort than 3a
Pro: Reduced chances to break anything compared to 3a
Con: More code pulled in than with 3a (though JType is ~15 classes only altogether)
Con: Pulls in unnecessary code into HV

4 Implement the required stuff ourselves

Pro: No dependency troubles for users
Con: NIH syndrome
Con: Implementation and testing efforts for us, and this is hard to write code

5 Introduce an interface encapsulating the reflection API

Pro: Flexible, allows to change implementations
Pro: Allows to align with Hibernate Core
Con: Increases complexity. Configuration efforts?
Con: Might make life harder for users, as they have to provide/configure the dependency implementing the interface
Con: Still needs one of the other options to provide an implementation?

Based on this list, I'd personally exclude #2 (too much magic) and #4 (NIH). The implications of #5 are not completely clear to me, how would that exactly look like?

Leaves me with #1 vs. #3, where I'd favor #3 as it puts burdens more on us than on users, which is generally a good thing for a library IMO. Ultimately I'm going for #3a (we don't have to ship code which we don't need). Where possible, we should stick to removing classes instead of changing them in order to reduce risks for bugs.

Hardy, I hope that helps in making a decision, you're the boss :)

[hferentschik]

@gunnarmorling Thanks for the summary. The choice for me is also between #1 and #3a. Looking at TypeUtils everything except TypeUtils.isAssignable is trivial to pull it (we could make this part of ReflectionHelper).

Do you have your experiments for #3a somewhere on GitHub?

I am going to merge this into master, because we all agree that we need to get rid of the shade plugin. If we can get a cleaned up version of TypeUtils.isAssignable with some basic tests I am happy to down this route.

[gunnarmorling]

Do you have your experiments for #3a somewhere on GitHub?

I've pushed that now to https://github.com/gunnarmorling/hibernate-validator/tree/HV-534. Altogether it's about 900 lines (I've pulled in no tests from JType, though). For now I've just moved everything into org.hibernate.validator.com.googlecode.jtype and removed all apparently unused methods/classes. Moving the functionality into ReflectionHelper might be an alternative, too.

[hferentschik]

Thanks @gunnarmorling I have a look at it asap.