CAMEL-14242: Fix binding parameters with arg.- is not possible when using endpointdsl and RabbitMQ #3385
Conversation
Trying to fix this one (https://issues.apache.org/jira/browse/CAMEL-14242) not sure about side effect but it's seem to do the job
omarsmak
changed the title
There was a problem hiding this comment.
Thanks @mikadev for the PR. I think you are heading into the right direction to fix this, good job! Some comments I added. Also, I'd appreciate if you can add a simple unit test simulating this fix, you can take a look at the a previous unit test for an example, in order to test the input of args on what do you expect if you have an input param of "arg.queue.x-max-priority" -> "10" vs "queue.x-max-priority" -> "10".
P/S: Please for the next time, please take a look at the contribution guideline, most importantly here in regards to the commit message and PR title.
| @@ -185,6 +185,8 @@ protected RabbitMQEndpoint createEndpoint(String uri, | |||
| } | |||
| } | |||
|
|
|||
| @SuppressWarnings("unchecked") | |||
| Map<String, Object> args = resolveAndRemoveReferenceParameter(params, "args", Map.class, getArgs()); | |||
There was a problem hiding this comment.
I think this is not needed, under by setProperties is doing the job to resolve the args param
There was a problem hiding this comment.
Same as my next comment
| @@ -202,6 +204,7 @@ protected RabbitMQEndpoint createEndpoint(String uri, | |||
| endpoint.setAddresses(getAddresses()); | |||
| endpoint.setThreadPoolSize(getThreadPoolSize()); | |||
| endpoint.setExchangeName(exchangeName); | |||
| endpoint.setArgs(args); | |||
There was a problem hiding this comment.
Same here, I think this is not needed, as this being handled by setProperties(endpoint, params);
There was a problem hiding this comment.
This is all the point params parameter is always an empty map where args is setting using query parameters is my PR there is something wrong i don't know where i think we have to cleanup this component
There was a problem hiding this comment.
What do you mean? What are you trying to fix exactly here? Because I feel we are mixing things up here.
There was a problem hiding this comment.
I trying to first getting args argument when it passed as query parameters lets just fix this for beginning
There was a problem hiding this comment.
But I thought if you pass it like this rabbitmq:foo?queue=foo&arg.queue.x-max-priority=10 it will work?
There was a problem hiding this comment.
What do you mean ? i really didn't understand
There was a problem hiding this comment.
The case is simple how i suppose to translate this weird way &arg.queue.x-max-priority=10 of passing parameter with the new DSL
There was a problem hiding this comment.
In this PR, I suggested to remove some lines from the code. This line where we are commenting now and here
There was a problem hiding this comment.
But this is the main point if your remove this line i'll get an empty map
There was a problem hiding this comment.
Maybe if you can try this and i think you'll see what i'm trying to solve
The main point is not about if we have |
|
Okay thanks @mikadev for the PR. I polished and worked on a fix with an unit test. |
|
Sorry for the delay, the fix looks good to me. I'm doing a full pass just for the context/tracking. |
Trying to fix this one (https://issues.apache.org/jira/browse/CAMEL-14242) not sure about side effect but it's seem to do the job