[RESTEASY-2845] ResteasyWebTarget.proxy(Class) problem with questionmark in @Path with Regex #2754
Conversation
|
Hi, there seems to be a problem with CI dependencies. I'm not aware of any changes in dependencies in my code, is that an error I should fix? |
petrberan
force-pushed
the
RESTEASY-2845
branch
from
b3eb9f7
to
cb3bf1d
Compare
| Annotation[] pathParams = new Annotation[method.getParameterTypes().length]; | ||
| HashMap<String, Object> pathParamsMap = new HashMap<>(); | ||
|
|
||
| for (int i = 0; i < pathParams.length; i++) |
There was a problem hiding this comment.
This could just be method.getParameterTypes().length and the pathParams could be deleted as it's only used here.
There was a problem hiding this comment.
Thank you for your advice, you are correct. I changed the code, tests run just fine and will push the changes once we resolve the second problem
There was a problem hiding this comment.
Fixed in new commit
| for (int i = 0; i < processors.length; i++) { | ||
| if (processors != null && processors[i] instanceof WebTargetProcessor) { |
There was a problem hiding this comment.
If the processors can be null we need to do the null check sooner otherwise processors.length will throw an NPE.
There was a problem hiding this comment.
Thank you for notifying me about this if statement, though this code should not be marked as changed - I may have unknowingly done some format changes and Github decided to highlight them. Similar if statement can be seen on line 185 in the committed file.
Despite that, I believe this check is not necessary, as processors is an object created by ProcessorFactory.createProcessorswhich returns an array of objects, no matter if it's empty or not. In my point of view, in order to get null Processors , the ProcessorFactory should throw an exception itself and thus such case can't happen.
I think that the original idea for the part of this line should be processors[i] since it's part of a for loop, but changing the part or completely removing it does not effect tests at all.
Should I fix the code so this check happens before the for loop?
There was a problem hiding this comment.
Ah right I see. It doesn't matter much, but if you're up for I say we do in this case as it looks odd.
There was a problem hiding this comment.
No problem, fixed in new commit
petrberan
force-pushed
the
RESTEASY-2845
branch
from
cb3bf1d
to
f8f0474
Compare
|
Thank you for the contribution. We are cleaning up older PR's that seem to be stale. If you feel this is still an issue please feel free to re-open. |
…ark in @path with Regex
petrberan
force-pushed
the
RESTEASY-2845
branch
from
f8f0474
to
9e6e014
Compare
|
Hi @jamezp can I ask for a review please? There is a one test failure, but I believe it's not caused by my changes as I don't see it in my local build |
Signed-off-by: James R. Perkins <jperkins@redhat.com>
https://issues.redhat.com/browse/RESTEASY-2845