You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cxf.apache.org by "jgoodyear (via GitHub)" <gi...@apache.org> on 2024/04/02 20:15:39 UTC
[PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
jgoodyear opened a new pull request, #1784:
URL: https://github.com/apache/cxf/pull/1784
JAXRS Bean introspection utility Beanspector relies on Class.getMethods natural order.
For most JVMs the Beanspector Tests will pass, however IBM Java does not return methods in the same ordering. Note: Class.getMethods does not provide a prescribed ordering of methods.
When CXF 4.0.x main branch is built, we'll observe Apache CXF JAX-RS Extensions: Search fails test cases.
We can improve this behaviour by detecting when IBM Java is in use, and having IBM process declared methods first, then process remaining methods. This will make IBM behave more like other JVMs.
Branch Tested on:
Windows 11 Pro (Intel) with IBM Semeru 17.
MacOS 14.4.1 (AArch64) with IBM Semeru 17, Bellsoft 17, and Zulu 17.
CentOS Stream 9 (POWER) with IBM Semeru 17, RedHat OpenJDK 17, and Temurin 17.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
Posted by "jgoodyear (via GitHub)" <gi...@apache.org>.
jgoodyear commented on PR #1784:
URL: https://github.com/apache/cxf/pull/1784#issuecomment-2037803661
The Ubuntu failure i could find in the logs above was for a bind exception on the Transports tests.
Can the build be re-ran?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
Posted by "jgoodyear (via GitHub)" <gi...@apache.org>.
jgoodyear commented on PR #1784:
URL: https://github.com/apache/cxf/pull/1784#issuecomment-2040684505
Doing declared methods first works for me - i had originally flagged for IBM Java just to limit any potential impacts a deeper change could have to existing applications. As to implementation strategy, I'm happy with which ever approach you prefer. Main goal is to have IBM Java be able to build & test CXF main (4.0.x) cleanly without breaking anyone else.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1784:
URL: https://github.com/apache/cxf/pull/1784#discussion_r1554575151
##########
rt/rs/extensions/search/src/main/java/org/apache/cxf/jaxrs/ext/search/Beanspector.java:
##########
@@ -58,49 +60,63 @@ public Beanspector(T tobj) {
init();
}
- @SuppressWarnings("unchecked")
- private void init() {
- if (tclass == null) {
- tclass = (Class<T>)tobj.getClass();
- }
- for (Method m : tclass.getMethods()) {
- if (isGetter(m)) {
- String pname = getPropertyName(m);
- if (!getters.containsKey(pname)) {
- getters.put(getPropertyName(m), m);
- } else {
- // Prefer the getter that has the most specialized class as a return type
- Method met = getters.get(pname);
- if (met.getReturnType().isAssignableFrom(m.getReturnType())) {
- getters.put(pname, m);
- }
- }
- } else if (isSetter(m)) {
- String pname = getPropertyName(m);
- if (!setters.containsKey(pname)) {
- setters.put(getPropertyName(m), m);
- } else {
- // Prefer the setter that has the most specialized class as a parameter
- Method met = setters.get(pname);
- if (met.getParameterTypes()[0].isAssignableFrom(m.getParameterTypes()[0])) {
- setters.put(pname, m);
- }
- }
- }
- }
- // check type equality for getter-setter pairs
- Set<String> pairs = new HashSet<>(getters.keySet());
- pairs.retainAll(setters.keySet());
- for (String accessor : pairs) {
- Class<?> getterClass = getters.get(accessor).getReturnType();
- Class<?> setterClass = setters.get(accessor).getParameterTypes()[0];
- if (!setterClass.isAssignableFrom(getterClass)) {
- throw new IllegalArgumentException(String
- .format("Accessor '%s' type mismatch, getter type is %s while setter type is %s",
- accessor, getterClass.getName(), setterClass.getName()));
- }
- }
- }
+ @SuppressWarnings("unchecked")
+ private void init() {
+ if (tclass == null) {
+ tclass = (Class<T>)tobj.getClass();
+ }
+
+ List<Method> methods = Arrays.asList(tclass.getMethods());
+ Collections.sort(methods, (m1, m2) -> {
+ if (m1.getDeclaringClass().equals(m2.getDeclaringClass())) {
+ return 0;
+ } else if (m1.getDeclaringClass().equals(tclass)) {
+ return -1;
+ } else {
+ return 1;
+ }
+ });
+
+ for (Method m : methods) {
+ if (isGetter(m)) {
+ String pname = getPropertyName(m);
+ if (!getters.containsKey(pname)) {
+ getters.put(getPropertyName(m), m);
+ } else {
+ // Prefer the getter that has the most specialized class as a return type
+ Method met = getters.get(pname);
+ if (met.getReturnType().isAssignableFrom(m.getReturnType())) {
+ getters.put(pname, m);
+ }
+ }
+ } else if (isSetter(m)) {
+ String pname = getPropertyName(m);
+ System.out.println(m);
Review Comment:
Debug leftovers?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1784:
URL: https://github.com/apache/cxf/pull/1784#discussion_r1550523564
##########
rt/rs/extensions/search/src/main/java/org/apache/cxf/jaxrs/ext/search/Beanspector.java:
##########
@@ -58,49 +58,62 @@ public Beanspector(T tobj) {
init();
}
- @SuppressWarnings("unchecked")
- private void init() {
- if (tclass == null) {
- tclass = (Class<T>)tobj.getClass();
- }
- for (Method m : tclass.getMethods()) {
- if (isGetter(m)) {
- String pname = getPropertyName(m);
- if (!getters.containsKey(pname)) {
- getters.put(getPropertyName(m), m);
- } else {
- // Prefer the getter that has the most specialized class as a return type
- Method met = getters.get(pname);
- if (met.getReturnType().isAssignableFrom(m.getReturnType())) {
- getters.put(pname, m);
- }
- }
- } else if (isSetter(m)) {
- String pname = getPropertyName(m);
- if (!setters.containsKey(pname)) {
- setters.put(getPropertyName(m), m);
- } else {
- // Prefer the setter that has the most specialized class as a parameter
- Method met = setters.get(pname);
- if (met.getParameterTypes()[0].isAssignableFrom(m.getParameterTypes()[0])) {
- setters.put(pname, m);
- }
- }
- }
- }
- // check type equality for getter-setter pairs
- Set<String> pairs = new HashSet<>(getters.keySet());
- pairs.retainAll(setters.keySet());
- for (String accessor : pairs) {
- Class<?> getterClass = getters.get(accessor).getReturnType();
- Class<?> setterClass = setters.get(accessor).getParameterTypes()[0];
- if (!setterClass.isAssignableFrom(getterClass)) {
- throw new IllegalArgumentException(String
- .format("Accessor '%s' type mismatch, getter type is %s while setter type is %s",
- accessor, getterClass.getName(), setterClass.getName()));
- }
- }
- }
+ @SuppressWarnings("unchecked")
+ private void init() {
+ if (tclass == null) {
+ tclass = (Class<T>)tobj.getClass();
+ }
+
+ //Class.getMethods does not provide an ordering.
Review Comment:
@jgoodyear I believe the dependency on ordering is an issue that we should fix, it is should not be JDK dependent (neither version or vendor), could we make the implementation resilient to any ordering dependencies? Thank you.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on PR #1784:
URL: https://github.com/apache/cxf/pull/1784#issuecomment-2040643978
> If you run this locally the BeanspectorTests pass with the exception of testMismatchedOverriddenBeans which fails the expected IllegalArg exception since we do not get tripped up by ordering.
OK, I think I figured possible solution, curious what do you think. The idea is very simple - make sure the declared methods go first (so the methods that are declared by the class itself are checked first). Before processing, the methods are ordered by declaring class:
```
List<Method> methods = Arrays.asList(tclass.getMethods());
Collections.sort(methods, (m1, m2) -> {
if (m1.getDeclaringClass().equals(m2.getDeclaringClass())) {
return 0;
} else if (m1.getDeclaringClass().equals(tclass)) {
return -1;
} else {
return 1;
}
});
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
Posted by "jgoodyear (via GitHub)" <gi...@apache.org>.
jgoodyear commented on PR #1784:
URL: https://github.com/apache/cxf/pull/1784#issuecomment-2040260938
I made a separate branch with a more generic processing of Class methods, and pair matching:
https://github.com/jgoodyear/cxf/blob/CXF-8996-investigation/rt/rs/extensions/search/src/main/java/org/apache/cxf/jaxrs/ext/search/Beanspector.java#L66
If you run this locally the BeanspectorTests pass with the exception of testMismatchedOverriddenBeans which fails the expected IllegalArg exception since we do not get tripped up by ordering.
Any other approach ideas?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta merged PR #1784:
URL: https://github.com/apache/cxf/pull/1784
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
Posted by "jgoodyear (via GitHub)" <gi...@apache.org>.
jgoodyear commented on PR #1784:
URL: https://github.com/apache/cxf/pull/1784#issuecomment-2037248749
I am in the process of testing the updated PR branch across various platforms/JVMs.
Current branch tested on MacOS 14.4.1 with IBM Semeru 17 and Bellsoft 17.
Will update comments as i expand test results.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1784:
URL: https://github.com/apache/cxf/pull/1784#discussion_r1552552637
##########
rt/rs/extensions/search/src/main/java/org/apache/cxf/jaxrs/ext/search/Beanspector.java:
##########
@@ -58,49 +58,59 @@ public Beanspector(T tobj) {
init();
}
- @SuppressWarnings("unchecked")
- private void init() {
- if (tclass == null) {
- tclass = (Class<T>)tobj.getClass();
- }
- for (Method m : tclass.getMethods()) {
- if (isGetter(m)) {
- String pname = getPropertyName(m);
- if (!getters.containsKey(pname)) {
- getters.put(getPropertyName(m), m);
- } else {
- // Prefer the getter that has the most specialized class as a return type
- Method met = getters.get(pname);
- if (met.getReturnType().isAssignableFrom(m.getReturnType())) {
- getters.put(pname, m);
- }
- }
- } else if (isSetter(m)) {
- String pname = getPropertyName(m);
- if (!setters.containsKey(pname)) {
- setters.put(getPropertyName(m), m);
- } else {
- // Prefer the setter that has the most specialized class as a parameter
- Method met = setters.get(pname);
- if (met.getParameterTypes()[0].isAssignableFrom(m.getParameterTypes()[0])) {
- setters.put(pname, m);
- }
- }
- }
- }
- // check type equality for getter-setter pairs
- Set<String> pairs = new HashSet<>(getters.keySet());
- pairs.retainAll(setters.keySet());
- for (String accessor : pairs) {
- Class<?> getterClass = getters.get(accessor).getReturnType();
- Class<?> setterClass = setters.get(accessor).getParameterTypes()[0];
- if (!setterClass.isAssignableFrom(getterClass)) {
- throw new IllegalArgumentException(String
- .format("Accessor '%s' type mismatch, getter type is %s while setter type is %s",
- accessor, getterClass.getName(), setterClass.getName()));
- }
- }
- }
+ @SuppressWarnings("unchecked")
+ private void init() {
+ if (tclass == null) {
+ tclass = (Class<T>)tobj.getClass();
+ }
+
+ // Class.getMethods - The elements in the returned array are not sorted and are not in any particular order.
+ // To provide some ordering, we can process DeclaredMethods first, then pick up the remaining.
+ processMethods(tclass.getDeclaredMethods());
Review Comment:
> The Beanspector code picks up the first instance of methods on a class, so for most JVMs the code gets the STRING SimpleBean, IBM gets setSimpleBean(org.apache.cxf.jaxrs.ext.search.BeanspectorTest$SimpleBean) , which results in an Illegal Argument being thrown even though the correct setter/getter pair does exist.
So we basically again are victims of the ordering, I would prefer to have make selection "smarter" by determining pairs of getters and setters based on arg / return type matches (otherwise we are not really solving the problem that you've identified). Happy to help if needed.
##########
rt/rs/extensions/search/src/main/java/org/apache/cxf/jaxrs/ext/search/Beanspector.java:
##########
@@ -58,49 +58,59 @@ public Beanspector(T tobj) {
init();
}
- @SuppressWarnings("unchecked")
- private void init() {
- if (tclass == null) {
- tclass = (Class<T>)tobj.getClass();
- }
- for (Method m : tclass.getMethods()) {
- if (isGetter(m)) {
- String pname = getPropertyName(m);
- if (!getters.containsKey(pname)) {
- getters.put(getPropertyName(m), m);
- } else {
- // Prefer the getter that has the most specialized class as a return type
- Method met = getters.get(pname);
- if (met.getReturnType().isAssignableFrom(m.getReturnType())) {
- getters.put(pname, m);
- }
- }
- } else if (isSetter(m)) {
- String pname = getPropertyName(m);
- if (!setters.containsKey(pname)) {
- setters.put(getPropertyName(m), m);
- } else {
- // Prefer the setter that has the most specialized class as a parameter
- Method met = setters.get(pname);
- if (met.getParameterTypes()[0].isAssignableFrom(m.getParameterTypes()[0])) {
- setters.put(pname, m);
- }
- }
- }
- }
- // check type equality for getter-setter pairs
- Set<String> pairs = new HashSet<>(getters.keySet());
- pairs.retainAll(setters.keySet());
- for (String accessor : pairs) {
- Class<?> getterClass = getters.get(accessor).getReturnType();
- Class<?> setterClass = setters.get(accessor).getParameterTypes()[0];
- if (!setterClass.isAssignableFrom(getterClass)) {
- throw new IllegalArgumentException(String
- .format("Accessor '%s' type mismatch, getter type is %s while setter type is %s",
- accessor, getterClass.getName(), setterClass.getName()));
- }
- }
- }
+ @SuppressWarnings("unchecked")
+ private void init() {
+ if (tclass == null) {
+ tclass = (Class<T>)tobj.getClass();
+ }
+
+ // Class.getMethods - The elements in the returned array are not sorted and are not in any particular order.
+ // To provide some ordering, we can process DeclaredMethods first, then pick up the remaining.
+ processMethods(tclass.getDeclaredMethods());
Review Comment:
> The Beanspector code picks up the first instance of methods on a class, so for most JVMs the code gets the STRING SimpleBean, IBM gets setSimpleBean(org.apache.cxf.jaxrs.ext.search.BeanspectorTest$SimpleBean) , which results in an Illegal Argument being thrown even though the correct setter/getter pair does exist.
So we basically again are victims of the ordering, I would prefer to make selection "smarter" by determining pairs of getters and setters based on arg / return type matches (otherwise we are not really solving the problem that you've identified). Happy to help if needed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on PR #1784:
URL: https://github.com/apache/cxf/pull/1784#issuecomment-2040693363
> . Main goal is to have IBM Java be able to build & test CXF main (4.0.x) cleanly without breaking anyone else.
Thanks @jgoodyear , may I ask you to make this simple change in this pull request at the beginning of `init()`, the iteration could go over `methods` list after.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
Posted by "jgoodyear (via GitHub)" <gi...@apache.org>.
jgoodyear commented on PR #1784:
URL: https://github.com/apache/cxf/pull/1784#issuecomment-2037500175
I have tested the updated PR with the below platforms/JVMs.
MacOS 14.4.1 (AArch64) IBM Semeru 17, BellSoft 17, Zulu 17, Temurin 17, and Amazon Corretto 17 .
Windows (Intel x64) IBM Semeru 17 and Azul Zulu 17.
CentOS Stream 9 (PPC64LE) IBM Semeru 17, RedHat OpenJDK 17, and Temurin 17.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
Posted by "jgoodyear (via GitHub)" <gi...@apache.org>.
jgoodyear commented on code in PR #1784:
URL: https://github.com/apache/cxf/pull/1784#discussion_r1552527419
##########
rt/rs/extensions/search/src/main/java/org/apache/cxf/jaxrs/ext/search/Beanspector.java:
##########
@@ -58,49 +58,59 @@ public Beanspector(T tobj) {
init();
}
- @SuppressWarnings("unchecked")
- private void init() {
- if (tclass == null) {
- tclass = (Class<T>)tobj.getClass();
- }
- for (Method m : tclass.getMethods()) {
- if (isGetter(m)) {
- String pname = getPropertyName(m);
- if (!getters.containsKey(pname)) {
- getters.put(getPropertyName(m), m);
- } else {
- // Prefer the getter that has the most specialized class as a return type
- Method met = getters.get(pname);
- if (met.getReturnType().isAssignableFrom(m.getReturnType())) {
- getters.put(pname, m);
- }
- }
- } else if (isSetter(m)) {
- String pname = getPropertyName(m);
- if (!setters.containsKey(pname)) {
- setters.put(getPropertyName(m), m);
- } else {
- // Prefer the setter that has the most specialized class as a parameter
- Method met = setters.get(pname);
- if (met.getParameterTypes()[0].isAssignableFrom(m.getParameterTypes()[0])) {
- setters.put(pname, m);
- }
- }
- }
- }
- // check type equality for getter-setter pairs
- Set<String> pairs = new HashSet<>(getters.keySet());
- pairs.retainAll(setters.keySet());
- for (String accessor : pairs) {
- Class<?> getterClass = getters.get(accessor).getReturnType();
- Class<?> setterClass = setters.get(accessor).getParameterTypes()[0];
- if (!setterClass.isAssignableFrom(getterClass)) {
- throw new IllegalArgumentException(String
- .format("Accessor '%s' type mismatch, getter type is %s while setter type is %s",
- accessor, getterClass.getName(), setterClass.getName()));
- }
- }
- }
+ @SuppressWarnings("unchecked")
+ private void init() {
+ if (tclass == null) {
+ tclass = (Class<T>)tobj.getClass();
+ }
+
+ // Class.getMethods - The elements in the returned array are not sorted and are not in any particular order.
+ // To provide some ordering, we can process DeclaredMethods first, then pick up the remaining.
+ processMethods(tclass.getDeclaredMethods());
Review Comment:
Here is a sample of the getMethods return values for Azul Zulu and IBM Semeru:
`Azul:
public org.apache.cxf.jaxrs.ext.search.BeanspectorTest$SimpleBean org.apache.cxf.jaxrs.ext.search.BeanspectorTest$MismatchedOverriddenBean.getSimpleBean()
public org.apache.cxf.jaxrs.ext.search.BeanspectorTest$OverriddenSimpleBean org.apache.cxf.jaxrs.ext.search.BeanspectorTest$MismatchedOverriddenBean.getSimpleBean()
public void org.apache.cxf.jaxrs.ext.search.BeanspectorTest$MismatchedOverriddenBean.setSimpleBean(java.lang.String)
public void org.apache.cxf.jaxrs.ext.search.BeanspectorTest$AnotherBean.setSimpleBean(org.apache.cxf.jaxrs.ext.search.BeanspectorTest$SimpleBean)
public final void java.lang.Object.wait(long,int) throws java.lang.InterruptedException
public final void java.lang.Object.wait() throws java.lang.InterruptedException
public final native void java.lang.Object.wait(long) throws java.lang.InterruptedException
public boolean java.lang.Object.equals(java.lang.Object)
public java.lang.String java.lang.Object.toString()
public native int java.lang.Object.hashCode()
public final native java.lang.Class java.lang.Object.getClass()
public final native void java.lang.Object.notify()
public final native void java.lang.Object.notifyAll()
IBM:
public final native java.lang.Class java.lang.Object.getClass()
public final void java.lang.Object.wait() throws java.lang.InterruptedException
public final void java.lang.Object.wait(long) throws java.lang.InterruptedException
public final void java.lang.Object.wait(long,int) throws java.lang.InterruptedException
public final native void java.lang.Object.notifyAll()
public void org.apache.cxf.jaxrs.ext.search.BeanspectorTest$AnotherBean.setSimpleBean(org.apache.cxf.jaxrs.ext.search.BeanspectorTest$SimpleBean)
public void org.apache.cxf.jaxrs.ext.search.BeanspectorTest$MismatchedOverriddenBean.setSimpleBean(java.lang.String)
public final native void java.lang.Object.notify()
public int java.lang.Object.hashCode()
public boolean java.lang.Object.equals(java.lang.Object)
public java.lang.String java.lang.Object.toString()
public org.apache.cxf.jaxrs.ext.search.BeanspectorTest$SimpleBean org.apache.cxf.jaxrs.ext.search.BeanspectorTest$MismatchedOverriddenBean.getSimpleBean()
public org.apache.cxf.jaxrs.ext.search.BeanspectorTest$OverriddenSimpleBean org.apache.cxf.jaxrs.ext.search.BeanspectorTest$MismatchedOverriddenBean.getSimpleBean()`
You'll notice that the declared methods are listed last for IBM - other JVMs have ordering the same or vastly similar to the Azul Zulu sample.
Now, why does this matter?
The Beanspector code picks up the first instance of methods on a class, so for most JVMs the code gets the STRING SimpleBean, IBM gets setSimpleBean(org.apache.cxf.jaxrs.ext.search.BeanspectorTest$SimpleBean) , which results in an Illegal Argument being thrown even though the correct setter/getter pair does exist.
When we force the declared methods to be upfront in the return set, then they appear to find the correct pairings. I originally set a flag for IBM behavoir to protect the ordering from affecting any other JVM. This approach puts all JVMs on the same ordering sematic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on PR #1784:
URL: https://github.com/apache/cxf/pull/1784#issuecomment-2040251828
Thanks for working on that @jgoodyear
> I'm beginning to hit a fundamental problem; when we detect, regardless of ordering, getter/setter pairs with correct signatures, we end up finding matches on IBM and the other JVMs, hence failing the testMismatchedOverriddenBeans test case (it expects due to ordering to not have a pair).
That's seems to be incorrect test, right?
> Should that be the correct behavoir for BeanSpector?
I think it should be a correct behavior for BeanSpector (the fact it worked just because of the ordering seems to be pure luck).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
Posted by "jgoodyear (via GitHub)" <gi...@apache.org>.
jgoodyear commented on code in PR #1784:
URL: https://github.com/apache/cxf/pull/1784#discussion_r1552562225
##########
rt/rs/extensions/search/src/main/java/org/apache/cxf/jaxrs/ext/search/Beanspector.java:
##########
@@ -58,49 +58,59 @@ public Beanspector(T tobj) {
init();
}
- @SuppressWarnings("unchecked")
- private void init() {
- if (tclass == null) {
- tclass = (Class<T>)tobj.getClass();
- }
- for (Method m : tclass.getMethods()) {
- if (isGetter(m)) {
- String pname = getPropertyName(m);
- if (!getters.containsKey(pname)) {
- getters.put(getPropertyName(m), m);
- } else {
- // Prefer the getter that has the most specialized class as a return type
- Method met = getters.get(pname);
- if (met.getReturnType().isAssignableFrom(m.getReturnType())) {
- getters.put(pname, m);
- }
- }
- } else if (isSetter(m)) {
- String pname = getPropertyName(m);
- if (!setters.containsKey(pname)) {
- setters.put(getPropertyName(m), m);
- } else {
- // Prefer the setter that has the most specialized class as a parameter
- Method met = setters.get(pname);
- if (met.getParameterTypes()[0].isAssignableFrom(m.getParameterTypes()[0])) {
- setters.put(pname, m);
- }
- }
- }
- }
- // check type equality for getter-setter pairs
- Set<String> pairs = new HashSet<>(getters.keySet());
- pairs.retainAll(setters.keySet());
- for (String accessor : pairs) {
- Class<?> getterClass = getters.get(accessor).getReturnType();
- Class<?> setterClass = setters.get(accessor).getParameterTypes()[0];
- if (!setterClass.isAssignableFrom(getterClass)) {
- throw new IllegalArgumentException(String
- .format("Accessor '%s' type mismatch, getter type is %s while setter type is %s",
- accessor, getterClass.getName(), setterClass.getName()));
- }
- }
- }
+ @SuppressWarnings("unchecked")
+ private void init() {
+ if (tclass == null) {
+ tclass = (Class<T>)tobj.getClass();
+ }
+
+ // Class.getMethods - The elements in the returned array are not sorted and are not in any particular order.
+ // To provide some ordering, we can process DeclaredMethods first, then pick up the remaining.
+ processMethods(tclass.getDeclaredMethods());
Review Comment:
I'll take a deeper look into the matching tomorrow - late hour here now. Thank you for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1784:
URL: https://github.com/apache/cxf/pull/1784#discussion_r1552650024
##########
rt/rs/extensions/search/src/main/java/org/apache/cxf/jaxrs/ext/search/Beanspector.java:
##########
@@ -58,49 +58,59 @@ public Beanspector(T tobj) {
init();
}
- @SuppressWarnings("unchecked")
- private void init() {
- if (tclass == null) {
- tclass = (Class<T>)tobj.getClass();
- }
- for (Method m : tclass.getMethods()) {
- if (isGetter(m)) {
- String pname = getPropertyName(m);
- if (!getters.containsKey(pname)) {
- getters.put(getPropertyName(m), m);
- } else {
- // Prefer the getter that has the most specialized class as a return type
- Method met = getters.get(pname);
- if (met.getReturnType().isAssignableFrom(m.getReturnType())) {
- getters.put(pname, m);
- }
- }
- } else if (isSetter(m)) {
- String pname = getPropertyName(m);
- if (!setters.containsKey(pname)) {
- setters.put(getPropertyName(m), m);
- } else {
- // Prefer the setter that has the most specialized class as a parameter
- Method met = setters.get(pname);
- if (met.getParameterTypes()[0].isAssignableFrom(m.getParameterTypes()[0])) {
- setters.put(pname, m);
- }
- }
- }
- }
- // check type equality for getter-setter pairs
- Set<String> pairs = new HashSet<>(getters.keySet());
- pairs.retainAll(setters.keySet());
- for (String accessor : pairs) {
- Class<?> getterClass = getters.get(accessor).getReturnType();
- Class<?> setterClass = setters.get(accessor).getParameterTypes()[0];
- if (!setterClass.isAssignableFrom(getterClass)) {
- throw new IllegalArgumentException(String
- .format("Accessor '%s' type mismatch, getter type is %s while setter type is %s",
- accessor, getterClass.getName(), setterClass.getName()));
- }
- }
- }
+ @SuppressWarnings("unchecked")
+ private void init() {
+ if (tclass == null) {
+ tclass = (Class<T>)tobj.getClass();
+ }
+
+ // Class.getMethods - The elements in the returned array are not sorted and are not in any particular order.
+ // To provide some ordering, we can process DeclaredMethods first, then pick up the remaining.
+ processMethods(tclass.getDeclaredMethods());
Review Comment:
> I'll take a deeper look into the matching tomorrow - late hour here now. Thank you for the review.
Thank you very much
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
Posted by "jgoodyear (via GitHub)" <gi...@apache.org>.
jgoodyear commented on PR #1784:
URL: https://github.com/apache/cxf/pull/1784#issuecomment-2040206237
I've been diving deep into a more generic methodology.
I'm beginning to hit a fundamental problem; when we detect, regardless of ordering, getter/setter pairs with correct signatures, we end up finding matches on IBM and the other JVMs, hence failing the testMismatchedOverriddenBeans test case (it expects due to ordering to not have a pair).
The check pairs routine is predicate on the ordering being a specifically lined up in order to not throw the illegal argument exception.
This leads to the question - if we ignore ordering, then the check routine will not throw an exception in our testMismatchedOverriddenBeans test case. We can throw an exception if NO pairing is found to be possible. Should that be the correct behavoir for BeanSpector? Otherwise, we're back to having to decide an ordering of methods from Class.getMethods for the behavoir to be 'correct'.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
Posted by "jgoodyear (via GitHub)" <gi...@apache.org>.
jgoodyear commented on PR #1784:
URL: https://github.com/apache/cxf/pull/1784#issuecomment-2040771868
Updated the CXF-8996 PR.
Tested with IBM Semeru 17 and Eclipse Adoptium (Temurin) 17.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] [CXF-8996] JAXRS Bean introspection utility Beanspector improvement [cxf]
Posted by "reta (via GitHub)" <gi...@apache.org>.
reta commented on code in PR #1784:
URL: https://github.com/apache/cxf/pull/1784#discussion_r1552520184
##########
rt/rs/extensions/search/src/main/java/org/apache/cxf/jaxrs/ext/search/Beanspector.java:
##########
@@ -58,49 +58,59 @@ public Beanspector(T tobj) {
init();
}
- @SuppressWarnings("unchecked")
- private void init() {
- if (tclass == null) {
- tclass = (Class<T>)tobj.getClass();
- }
- for (Method m : tclass.getMethods()) {
- if (isGetter(m)) {
- String pname = getPropertyName(m);
- if (!getters.containsKey(pname)) {
- getters.put(getPropertyName(m), m);
- } else {
- // Prefer the getter that has the most specialized class as a return type
- Method met = getters.get(pname);
- if (met.getReturnType().isAssignableFrom(m.getReturnType())) {
- getters.put(pname, m);
- }
- }
- } else if (isSetter(m)) {
- String pname = getPropertyName(m);
- if (!setters.containsKey(pname)) {
- setters.put(getPropertyName(m), m);
- } else {
- // Prefer the setter that has the most specialized class as a parameter
- Method met = setters.get(pname);
- if (met.getParameterTypes()[0].isAssignableFrom(m.getParameterTypes()[0])) {
- setters.put(pname, m);
- }
- }
- }
- }
- // check type equality for getter-setter pairs
- Set<String> pairs = new HashSet<>(getters.keySet());
- pairs.retainAll(setters.keySet());
- for (String accessor : pairs) {
- Class<?> getterClass = getters.get(accessor).getReturnType();
- Class<?> setterClass = setters.get(accessor).getParameterTypes()[0];
- if (!setterClass.isAssignableFrom(getterClass)) {
- throw new IllegalArgumentException(String
- .format("Accessor '%s' type mismatch, getter type is %s while setter type is %s",
- accessor, getterClass.getName(), setterClass.getName()));
- }
- }
- }
+ @SuppressWarnings("unchecked")
+ private void init() {
+ if (tclass == null) {
+ tclass = (Class<T>)tobj.getClass();
+ }
+
+ // Class.getMethods - The elements in the returned array are not sorted and are not in any particular order.
+ // To provide some ordering, we can process DeclaredMethods first, then pick up the remaining.
+ processMethods(tclass.getDeclaredMethods());
Review Comment:
The call to `declaredMethods` is unclear, could you please explain why it is needed? The `getMethods` should cover it all (as per javadoc):
> Returns an array containing Method objects reflecting all the public methods of the class or interface represented by this Class object, including those declared by the class or interface and those inherited from superclasses and superinterfaces.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@cxf.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org