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 2022/11/22 18:44:55 UTC
[GitHub] [cxf] aziubin opened a new pull request, #1033: CXF-8796: Replace REMOVED_MARKER with null
aziubin opened a new pull request, #1033:
URL: https://github.com/apache/cxf/pull/1033
The reason of java.lang.IllegalArgumentException: argument type mismatch exception with code first RPC is that inherited ArrayList.toArray is used to prepare arguments for DelegatingMethodAccessorImpl.invoke passing MessageContentsList.REMOVED_MARKER java.lang.Object instance directly to parameter of Web service method (see stack below).
MessageContentsList.REMOVED_MARKER instance acts as an indicator of absent value and appears if the value of parameter is not provided in SOAP request (see added comments in the source code). In the fix, REMOVED_MARKER is replaced with "null" value in overriden MessageContentsList.toArray. This change prevents java.lang.IllegalArgumentException: argument type mismatch exception and works with bean validation for both hibernate-validator and Apache bval bean validation providers.
CXF 3.5.2 stack and values when ArrayList.toArray is used to prepare arguments for DelegatingMethodAccessorImpl.invoke, where java.lang.Object@12d4589f is MessageContentsList.REMOVED_MARKER:
```
@WebService(targetNamespace = "http://test.apache.org/")
@SOAPBinding(style = javax.jws.soap.SOAPBinding.Style.RPC, use = javax.jws.soap.SOAPBinding.Use.LITERAL)
public class SoapBindingArgumentTypeMismatch {
public boolean allocate(Integer projectId,
Integer[] targetIds,
Integer[] parameterIds) {
return targetIds == null;
}
}
```
```
MessageContentsList.toArray() line: 124
JAXWSMethodInvoker(AbstractInvoker).invoke(Exchange, Object, Method, List<Object>) line: 93
JAXWSMethodInvoker(AbstractJAXWSMethodInvoker).invoke(Exchange, Object, Method, List<Object>) line: 232
JAXWSMethodInvoker.invoke(Exchange, Object, Method, List<Object>) line: 85
JAXWSMethodInvoker(AbstractInvoker).invoke(Exchange, Object) line: 74
ServiceInvokerInterceptor$1.run() line: 59
Executors$RunnableAdapter<T>.call() line: 515
ServiceInvokerInterceptor$2(FutureTask<V>).run() line: 264
ServiceInvokerInterceptor$2.run() line: 126
SynchronousExecutor.execute(Runnable) line: 37
ServiceInvokerInterceptor.handleMessage(Message) line: 131
PhaseInterceptorChain.doIntercept(Message) line: 307
ChainInitiationObserver.onMessage(Message) line: 121
ServletDestination(AbstractHTTPDestination).invoke(ServletConfig, ServletContext, HttpServletRequest, HttpServletResponse) line: 265
ServletController.invokeDestination(HttpServletRequest, HttpServletResponse, AbstractHTTPDestination) line: 234
ServletController.invoke(HttpServletRequest, HttpServletResponse, boolean) line: 208
ServletController.invoke(HttpServletRequest, HttpServletResponse) line: 160
CXFServlet(CXFNonSpringServlet).invoke(HttpServletRequest, HttpServletResponse) line: 225
CXFServlet(AbstractHTTPServlet).handleRequest(HttpServletRequest, HttpServletResponse) line: 304
CXFServlet(AbstractHTTPServlet).doPost(HttpServletRequest, HttpServletResponse) line: 217
CXFServlet(HttpServlet).service(HttpServletRequest, HttpServletResponse) line: 681
CXFServlet(AbstractHTTPServlet).service(ServletRequest, ServletResponse) line: 279
ApplicationFilterChain.internalDoFilter(ServletRequest, ServletResponse) line: 227
ApplicationFilterChain.doFilter(ServletRequest, ServletResponse) line: 162
WsFilter.doFilter(ServletRequest, ServletResponse, FilterChain) line: 53
ApplicationFilterChain.internalDoFilter(ServletRequest, ServletResponse) line: 189
ApplicationFilterChain.doFilter(ServletRequest, ServletResponse) line: 162
StandardWrapperValve.invoke(Request, Response) line: 197
StandardContextValve.invoke(Request, Response) line: 97
NonLoginAuthenticator(AuthenticatorBase).invoke(Request, Response) line: 540
StandardHostValve.invoke(Request, Response) line: 135
RewriteValve.invoke(Request, Response) line: 289
ErrorReportValve.invoke(Request, Response) line: 92
StandardEngineValve.invoke(Request, Response) line: 78
CoyoteAdapter.service(Request, Response) line: 359
Http11Processor.service(SocketWrapperBase<?>) line: 399
Http11Processor(AbstractProcessorLight).process(SocketWrapperBase<?>, SocketEvent) line: 65
AbstractProtocol$ConnectionHandler<S>.process(SocketWrapperBase<S>, SocketEvent) line: 889
NioEndpoint$SocketProcessor.doRun() line: 1735
NioEndpoint$SocketProcessor(SocketProcessorBase<S>).run() line: 49
ThreadPoolExecutor.runWorker(ThreadPoolExecutor$Worker) line: 1191
ThreadPoolExecutor$Worker.run() line: 659
TaskThread$WrappingRunnable.run() line: 61
TaskThread(Thread).run() line: 829
```
```
this MessageContentsList (id=72) [1, java.lang.Object@12d4589f, [Ljava.lang.Integer;@16ebce1d]
elementData Object[6] (id=133) [1, java.lang.Object@12d4589f, [222], null, null, null]
[0] Integer (id=134) 1
value 1 1
[1] Object (id=138) java.lang.Object@12d4589f
[2] Integer[1] (id=139) [222]
[0] Integer (id=142) 222
[3] null null
[4] null null
[5] null null
modCount 3 3
size 3 3
idx 0 0
size 3 3
array Object[3] (id=82) [null, null, null]
[0] null null
[1] null null
[2] null null
```
--
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
[GitHub] [cxf] aziubin commented on pull request #1033: CXF-8796: Replace REMOVED_MARKER with null
Posted by GitBox <gi...@apache.org>.
aziubin commented on PR #1033:
URL: https://github.com/apache/cxf/pull/1033#issuecomment-1331969825
@reta thanks. I have verified that last change and it work OK for me too.
--
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
[GitHub] [cxf] reta commented on pull request #1033: CXF-8796: Replace REMOVED_MARKER with null
Posted by GitBox <gi...@apache.org>.
reta commented on PR #1033:
URL: https://github.com/apache/cxf/pull/1033#issuecomment-1324464561
Hey @aziubin, thanks a lot reporting the issue and trying to fix it. I was able to craft the test case and reproduce it (again, because of your very detailed report).
Sadly the solution you are suggesting is covering only one specific variation of the problem. For example, when argument is removed in between (last argument is omitted, we would end up with `wrong number of arguments`). Another issue is primitive types: by stripping the `REMOVED_MARKER` placeholder you are effectively trading it for `null` which does not play well with primitive Java type (in this particular case, imagine that argument in the middle is `int`, we end up with `argument type mismatch` anyway).
I don't have the solution off the top of my head, also it would be great to consult the SOAP spec regarding the cases when operation parameters are omitted.
--
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
[GitHub] [cxf] reta commented on pull request #1033: CXF-8796: Replace REMOVED_MARKER with null
Posted by GitBox <gi...@apache.org>.
reta commented on PR #1033:
URL: https://github.com/apache/cxf/pull/1033#issuecomment-1331255715
> @reta looks good. I tested new fix in my environment and it works well. There is one thing, which I wanted to discuss with you. The `MessageContentsList(List<?> values)` constructor can be used to create new `MessageContentsList` instance based on existing generic list. There is a possibility that existing code uses this constructor, passing another `MessageContentsList` as argument. In this hypothetical situation, `removed` hash set of the original `MessageContentsList` will not be taken into account, and all markers will be lost in new instance of `MessageContentsList`. With original implementation all removed markers will be presented. To solve this, we may change code like below:
>
> ```
> public MessageContentsList(List<?> values) {
> super(values);
> if (values instanceof MessageContentsList) {
> removed = new HashSet<>(((MessageContentsList) values).removed);
> }
> }
> ```
>
> I verified that `MessageContentsList` is never passed as argument of `MessageContentsList(List<?> values)` in CXF source code and this additional change can be necessary just to keep backward compatibility if existing custom code passes `MessageContentsList` to `MessageContentsList(List<?> values)`.
It makes perfect sense, thank you @aziubin , the suggested change is pushed
--
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
[GitHub] [cxf] reta commented on pull request #1033: CXF-8796: Replace REMOVED_MARKER with null
Posted by GitBox <gi...@apache.org>.
reta commented on PR #1033:
URL: https://github.com/apache/cxf/pull/1033#issuecomment-1327922758
@aziubin do you mind if push a few changes to your pull request? I would like to add tests and share an idea that does not use `REMOVED_MARKER` in the first place (but preserving the option to use it in case the old behavior is important). If you are not against it, I probably need to ask you please to allow the changes from maintainers.
--
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
[GitHub] [cxf] reta commented on pull request #1033: CXF-8796: Replace REMOVED_MARKER with null
Posted by GitBox <gi...@apache.org>.
reta commented on PR #1033:
URL: https://github.com/apache/cxf/pull/1033#issuecomment-1332149388
@dkulp @coheigea guys I am wondering if you have any comments / concerns here
--
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
[GitHub] [cxf] aziubin commented on pull request #1033: CXF-8796: Replace REMOVED_MARKER with null
Posted by GitBox <gi...@apache.org>.
aziubin commented on PR #1033:
URL: https://github.com/apache/cxf/pull/1033#issuecomment-1325348559
Hi @reta, I did some corrections of my update. Please see updated version and ignore the first variant. Thanks!
--
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
[GitHub] [cxf] aziubin commented on pull request #1033: CXF-8796: Replace REMOVED_MARKER with null
Posted by GitBox <gi...@apache.org>.
aziubin commented on PR #1033:
URL: https://github.com/apache/cxf/pull/1033#issuecomment-1331034447
@reta looks good. I tested new fix in my environment and it works well. There is one thing, which I wanted to discuss with you. The `MessageContentsList(List<?> values)` constructor can be used to create new `MessageContentsList` instance based on existing generic list. There is a possibility that existing code uses this constructor, passing another `MessageContentsList` as argument. In this hypothetical situation, `removed` hash set of the original `MessageContentsList` will not be taken into account, and all markers will be lost in new instance of `MessageContentsList`. With original implementation all removed markers will be presented. To solve this, we may change code like below:
public MessageContentsList(List<?> values) {
super(values);
if (values instanceof MessageContentsList) {
removed = new HashSet<>(((MessageContentsList) values).removed);
}
}
I verified that `MessageContentsList` is never passed as argument of `MessageContentsList(List<?> values)` in CXF source code and this additional change can be necessary just to keep backward compatibility if existing custom code passes `MessageContentsList` to `MessageContentsList(List<?> values)`.
--
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
[GitHub] [cxf] aziubin commented on pull request #1033: CXF-8796: Replace REMOVED_MARKER with null
Posted by GitBox <gi...@apache.org>.
aziubin commented on PR #1033:
URL: https://github.com/apache/cxf/pull/1033#issuecomment-1325250610
Hi @reta, thank you too for reproducing this issue and looking at code changes. The case when the last argument is omitted is not covered. The "argument type mismatch while invoking" error is blocking our migration from Axis to CXF. I put my effort mostly to understand if proposed change is isolated, safe and will not have negative impact on existing codebase. I did the following: verified that `REMOVED_MARKER` is not used in CXF code or out there; reviewed the code where `MessageContentsList.toArray` is used (that way I discovered an issue with bean validation, which is covered by this fix); reviewed the history of `MessageContentsList.java` file (that way I found CXF 2.0.3 SVN commit 587171, which introduced this issue - see [[CXF-1129] Fix issues with out of band headers causing marshalling issues Re-add asm jars to distribution](http://svn.apache.org/viewvc/cxf/trunk/api/src/main/java/org/apache/cxf/message/MessageContentsList.java?r1=587170&r2=587171&pathrev=1434564&)). As a
result, overriding `toArray` in my opinion looks like a missing change, which should have been done as part of commit 587171.
Wider fix, which will also cover the case when the last argument is omitted, would need research and additional development and I am opened for it. Potentially, this is not going to be isolated and safe change, so I need to hear your opinion first before starting.
The case with primitive type is different. Instead of an "argument type mismatch while invoking" error message, which does not allow us to understand the root cause of the error, there will be "null while invoking ..." message, which is correct and not confusing. Also, in opposite to the issue, which I am trying to fix, this case can be work around with a method wrapper, which has compatible wrapper class in the place of primitive type, for example `void allocateWrapper(Integer i) { allocate(i); } void allocate(int i) { }`. What do you think?
--
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
[GitHub] [cxf] reta commented on pull request #1033: CXF-8796: Replace REMOVED_MARKER with null
Posted by GitBox <gi...@apache.org>.
reta commented on PR #1033:
URL: https://github.com/apache/cxf/pull/1033#issuecomment-1325805132
> Hi @reta, thank you too for reproducing this issue and looking at code changes. The case when the last argument is omitted is not covered. The "argument type mismatch while invoking" error is blocking our migration from Axis to CXF. I put my effort mostly to understand if proposed change is isolated, safe and will not have negative impact on existing codebase.
Thanks @aziubin , the `MessageContentsList` is very widely used with CXF and may be outside (since this is a public class), so none of the "isolated, safe and will not have negative impact on existing codebase" could be asserted for sure. Also, the change would introduce a very tricky class of errors, where `REMOVED_MARKER` will still appear in `toArray(T[])`, `subList(...)`, `sort(...)` ... unless we override everything (probably not a good idea).
> I did the following: verified that `REMOVED_MARKER` is not used in CXF code or out there; reviewed the code where `MessageContentsList.toArray` is used (that way I discovered an issue with bean validation, which is covered by this fix); reviewed the history of `MessageContentsList.java` file (that way I found CXF 2.0.3 SVN commit 587171, which introduced this issue - see [[CXF-1129] Fix issues with out of band headers causing marshalling issues Re-add asm jars to distribution](http://svn.apache.org/viewvc/cxf/trunk/api/src/main/java/org/apache/cxf/message/MessageContentsList.java?r1=587170&r2=587171&pathrev=1434564&)). As a result, overriding `toArray` in my opinion looks like a missing change, which should have been done as part of commit 587171.
>
@dkulp I think you where working on that at the time, do you have opinion on the matter? May be we could keep the `null` placeholder but mirror the `REMOVED_MARKER` through complementary array?
> Wider fix, which will also cover the case when the last argument is omitted, would need research and additional development and I am opened for it. Potentially, this is not going to be isolated and safe change, so I need to hear your opinion first before starting.
>
I am planning to look for the clarification in spec regarding the omitted operation input, will get back with my findings.
> The case with primitive type is different. In opposite to the issue, which I am trying to fix, this case can be work around with a method wrapper, which has compatible wrapper class in the place of primitive type, for example `void allocateWrapper(Integer i) { if (i != null) allocate(i); } void allocate(int i) { }`. What do you think? We can look at improving error message for this case if necessary.
If we have to cover the omitted operation input, we should cover such cases as well, we should not be asking users for workarounds.
--
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
[GitHub] [cxf] dkulp commented on pull request #1033: CXF-8796: Replace REMOVED_MARKER with null
Posted by GitBox <gi...@apache.org>.
dkulp commented on PR #1033:
URL: https://github.com/apache/cxf/pull/1033#issuecomment-1341247761
LGTM
--
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
[GitHub] [cxf] reta commented on pull request #1033: CXF-8796: Replace REMOVED_MARKER with null
Posted by GitBox <gi...@apache.org>.
reta commented on PR #1033:
URL: https://github.com/apache/cxf/pull/1033#issuecomment-1326921258
> Below is another example of misbehavior because `REMOVED_MARKER` is passed outside of MessageContentsList class:
>
Yes, the choice of `REMOVED_MARKER` is unclear to me
> I might not completely understand this. Do you mean for existing code, or when developing new one assuming that parameter can be omitted?
I mean the fix should treat (imho) these two examples properly: when `projectId` is not present, it has to apply the correct placeholder instead of `REMOVED_MARKER` (assuming this flow has to be supported):
```
public boolean allocate(Integer projectId, Object targetIds, Integer[] parameterIds) { }
public boolean allocate(int projectId, Object targetIds, Integer[] parameterIds) { }
```
--
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
[GitHub] [cxf] aziubin commented on pull request #1033: CXF-8796: Replace REMOVED_MARKER with null
Posted by GitBox <gi...@apache.org>.
aziubin commented on PR #1033:
URL: https://github.com/apache/cxf/pull/1033#issuecomment-1328035372
> @aziubin do you mind if push a few changes to your pull request? I would like to add tests and share an idea that does not use `REMOVED_MARKER` in the first place (but preserving the option to use it in case the old behavior is important). If you are not against it, I probably need to ask you please to allow the changes from maintainers.
@reta, thank you for your help, please feel free to add any corrections. I had edits by maintainers enabled when I created pull request, let me know if that did not work for you. Could you also pass my change through CXF standard CI checks beforehand as it was not passed automatically.
--
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
[GitHub] [cxf] reta commented on pull request #1033: CXF-8796: Replace REMOVED_MARKER with null
Posted by GitBox <gi...@apache.org>.
reta commented on PR #1033:
URL: https://github.com/apache/cxf/pull/1033#issuecomment-1328074643
@aziubin pushed the change, wondering what do you think, basically the idea behind is to stop leaking the REMOVED_MARKER through underlying array at all occurrences (always use `null` placeholder). It does not work with primitive arguments though yet ...
--
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
[GitHub] [cxf] dkulp merged pull request #1033: CXF-8796: Replace REMOVED_MARKER with null
Posted by GitBox <gi...@apache.org>.
dkulp merged PR #1033:
URL: https://github.com/apache/cxf/pull/1033
--
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
[GitHub] [cxf] aziubin commented on pull request #1033: CXF-8796: Replace REMOVED_MARKER with null
Posted by GitBox <gi...@apache.org>.
aziubin commented on PR #1033:
URL: https://github.com/apache/cxf/pull/1033#issuecomment-1326759798
> Thanks @aziubin , the `MessageContentsList` is very widely used with CXF and may be outside (since this is a public class), so none of the "isolated, safe and will not have negative impact on existing codebase" could be asserted for sure. Also, the change would introduce a very tricky class of errors, where `REMOVED_MARKER` will still appear in `toArray(T[])`, `subList(...)`, `sort(...)` ... unless we override everything (probably not a good idea).
Thank you @reta, @dkulp, I agree. MessageContentsList is core functionality, which is used everywhere in CXF, so we should be careful. Unfortunately, `REMOVED_MARKER` was made publicly accessible, although its meaning (the indication of missing value in the list) points that it should be private or protected and not leave the methods of the class. As long as `REMOVED_MARKER` is not referenced in the code of other classes it is I think safe to stop returning it outside in the inherited toArray method. If there is a suspicion that other inherited methods of the ArrayList might be used too, we may address them as well to prevent potential defects. Below is another example of misbehavior because `REMOVED_MARKER` is passed outside of MessageContentsList class:
```
import javax.validation.constraints.NotNull;
import javax.validation.Valid;
. . .
public boolean allocate(Integer projectId,
@Valid @NotNull Object targetIds,
@Valid @NotNull Integer[] parameterIds) { }
```
When targetIds parameter is omitted, the NotNull rule will not work because `REMOVED_MARKER` instance is passed as argument to the targetIds parameter, although `null` should have been passed instead. This case is covered with this fix too.
> If we have to cover the omitted operation input, we should cover such cases as well, we should not be asking users for workarounds.
I might not completely understand this. Do you mean for existing code, or when developing new one assuming that parameter can be omitted? When `REMOVED_MARKER` argument is passed to primitive type it is not compatible, as well as when `null` is passed instead, so this fix as I can see does not change the behavior with primitive types for existing code, but only the error message. I do not have CXF expertise as yours, so let me know if I am missing something. Thanks!
--
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