Hello @struberg

Checking against an interface in isJavaInternalClass() is not going to work.

I updated EqualsBuilderReflectJreImplementationTest in git master.

Gary, this has already long been resolved in f20639a and ec4150b

I've even merged your changes (and removed that new --add-opens again...) from main to this very branch.

This branch clearly improves the situation with standard jpms setup for a lot of common cases. Of course, this does not yet fix ALL the problems (albeit it's a good start). But if people come across another InaccessibleObjectException then they can still --add-opens as needed. So the current version is clearly an improvement. If you agree, then we should merge it into main instead of yet more parallel work in different branches...

Hello @struberg
Checking against an interface in isJavaInternalClass() is not going to work.
I updated EqualsBuilderReflectJreImplementationTest in git master.

Gary, this has already long been resolved in f20639a and ec4150b

I've even merged your changes (and removed that new --add-opens again...) from main to this very branch.

This branch clearly improves the situation with standard jpms setup for a lot of common cases. Of course, this does not yet fix ALL the problems (albeit it's a good start). But if people come across another InaccessibleObjectException then they can still --add-opens as needed. So the current version is clearly an improvement. If you agree, then we should merge it into main instead of yet more parallel work in different branches...

Hello @struberg

The issue is not that this PR is a wants to start improving something, the issue is that this PR breaks existing behavior.

You cannot check for interfaces to short-circuit logic in Reflection.isJavaInternalClass(). This will break existing applications.

The branch is not up to date with master. This fails:

git clone https://github.com/struberg/commons-lang.git 
cd commons-lang
git checkout fb_jpms
git remote add upstream https://github.com/apache/commons-lang.git
git pull upstream master
mvn clean test -Dtest=EqualsBuilderReflectJreImplementationTest

You cannot check for interfaces to short-circuit logic in Reflection.isJavaInternalClass(). This will break existing applications.

Which of those is an interface?
https://github.com/struberg/commons-lang/blob/ec4150bc13cfd2705e5d672f47706930af578c39/src/main/java/org/apache/commons/lang3/builder/Reflection.java#L62

The thing you are talking about IS ALREADY RESOLVED since yesterday.

And one other thing: commons-lang3 ReflectionToString ReflectionEquals etc was BROKEN on anything Java9++ - period!
The build is ONLY green because you just disabled all the modularity - which is something not everyone can do in real projects!

My spare time to work on this stuff is very limited - as is yours I bet. So please use our both time productively. As I said: not everything will fully work from the beginning. You just added java.time.Period, fine, good catch. But why on earth don't we just take that PR and commit it and then improve it TOGETHER? I'm not willing to do more empty rounds getting sticks thrown at me at will. I really appreciate the review feedback as it indeed did highly improve the code. But now is the point to decide whether the direction is right or not - and then improve things from here TOGETHER.

Btw, I'm looking for another method name for isJavaInternalClass(). It should rather express that the given clazz cannot be introspected via reflection. Maybe something like isNonIntrospectibleClass()?

Further down the road we need some way to tell whether a class is open or not. And all that in a way which also works on Java8 still...

You cannot check for interfaces to short-circuit logic in Reflection.isJavaInternalClass(). This will break existing applications.

Which of those is an interface? https://github.com/struberg/commons-lang/blob/ec4150bc13cfd2705e5d672f47706930af578c39/src/main/java/org/apache/commons/lang3/builder/Reflection.java#L62

The thing you are talking about IS ALREADY RESOLVED since yesterday.

I guess you could be bothered to look it up? https://docs.oracle.com/javase/8/docs/api/java/time/temporal/Temporal.html

I guess you could be bothered to look it up

Indeed, had in the back of my mind that Temporal is an abstract class.
Latest version uses reflection to figure that a class is not accessible. This works, but maybe we should try a mixture of the old approach + the new one?

Thought about having a static Map<Class, Boolean> KNOWN_INACCESSIBLE_CLASSES which contains the concrete classes like LocalDate, Boolean, Character, Integer, etc.
And then in a second step we do the reflection check. But as this might also have unexpected results (people might forget that they closed the jars) we should probably log it out?

So far, I'm still against this change.

These features are designed to use reflection, and this PR tries to turn some of that off.

I agree. All the reflectionX methods have a warning:

Fields can be private, thus AccessibleObject.setAccessible is used to bypass normal access control checks. This will fail under a security manager unless the appropriate permissions are set.

The warning might require an update, but it seems clear to me that it also applies to the JPMS access checks. So it is up to caller to ensure that the class is open to org.apache.commons.lang3. If the caller uses testRecursive, he must also ensure that the appropriate permissions are set for all the field types.

I agree. All the reflectionX methods have a warning:

Fields can be private, thus AccessibleObject.setAccessible is used to bypass normal access control checks.

why is this disussion again NOT happening on any ASF list but here where it does not get reflected to any ASF list afaict?

This discussion is mirrored on the apache list here:
https://lists.apache.org/thread/gwljc3ylz3cjvbg9m7sv9dtj139obprz

further comments over at the commons dev list.

Note that this PR is worse than I thought because we already have a feature to bypass reflection! Use EqualsBuilder.setBypassReflectionClasses(List<Class<?>>).

Are you sure that you really understand that code?

If you would setBypassReflectionClasses(ArrayList.class) or HashSet.class or HashMap.class etc, then you would totally break the EqualsBuilder. Because this would also stop looking down into the equality of the elements contained in that List/Set/Map.

Funnily you yourself did add hand-tinkered bypasses for those classes in selected places - just not for every use case. So you yourself added code to NOT use reflection for some of the cases.

So this rather sounds like a strawman argument to me.

I agree. All the reflectionX methods have a warning:

Well this is way more subtly weird right now.

Imagine the following code:

 Map<String, String> a = new HashMap<>(20);
 a.put("A", "A");
 Map<String, String> b = new HashMap<>();
 b.put("A", "A");

Here we have the case that a.equals(b) returns true. Because that's the contract of Maps!
But when you use new EqualsBuilder().setTestRecursive(true).append(a, b).isEquals() then you'll get false.
That's because by giving a different initial array size, we'll get a different internal (!) structure (bigger internal arrays, etc).

But that means that right now the EqualsBuilder (even in Java8) does not even adheres to the contract of the Map interface. But those internal details should not matter! Even if I would not have set an initial size - what if I add 30 elements to one of the 2 maps, but then remove everything but the "A" entry again? It will be equals according to the Map contract, but not for the EqualsBuilder. Looks rather broken to me.

One might argue that that's exactly how all the reflection stuff in commons-lang was designed from the beginning.
Otoh there are already a few places where Maps, Collections, etc get a special treatment. So why not also here?

Note that this PR is worse than I thought because we already have a feature to bypass reflection! Use EqualsBuilder.setBypassReflectionClasses(List<Class<?>>).

This is just plain wrong. You cannot use setBypassReflectionClasses for e.g. Map without loosing the reflexive comparison of it's entries.