Implemented isReadable on Oslc XML/RDF Providers #228
Conversation
|
Sorry, for the formatting, I was using the .editorconfig for formatting but it seems it didn't work pretty well :/ |
jhemm2
force-pushed
the
bugfix/#226-OslcXmlProvider_/_OslcRdfXmlProvider_always_returns_is_readable
branch
from
75d0ce0
to
e272199
Compare
jhemm2
changed the title
Not sure how .editorconfig works. But maybe you can just commit the files (or parts of) with the substantial changes? |
Well, the PR contains only the files changed actually. Unfortunately I can't get it done without breaking the formatting.
MessageBodyReaderTest and ResourceWithoutShape
AbstractOslcRdfXmlProvider: Added method isReadable and isCompatible |
There was a problem hiding this comment.
Thank you so much @jhemm2 for the pull request!
I asked Jad and Jim to check on the use of the OslcNotQueryResult that your code may disrupt. I also linked my old PR in the comments.
I think the list of types accepted by the isReadable method and the @Consumes (and identically for the writes) should be the same unless I am missing something.
Finally, I am trying to find some corner cases in the OSLC adaptors code I have because this part was not well tested to check for regressions. For now let's focus with you on the isReadable/@Consumes logic.
Formatting changes are OK, though in the future it would be better to only reformat the code you touch and all the other lines to only wrap code that exceeds the margin and correct whitespace. But we needed to reformat this class long time ago anyway.
Merry Christmas and happy holidays!
We will aim to merge this PR in Lyo 5.0.0.beta1 and may backport it to 4.2.x if we can establish that it doesn't break any existing code.
|
|
||
| protected static boolean isReadable(final Class<?> type, final MediaType actualMediaType, | ||
| final MediaType... requiredMediaTypes) { | ||
| if (type.getAnnotation(OslcResourceShape.class) != null) { |
There was a problem hiding this comment.
@jamsden @jadelkhoury could you please remind me how do we handle OSLC Query responses? AFAIK they are NOT annotated with the OslcResourceShape annotation.
@jhemm2 this change seems correct to me but I am afraid it may be breaking for some legacy code. Let me ask around and dig some old implementations to see if I can test the effect of your change. I think this is my old PR for replacing many RDF providers with one and there I also checked for the OslcNotQueryResult annotation. Please compare the logic in our PRs if you could.
| final MediaType mediaType) { | ||
| if (type.isArray()) { | ||
| return isReadable(type.getComponentType(), mediaType, OslcMediaType.APPLICATION_RDF_XML_TYPE, | ||
| OslcMediaType.APPLICATION_XML_TYPE, OslcMediaType.TEXT_XML_TYPE); |
There was a problem hiding this comment.
I think I am recalling a bit more about this. Doesn't @Consumes protect you from having to do this check? Or do you think it broke with the Wink to Jersey transition? Do you use Jersey, by the way? I would also say the types in this check must match the @Consumes annotation.
There was a problem hiding this comment.
Quite a long time ago so I cannot completely remember ..I think you are right, that the media types should match the @consumes annotation. Then we would be able to read application/rdf+xml only but not application/xml anymore.
I don't know how far it should be restricted but actually it should also be checked to have @OslcResourceShape annotation, shouldn't it?
| import org.eclipse.lyo.oslc4j.provider.jena.test.resources.TestResource; | ||
| import org.junit.Test; | ||
|
|
||
| public class MessageBodyReaderTest { |
There was a problem hiding this comment.
Need to check if we can add a simple @OslcNotQueryResult test here. @jadelkhoury @jamsden do you have some code that you could find that uses that annotation to help @jhemm2 write a test? I think we annotate JAX-RS methods with it IIRC.
| public void testOslcXmlProvider() { | ||
| OslcXmlProvider provider = new OslcXmlProvider(); | ||
|
|
||
| boolean isReadable = provider.isReadable(TestResource.class, null, TestResource.class.getAnnotations(), |
There was a problem hiding this comment.
@jhemm2 I am not sure the 3rd argument is passed correctly:
if the message body is to be converted into a method parameter, this will be the annotations on that parameter returned by Method.getParameterAnnotations
from https://docs.oracle.com/javaee/7/api/javax/ws/rs/ext/MessageBodyReader.html
There was a problem hiding this comment.
Not sure, too as it also states:
annotations an array of the annotations on the declaration of the artifact that will be initialized with the produced instance
What do you think would be correct here?
|
Can one of the admins verify this patch? |
|
ok to test |
|
@jhemm2 we will release Lyo 5.0 without this patch. Please reply to my comments on the PR if we are to target this PR for Lyo 5.1. |
|
Dear @jhemm2, thank you again for your PR! For a number of reasons, we could not merge it right then but your PR brought the problem to our attention and inspired us to make thorough changes in #286 and #288. PR #288 also comes with a set of extensive unit tests. I will thus close your PR. Please check if your issue persists on Thank you for your input and a great effort! |
Description
OslcXmlProvider(s) returned always true for isReadable. This has been changed to verify the type and annotations first.
Without this fix, XML entities have been read by the OslcXmlProvider instead of e.g. an JaxbProvider which has lead to errors.
Checklist
Issues
Closes #226 ;
(use exactly this syntax to link to issues, one issue per statement only!)