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

Nilguard delayed arguments #639

Merged
merged 4 commits into from
Mar 1, 2024
Merged

Nilguard delayed arguments #639

merged 4 commits into from
Mar 1, 2024

Conversation

kickster97
Copy link
Member

WHAT is this pull request doing?

If you try and redeclare a delayed exchange with no x-delayed-type argument, it will crash in exchanges.cr: match?(frame)

This pull request adds a nilguard for the argument check, and as a result, the match method will return false if there is no x-delayed-type argument and send aPreconditioned Failed: Existing exchange 'my_delayed_exchange' declared with other arguments instead of crashing.

HOW can this pull request be tested?

Declare a delayed exchange "my-delayed-exchange", with argument { "x-delayed-type" => "direct" }, then try and declare the exchange again, but with no x-delayed-type argument.

@CLAassistant
Copy link

CLAassistant commented Mar 1, 2024

CLA assistant check
All committers have signed the CLA.

@kickster97
Copy link
Member Author

for some reason the other pr would not track my commits... maybe something is fishy with the CLA assistant :(
I closed that one and opened a new one.

@kickster97 kickster97 force-pushed the nilguard_delayed_arguments branch 2 times, most recently from 80445c1 to d5638aa Compare March 1, 2024 15:41
@kickster97 kickster97 merged commit e9b2640 into main Mar 1, 2024
26 of 27 checks passed
@kickster97 kickster97 deleted the nilguard_delayed_arguments branch March 1, 2024 16:00
viktorerlingsson pushed a commit that referenced this pull request Sep 20, 2024
* add nilguard for matching x-delayed-type argument

* add specs for delayed message exchanges
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.

2 participants