Add MethodUtil to make hasProperty() work for Java Records

[djkeh]

Resolves #392

The current hasProperty() matcher can't deal with Java Records, as they are not compatible with JavaBeans specification.
In this change, HasProperty tries the original thing first,
and if there's no matching property, tries to find a method of which name is equivalent to the given property name.
So for example, if the class has a method named property(), hasProperty() would regard it as the class has a property named property.
This change is locally tested on JDK 17 with record .
I deliberately created MethodUtil for the future change. If Hamcrest starts to support Java 17,
the current approach can be polished using modern features such as java.lang.Class.isRecord().

I hope this change might be properly removed when the time comes and Hamcrest starts to support Java 17.
If merged, I will look into the other relevant matchers like HasPropertyWithValue based on this change.

Reference

• https://docs.oracle.com/en/java/javase/17/language/records.html

[tumbarumba]

I've had a closer look. There are a few points I'd like to see addressed.

Firstly, tests looked good and worked fine for the HasPropertyTest usages. One thing that struck me was that there was no tests for HasPropertyWithValue. I was also curious to see how the code worked againsts an actual record. I tried the following example:

import org.junit.jupiter.api.Test;

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.hasProperty;
import static org.hamcrest.core.IsEqual.equalTo;

record Example(String strAttribute, int intAttribute) {}

public class RecordPropertyTest {
    @Test
    public void
    testExampleRecord() {
        Example example = new Example("one", 1);
        assertThat(example, hasProperty("strAttribute"));
        assertThat(example, hasProperty("intAttribute"));
        assertThat(example, hasProperty("strAttribute", equalTo("one")));
        assertThat(example, hasProperty("intAttribute", equalTo(1)));
    }
}

The above code worked fine for the first 2 assertions, but failed for the last 2 that tried to match the the value. Looking at the changes, this is expected. I do think that we should offer a complete solution when we merge this PR, though. Having said that, I think it's understandable to check the approach first. HasPropertyWithValue is much more complicated, and likely much harder to adapt to your approach. I didn't check, but I expect that SamePropertyValueAs would have the same problem.

Looking at MethodUtil, I was struck by how similar it was to PropertyUtil. I'm wondering if we should move the functionality into PropertyUtil. Note that the existing public API could be used anywhere (including outside projects), and so we can't change that, but adding extra methods is quite doable.

What do you think?

[djkeh]

Thank you for the detailed feedback, @tumbarumba!

1. As I mentioned in the second paragraph, this pr is not about HasPropertyWithValue. The base idea dealing with record in Java 8 needed to be confirmed first. It would be meaningless to work with HasPropertyWithValue and its hasProperty(propertyName, valueMatcher) matcher if the idea has a critical flaw to proceed. That's why I separated HasPropertyWithValue from this pr. Please note that HasProperty Matcher doesn't work with Java Records #392 is also reporting hasProperty(propertyName) only. Of course I was aware of HasPropertyWithValue, and I'm strongly eager to work on it in the next pr. Making 2 prs for each HasProperty and HasPropertyWithValue was my first intention, but if you'd like to include HasPropertyWithValue in this pr, I'm happy to do it.
2. In the commit message 8201577 and the javadoc, I'm explaining the meaning of MethodUtil. As you pointed out the similarity to PropertyUtil, this utility class is not for its unique functionality, but for the temporary process to find properties from java records in java 8 environment. This is kind of special circumstance. If Hamcrest goes up to version 4 and starts to support java 17, there could be a much better approach to solve this problem, and the logics in MethodUtil could be useless. When that happens, I expect the pr at that moment will be much simpler as we simply remove MethodUtil without changing PropertyUtil. I basically agree with your idea - the MethodDescriptor logic in MethodUtil could be placed in PropertyUtil as well. In that case PropertyUtil would not only be dealing with PropertyDescriptor but also dealing with MethodDescriptor, so the name of the class could also be changed. This idea will make changes to the PropertyUtil somehow. That means more change, and more influence on the current system. I suggest we don't have to take that risk if the changes are expected to be rolled back in the near future. In brief, I deliberately made MethodUtil detached from PropertyUtil so it would never affect the current functionalities related to PropertyUtil, and so it could be easily removed in the near future.
3. SamePropertyValueAs was out of my consideration in this pr as well as HasPropertyWithValue. However, I agree that all those three classes are relevant to each other. At this moment I'd like to suggest we focus on issue HasProperty Matcher doesn't work with Java Records #392, checking idea that makes java records work in java 8 environment, then make another prs to deal with the rest. I understand it might look not so complete and natural in an aspect. After merging this pr, only hasProperty(propertyName) will work for java records and hasProperty(propertyName, valueMatcher), samePropertyValuesAs(expectedBean, ignoredProperties...) will not. But looking at the other way, those three matchers are not working for java records already. The users won't feel that weird if those matchers start to support java records one by one, leaving the rest not working in the middle.

To summarize, I'd like to suggest that we focus on HasProperty first, separating MethodUtil as a temporary solution, and remove it when the java 17 comes into Hamcrest. However, if you still think the relevant matchers have to serve complete functionality at the same time in a single pr, I still support that idea as well, so please don't mind and let me know. I'll do it in this pr.

Thank you.

[tumbarumba]

Thanks for your comments @djkeh.

I'm not keen to merge the PR as is. I think many people will find it confusing as to why one of the Matcher.hasProperty methods works, and the other doesn't. I know that there are different backing implementations, but I worry that a lot of users won't try to understand the difference. They'll just see this as a bug.

As to MethodUtil, I wouldn't try to organise the code around an eventual migration JDK 17. Hamcrest is very conservative with backwards compatibility, and I don't see the project dropping compatibility for JDK 8 and 11 until well after they are EOL. Instead, try to focus on making the code simple and readable as possible, with clear abstrations. To my mind, both PropertyUtil and MethodUtil are trying to answer the question "does this object I'm checking have a propery like thing in it?", but with different ways of checking for the property like thing

[djkeh]

I got your point, @tumbarumba

I agree with you. I'll respond in detail when I get back home.

[djkeh]

Thanks for the detailed feed back, @tumbarumba
I totally understand your point, and it seems like the original idea to deal with java records is fairly acceptable for you.
Then I'll do the following:

1. I'll Implement the features dealing with java records for HasPropertyWithValue and SamePropertyValuesAs and complete this pr.
2. I'll move the methods in MethodUtils to PropertyUtils.