Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changed header pattern #186

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Changed header pattern #186

wants to merge 2 commits into from

Conversation

boginw
Copy link

@boginw boginw commented Sep 25, 2020

According to the STOMP protocol specification 1.2; each header should be terminated with an EOL. Currently, the regex for headers stops if it sees a :. This pull request fixes the regex.

An example of a message that the current implementation can't handle is shown below:

ERROR
message:Failed to send message to ExecutorSubscribableChannel[clientInboundChannel]; nested exception is org.springframework.security.access.AccessDeniedException\c Access is denied
content-length:0

 

@forresthopkinsa
Copy link
Contributor

The regex currently includes a colon in the middle to address standard headers like the example you gave:

([^:\s]+)\s*:\s*([^:\s]+)

@boginw
Copy link
Author

boginw commented Sep 26, 2020

Yes, and that is fine. The issue is the second group. Please see the following link, with the original regex and the body given in the example : regexr.com/5crbl. There you'll notice, that only the first word is recognized. What this PR does, is to capture the second group fully, by only terminating on a newline, instead of a colon.

@forresthopkinsa
Copy link
Contributor

Ahh I understand. 👍

Copy link
Contributor

@forresthopkinsa forresthopkinsa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

build.gradle changes aren't necessary, but otherwise LGTM

@boginw boginw force-pushed the master branch 2 times, most recently from 1b803ac to 9cb5e63 Compare October 16, 2020 15:05
@boginw
Copy link
Author

boginw commented Oct 16, 2020

The PR now only includes the header change

@kientux
Copy link

kientux commented Jan 13, 2021

Can someone merge this and release a new version please?

@boginw
Copy link
Author

boginw commented Sep 16, 2021

@NaikSoftware this issue is still present. Would you be so kind to review and merge this branch?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants