search

rabbitmq / amqp091-go

An AMQP 0-9-1 Go client maintained by the RabbitMQ team. Originally by @streadway: `streadway/amqp`
Other
1.49k stars 135 forks source link

Add support for additional AMQP URI query parameters #251

Closed vilius-g closed 6 months ago

vilius-g commented 7 months ago

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

Zerpet commented 7 months ago

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

Zerpet commented 7 months ago

@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 commented 7 months ago

@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 commented 7 months ago

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

Zerpet commented 7 months ago

@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.

vilius-g commented 7 months ago

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 commented 7 months ago

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.