You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cxf.apache.org by GitBox <gi...@apache.org> on 2021/04/25 19:11:34 UTC
[GitHub] [cxf] reta opened a new pull request #779: CXF-8516: Fixing jaxrs.spec.provider.jaxbcontext readWriteProviderTest
reta opened a new pull request #779:
URL: https://github.com/apache/cxf/pull/779
While troubleshooting the TCK test case in question discovered that sorting of message body readers and writers is not symmetrical:
- readers are sorted first by media type
- writers are sorted first by classes comparison which **does not look right**
Fixed the message body writers sorting by starting with media types, the same as for message body readers.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] andymc12 commented on pull request #779: CXF-8516: Fixing jaxrs.spec.provider.jaxbcontext readWriteProviderTest
Posted by GitBox <gi...@apache.org>.
andymc12 commented on pull request #779:
URL: https://github.com/apache/cxf/pull/779#issuecomment-827062423
@reta based on my reading of the spec, I think the original behavior is correct. (I'll reference Jakarta REST 3.0 spec, since it is publicly available - but it should be the same as 2.1, only with different package spaces):
For MBRs, the first thing checked should be the media type, followed by Java parameter type, then the `isReadable` method.
https://jakarta.ee/specifications/restful-ws/3.0/jakarta-restful-ws-spec-3.0.html#message_body_reader
For MBWs, the first thing checked should be the Java return type, then the media type, then the `isWritable` method.
https://jakarta.ee/specifications/restful-ws/3.0/jakarta-restful-ws-spec-3.0.html#message_body_writer
Ultimately though, I don't think the order of these operations matters so long as there ends up with a sorted list of providers that satisfy all three requirements (can convert to/from the same object type, compatible media type, and `isReadable` / `isWritable` method returns true).
In Open Liberty, we made a few tweaks to this process that you might want to incorporate into your changes:
(1) We changed the `compareClasses` method slightly so that sub-classes are handled correctly:
https://github.com/OpenLiberty/open-liberty/blob/eca9d0bebc1a380e4f78ad1e849735a28aa87754/dev/com.ibm.ws.jaxrs.2.0.common/src/org/apache/cxf/jaxrs/provider/ProviderFactory.java#L1456
(2) We cache the sorted list of MediaTypes for the MBRs/MBWs to avoid looking them up and sorting them on each request, ex:
https://github.com/OpenLiberty/open-liberty/blob/eca9d0bebc1a380e4f78ad1e849735a28aa87754/dev/com.ibm.ws.org.apache.cxf.cxf.rt.frontend.jaxrs.3.2/src/org/apache/cxf/jaxrs/provider/ProviderFactory.java#L1170
Hope this helps!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] dblevins commented on pull request #779: CXF-8516: Fixing jaxrs.spec.provider.jaxbcontext readWriteProviderTest
Posted by GitBox <gi...@apache.org>.
dblevins commented on pull request #779:
URL: https://github.com/apache/cxf/pull/779#issuecomment-827858604
I take back "very easy" and replace it with "fixable" :)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on a change in pull request #779: CXF-8516: Fixing jaxrs.spec.provider.jaxbcontext readWriteProviderTest
Posted by GitBox <gi...@apache.org>.
reta commented on a change in pull request #779:
URL: https://github.com/apache/cxf/pull/779#discussion_r620753707
##########
File path: rt/frontend/jaxrs/src/main/java/org/apache/cxf/jaxrs/provider/ProviderFactory.java
##########
@@ -1133,8 +1132,13 @@ protected static int compareClasses(Class<?> expectedCls, Object o1, Object o2)
if (realClass1.isAssignableFrom(realClass2)) {
// subclass should go first
return 1;
+ } else if (realClass2.isAssignableFrom(realClass1)) {
Review comment:
Also aligned with Jersey (JAX-RS RI) in this regards
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on pull request #779: CXF-8516: Fixing jaxrs.spec.provider.jaxbcontext readWriteProviderTest
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #779:
URL: https://github.com/apache/cxf/pull/779#issuecomment-826461004
@andymc12 mind please taking a quick look? thanks a lot!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] dblevins edited a comment on pull request #779: CXF-8516: Fixing jaxrs.spec.provider.jaxbcontext readWriteProviderTest
Posted by GitBox <gi...@apache.org>.
dblevins edited a comment on pull request #779:
URL: https://github.com/apache/cxf/pull/779#issuecomment-827838166
I think we should hit the merge button on this.
I've found other issues with our sorting and am in the process of writing very simple test cases and creating fixes. I don't want to get too far in that process, however, as it's just going to create merge conflicts with this PR.
As mentioned, I'm already working on a fix, but there's an example of a test case I have that fails with the current sorting:
```
@Test
public void mostSpecificClassTypeWins() {
class Shape {
}
class Square extends Shape {
}
class ShapeReader extends Reader<Shape> {
}
class SquareReader extends Reader<Square> {
}
class ObjectReader extends Reader<Object> {
}
final List<ProviderInfo<MessageBodyReader<?>>> providers = Providers.readers()
.system(new ShapeReader())
.system(new SquareReader())
.system(new ObjectReader())
.get();
assertOrder(providers, "SquareReader\n"
+ "ShapeReader\n"
+ "ObjectReader");
}
```
With the current code, we're actually sorting in the reverse preferred order: `ObjectReader`, `ShapeReader`, `SquareReader`
Very easy fix, just don't want to get too far with pending PRs on the sorting.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] dblevins commented on pull request #779: CXF-8516: Fixing jaxrs.spec.provider.jaxbcontext readWriteProviderTest
Posted by GitBox <gi...@apache.org>.
dblevins commented on pull request #779:
URL: https://github.com/apache/cxf/pull/779#issuecomment-827286656
Confirming what @andymc12 said. With that previous change in the precedence of class comparisons, that causes issues in these tests:
```
com.sun.ts.tests.jaxrs.ee.rs.core.configurable.JAXRSClient registerClassWriterContractsTest_from_standalone
com.sun.ts.tests.jaxrs.ee.rs.core.configurable.JAXRSClient registerObjectWriterContractsTest_from_standalone
com.sun.ts.tests.jaxrs.ee.rs.core.configurable.JAXRSClient registerClassWriterContractsInMapTest_from_standalone
com.sun.ts.tests.jaxrs.ee.rs.core.configurable.JAXRSClient registerObjectWriterContractsInMapTest_from_standalone
```
I'll try out the most recent isAssignableFrom tweak shortly.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta merged pull request #779: CXF-8516: Fixing jaxrs.spec.provider.jaxbcontext readWriteProviderTest
Posted by GitBox <gi...@apache.org>.
reta merged pull request #779:
URL: https://github.com/apache/cxf/pull/779
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on pull request #779: CXF-8516: Fixing jaxrs.spec.provider.jaxbcontext readWriteProviderTest
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #779:
URL: https://github.com/apache/cxf/pull/779#issuecomment-827210345
Thanks a lot @andymc12 , you are absolutely correct, the object (method's return type), should be consulted first, followed by the media type(s). I will revert and redo the change to follow the specification closely, 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] reta commented on pull request #779: CXF-8516: Fixing jaxrs.spec.provider.jaxbcontext readWriteProviderTest
Posted by GitBox <gi...@apache.org>.
reta commented on pull request #779:
URL: https://github.com/apache/cxf/pull/779#issuecomment-826461004
@andymc12 mind please taking a quick look? thanks a lot!
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [cxf] dblevins commented on pull request #779: CXF-8516: Fixing jaxrs.spec.provider.jaxbcontext readWriteProviderTest
Posted by GitBox <gi...@apache.org>.
dblevins commented on pull request #779:
URL: https://github.com/apache/cxf/pull/779#issuecomment-827838166
I think we should hit the merge button on this.
I've found other issues with our sorting and am in the process of writing very simple test cases and creating fixes. I don't want to get too far in that process, however, as it's just going to create merge conflicts with this PR.
As mentioned, I'm already working on a fix, but there's an example of a test case I have that fails with the current sorting:
```
@Test
public void mostSpecificClassTypeWins() {
class Shape {
}
class Square extends Shape {
}
class ShapeReader extends Reader<Shape> {
}
class SquareReader extends Reader<Square> {
}
class ObjectReader extends Reader<Object> {
}
final List<ProviderInfo<MessageBodyReader<?>>> providers = Providers.readers()
.system(new ShapeReader())
.system(new SquareReader())
.system(new ObjectReader())
.get();
assertOrder(providers, "SquareReader\n"
+ "ShapeReader\n"
+ "ObjectReader");
}
```
With the current code, we're actually sorting in the reverse preferred order: `ObjectReader`, `ShapeReader`, `ObjectReader`
Very easy fix, just don't want to get too far with pending PRs on the sorting.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org