Make hasProperty(), hasPropertyAtPath(), samePropertyValuesAs() work for Java Records
#426
Conversation
The current `hasProperty()` matcher can't deal with Java Records, as they are not compatible with JavaBeans specification. In this change, `HasProperty` first tries to do the original thing and then tries to find a method of which name is equivalent to the given property name, if there's no matching property. So for example, if the class has a getter method named `property()`, `hasProperty()` would regard it as the class has a property named `property`. I hope this change might be properly and easily removed when the time comes and Hamcrest starts to support Java 17.
There is a flaw in 8201577, which is that the logic can't distinguish if the found method is a getter. Let's put a simple additional rule: The property must be a getter, so it should return something.
|
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 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. Looking at What do you think? |
|
Thank you for the detailed feedback, @tumbarumba!
To summarize, I'd like to suggest that we focus on Thank you. |
|
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 As to |
|
I got your point, @tumbarumba I agree with you. I'll respond in detail when I get back home. |
|
Thanks for the detailed feed back, @tumbarumba
|
After the debate of pr hamcrest#426, We decided to move all the methods of `MethodUtil` to `PropertyUtil`. There was a confusion of understanding the name of the class `PropertyUtil`. First I thought it was kind of a technical name indicating that `PropertyUtil` deals with java beans property. But actually it wasn't just a technical name. `PropertyUtil` means a tool to find out some properties in the target class. So to say, it is like a `PropertyFindingUtil`. We don't have to change the utility name even if it finds some methods of the target class, not properties of it, as it does what it is meant to do.
and polished its javadoc.
The basic approach is as same as the `HasProperty` in 8201577. `hasProperty(propertyName, valueMatcher)` will recognize properties in java record classes if: 1. There's a method of which name is same to the given property name 2. The found method returns something If it doesn't meet condition 1, the property does not exist. If it doesn't meet condition 2, the property is write-only.
To achieve this goal I needed a different approach from 8201577. `samePropertyValuesAs(expectedBean, ignoredProperties)` will recognize properties in java record classes if: * There's a read accessor method of which name is same to the given property name A read accessor method is a method which acts like a getter for a field. To check if the method is a read accessor method, the algorithm filters the following: 1. The name of it should be identical to the name of the field. 2. It should return something. 3. It should not have any method parameters. In this manner, we can now assure that the collected methods are getters. ## Reference * https://docs.oracle.com/en/java/javase/14/language/records.html
The former implementation `methodDescriptorsFor()` can be replaced to the new method `recordReadAccessorMethodDescriptorsFor()`. This doesn't change current behavior except one, the case for the non-readable property. Actually, there can't be any non-readable property in Java Records by its specification.
djkeh
changed the title
MethodUtil to make hasProperty() work for Java RecordsMethodUtil to make hasProperty(), samePropertyValuesAs() work for Java Records
djkeh
changed the title
MethodUtil to make hasProperty(), samePropertyValuesAs() work for Java RecordshasProperty(), samePropertyValuesAs() work for Java Records
|
It's ready again. Now it works for |
djkeh
changed the title
hasProperty(), samePropertyValuesAs() work for Java RecordshasProperty(), hasPropertyAtPath(), samePropertyValuesAs() work for Java Records
There was a problem hiding this comment.
Thanks for taking the time to do this work, @djkeh. I like how you were able to make use of FeatureDescriptor to consolate the two approaches to finding the property value.
There are a few other tweaks I'd like to see, but I'll merge this as is and we can make them later
Resolves #392
The current
hasProperty()matcher can't deal with Java Records, as they are not compatible with JavaBeans specification.In this change,
HasPropertytries 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 namedproperty.This change is locally tested on JDK 17 with
record.I deliberately created
MethodUtilfor 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
HasPropertyWithValuebased on this change.Reference