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.

Sign up for GitHub

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

Jump to bottom

Add support for additional AMQP URI query parameters #251

Merged
merged 1 commit into from
Mar 13, 2024
Merged

Add support for additional AMQP URI query parameters #251

merged 1 commit into from
Mar 13, 2024

Conversation

vilius-g
Copy link
Contributor

RabbitMQ URI Query Parameters specifies several parameters that are used in this library, but not supported in URIs.

This commit adds support for the following parameters:
auth_mechanism
heartbeat
connection_timeout
channel_max

@lukebakken lukebakken self-assigned this Feb 29, 2024
@lukebakken lukebakken self-requested a review February 29, 2024 16:24
@lukebakken lukebakken added this to the 1.9.1 milestone Feb 29, 2024
Zerpet
Zerpet requested changes Mar 1, 2024
View reviewed changes
connection.go Outdated Show resolved Hide resolved
@Zerpet
Copy link
Contributor

Zerpet commented Mar 1, 2024

The check that fails was not introduced in this PR. We can safely ignore it for now and address it in a different PR.

@vilius-g vilius-g force-pushed the additional_amqp_uri_parameters branch from 8e0d6fc to 4835984 Compare March 2, 2024 08:21
Zerpet
Zerpet approved these changes Mar 5, 2024
View reviewed changes
Copy link
Contributor

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

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

Looks good, I'll merge after CI passes 👍

@Zerpet
Copy link
Contributor

Zerpet commented Mar 6, 2024

@vilius-g I updated this PR from main to include #252 and get rid of the CI error. It looks like I broke your code in the process. Sorry about that!

I understand if you don't want to put more time into this PR. Would you let me know if you can fix these errors? I can take over otherwise.

@vilius-g
Copy link
Contributor Author

vilius-g commented Mar 6, 2024

@vilius-g I updated this PR from main to include #252 and get rid of the CI error. It looks like I broke your code in the process. Sorry about that!

No problem, already fixed.

But I have noticed another issue. When using Dial, DialTLS, DialTLS_ExternalAuth, default heartbeat value will always be used regardless of what the URI contains. I am not sure how to best solve this, because this will require changes to the after-mentioned methods.

@lukebakken
Copy link
Contributor

I will fix up this PR, and will check out the functions that @vilius-g mentions.

@Zerpet
Copy link
Contributor

Zerpet commented Mar 6, 2024

@vilius-g I updated this PR from main to include #252 and get rid of the CI error. It looks like I broke your code in the process. Sorry about that!

No problem, already fixed.

But I have noticed another issue. When using Dial, DialTLS, DialTLS_ExternalAuth, default heartbeat value will always be used regardless of what the URI contains. I am not sure how to best solve this, because this will require changes to the after-mentioned methods.

We can resolve this by removing the default heartbeat from afore mentioned functions, and performing validation in DialConfig function, such as this pseudo-code

if config.heartbeat == 0; 
then if uri.heartbeat == 0; 
then config.heartbeat = defaultheartbeat; 
else config.heartbeat = uri.heartbeat

We will maintain same behaviour in other functions when URI parameters are not provided, and support URI parameters in those functions. We must keep the previous behaviour, otherwise, it can be a potentially breaking change.

@lukebakken lukebakken force-pushed the additional_amqp_uri_parameters branch from de8f134 to 9044e89 Compare March 6, 2024 18:07
@lukebakken lukebakken requested a review from Zerpet March 6, 2024 20:52
@vilius-g
Copy link
Contributor Author

vilius-g commented Mar 7, 2024

We can resolve this by removing the default heartbeat from afore mentioned functions, and performing validation in DialConfig function

The only potential issue I see with this, is that now DialConfig will set default heartbeat where it previously wouldn't.

And there will no longer be a way to disable heartbeat from the client?

@Zerpet
Copy link
Contributor

Zerpet commented Mar 8, 2024

We can resolve this by removing the default heartbeat from afore mentioned functions, and performing validation in DialConfig function

The only potential issue I see with this, is that now DialConfig will set default heartbeat where it previously wouldn't.

And there will no longer be a way to disable heartbeat from the client?

That's a good catch! I usually solve this problem of "is unset or is zero-value?" using a custom type, or a pointer. With the custom type, you can add a function IsSet() that returns true if the value was set. A pointer to a zero-value is the zero value, and you pass it; a nil pointer means the value was not set, and you apply a default. I'm happy with either solution. Since the field Heartbeat is new in the struct, we can go for either solution.

lukebakken
lukebakken approved these changes Mar 9, 2024
View reviewed changes
Copy link
Contributor

@lukebakken lukebakken left a comment

Choose a reason for hiding this comment

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

@Zerpet, what do you think?

@lukebakken lukebakken force-pushed the additional_amqp_uri_parameters branch from 9b20c40 to 70d14d8 Compare March 11, 2024 23:54
@lukebakken lukebakken self-requested a review March 11, 2024 23:54
@lukebakken lukebakken force-pushed the additional_amqp_uri_parameters branch from 421d09a to 05a8b67 Compare March 12, 2024 14:49
Zerpet
Zerpet approved these changes Mar 13, 2024
View reviewed changes
Copy link
Contributor

@Zerpet Zerpet left a comment

Choose a reason for hiding this comment

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

This is as good as we can do. We should mention in the function comment of DialConfig that heartbeat in the URI takes precedence over heartbeat in the Config. If you want to disable heartbeats, set in the URI the heartbeat to 0. I personally don't mind that we make somewhat harder to disable heartbeats, as 99% of environments shouldn't disable them.

@vilius-g @lukebakken
Add support for additional AMQP URI query parameters
28ebb24 
https://www.rabbitmq.com/docs/uri-query-parameters specifies several parameters that are used in this library, but not yet supported in URIs.

This commit adds support for the following parameters:
auth_mechanism
heartbeat
connection_timeout
channel_max

Fix default value check when setting SASL authentication from URI

Add documentation for added query parameters

Add support for additional AMQP URI query parameters

https://www.rabbitmq.com/docs/uri-query-parameters specifies several parameters that are used in this library, but not yet supported in URIs.

This commit adds support for the following parameters:
auth_mechanism
heartbeat
connection_timeout
channel_max

Fix default value check when setting SASL authentication from URI

Fix ChannelMax type mismatch

Use URI heartbeat

Bump versions on Windows
@lukebakken lukebakken force-pushed the additional_amqp_uri_parameters branch from 05a8b67 to 28ebb24 Compare March 13, 2024 17:59
@lukebakken lukebakken merged commit 4172682 into rabbitmq:main Mar 13, 2024
7 checks passed
Zerpet added a commit that referenced this pull request Apr 23, 2024
@Zerpet
Update deps: use Go 1.20
532824e 
In #251, we used `URI.Has()` function, which was introduced in Go 1.17.
We also have a dependency on uber-go-leak, which requires Go 1.20. In
addition, we only test in "oldstable" and "stable" versions in CI. That
means, we test in version N and N-1, where N is the latest available
version of Go.

Signed-off-by: Aitor Perez Cedres <[email protected]>
@Zerpet Zerpet mentioned this pull request Apr 23, 2024
Merged
@Zerpet Zerpet added the enhancement New feature or request label May 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Zerpet Zerpet Zerpet approved these changes

@lukebakken lukebakken Awaiting requested review from lukebakken

Assignees

@Gsantomaggio Gsantomaggio

@lukebakken lukebakken

@Zerpet Zerpet

Labels
enhancement New feature or request
Projects
None yet
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@vilius-g @Zerpet @lukebakken @Gsantomaggio