You are viewing a plain text version of this content. The canonical link for it is here.
Posted to gitbox@activemq.apache.org by GitBox <gi...@apache.org> on 2021/02/23 15:30:10 UTC
[GitHub] [activemq-artemis] cshannon opened a new pull request #3465: ARTEMIS-3118: Add a null check when checking matching metaData inside Federated Queue
cshannon opened a new pull request #3465:
URL: https://github.com/apache/activemq-artemis/pull/3465
Other protocols besides CORE may not have a metadata map so we need to
check for null before passing to the filter matcher
----------------------------------------------------------------
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] [activemq-artemis] jbertram commented on pull request #3465: ARTEMIS-2802: Add a null check when checking matching metaData inside Federated Queue
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3465:
URL: https://github.com/apache/activemq-artemis/pull/3465#issuecomment-784288442
Nice catch, @cshannon. Any chance you could add a test to avoid potential regressions in the future?
----------------------------------------------------------------
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] [activemq-artemis] cshannon commented on pull request #3465: ARTEMIS-2802: Add a null check when checking matching metaData inside Federated Queue
Posted by GitBox <gi...@apache.org>.
cshannon commented on pull request #3465:
URL: https://github.com/apache/activemq-artemis/pull/3465#issuecomment-784366414
@jbertram - So it looks like it would take some work to convert the tests over to use a real port and to then use OpenWire, etc. I tried to do it real fast but the RetryRule is giving me a lot of problems with port binding issues and I don't really have more time this week to work on it unfortunately as I have a bunch of other stuff to do. If it's not urgent I can try and get back to it when I have time if no one else wants to pick it up and try and do a test.
----------------------------------------------------------------
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] [activemq-artemis] cshannon commented on pull request #3465: ARTEMIS-2802: Add a null check when checking matching metaData inside Federated Queue
Posted by GitBox <gi...@apache.org>.
cshannon commented on pull request #3465:
URL: https://github.com/apache/activemq-artemis/pull/3465#issuecomment-784410891
@jbertram - I took one more quick look at writing a test because I realized @michaelandrepearce uploaded some modified test files that work with OpenWire. After looking at it i'm not really sure we can write a good test.
I ran those test files and I do see the log but as pointed out everything still passes. The reason why those tests pass are because when the exception occurs inside of FilterImpl it just returns false which is what the proper result should be anyways. See: https://github.com/apache/activemq-artemis/blob/36a771150b3ce7a0d889f7df3b7c7a6cd37769f7/artemis-server/src/main/java/org/apache/activemq/artemis/core/filter/impl/FilterImpl.java#L128
So, I'm not really sure of a way (at least with integration tests) to write a test to verify the fix as the correct thing already happens either way thanks to the error handling in the exception block. The only thing I can think of would be to maybe mock some stuff and add a test to try and verify that the createRemoteConsumer method checks for null properly before passing to the metaDataFilter but that would probably require re-working the FederatedQueue class to be able to get access to test as everything is private and non-static now.
----------------------------------------------------------------
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] [activemq-artemis] asfgit closed pull request #3465: ARTEMIS-2802: Add a null check when checking matching metaData inside Federated Queue
Posted by GitBox <gi...@apache.org>.
asfgit closed pull request #3465:
URL: https://github.com/apache/activemq-artemis/pull/3465
----------------------------------------------------------------
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] [activemq-artemis] cshannon commented on pull request #3465: ARTEMIS-2802: Add a null check when checking matching metaData inside Federated Queue
Posted by GitBox <gi...@apache.org>.
cshannon commented on pull request #3465:
URL: https://github.com/apache/activemq-artemis/pull/3465#issuecomment-784292407
@jbertram - I can try, the current tests are only set up for CORE. I can see if I can add a quick test for OpenWire to FederatedQueue before merging.
----------------------------------------------------------------
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] [activemq-artemis] jbertram commented on pull request #3465: ARTEMIS-2802: Add a null check when checking matching metaData inside Federated Queue
Posted by GitBox <gi...@apache.org>.
jbertram commented on pull request #3465:
URL: https://github.com/apache/activemq-artemis/pull/3465#issuecomment-788218859
@cshannon, that's fair enough. Thanks for looking into it. I've merged the PR.
----------------------------------------------------------------
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