[Messenger] support for multiple bindings on the same queue #38485

inferont · 2020-10-08T16:01:47Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
Tickets	Fix #37233
License	MIT

Previous Discussion
#37722

Description
When using header based queues and exchanges to keep track of events, one may want to have multiple bindings on those headers in those queues. Adding a few lines of code to Connection.php in amqp-messenger will allow Symfony Messenger to support this type of queue. This is pretty standard in other amqp library implementations.

Details
This PR introduces a small refactoring that deprecates the binding_keys and binding_arguments options with a new array option, bindings. This bindings options allows any number of arrays with a 'key' string value, another 'arguments' array value, and possibly an 'exchange' value in future revisions to support multiple exchanges.

Example

async_consume_task:
    dsn: '%env(MESSENGER_TRANSPORT_DSN)%'
    options:
        vhost: message-bus
        exchange:
            name: message-bus-exchange
            type: headers
        queues:
            'cart-purchases-header-queue':
                'bindings':
                    'manual-messages':
                        'arguments':
                            message-type: 'task'
                            subject: 'cart'
                            x-bind: all
                    'customer-messages':
                        'arguments':
                            message-type: 'event'
                            subject: 'customer'
                            x-bind: all
            'general-events-topic-queue':
                'bindings':
                    0:
                        'key': 'entity-change.*.*.*'

Nyholm · 2020-10-08T16:19:35Z

Thank you for this PR.

It seams like this is a feature specific to AMQP. It is also not a "80% or more people are doing this". I suggest that this advanced/uncommon configuration of AMQP should be done outside the scope of the messenger component.

Correct me if Im wrong, but one could easily configure like RabbitMq to have these bindings and then just use the exchange normally with the messenger component, right?

inferont · 2020-10-08T19:06:57Z

@Nyholm
Please also see the discussion in the previous PR at #37233

Having more than one binding is a very common configuration for queues in AMQP. Setting multiple bindings is also supported by all of the other AMQP libraries. I would also guess that this might be more common than confirming message delivery which has also been recently added to the amqp-messenger component.

Yes, it is possible to manually configure RabbitMQ with these bindings. This isn't exactly the best idea though. When you have an application that runs in development, staging, and production these bindings and other RabbitMQ configuration options can be easily stored in a config file to be applied on any environment needed. They also have a commit history for other developers to see what was changed. Having these as config values follows the same reasoning as using migration files to apply SQL changes to the database rather than manually applying them by hand.

Thanks for your time!

Nyholm

Hm. I see the previous discussions in #37722. Thank you.

When Im looking at the implementation I do like the fact that we only modify the AMQP bridge part of the code and that we not putting logic in the FrameworkBundle. I also agree with you that having config in version control makes sense. However, the Messenger component is not the only place to put infrastructure config =)

Im am convinced that a majority of people (more that 50% of AMQP users) do NOT use this feature, even though you claim it is very common.

I see that others have upvoted your original PR. Im going to try to give this a review and some testing later.

Nyholm · 2020-10-08T20:20:15Z

src/Symfony/Component/Messenger/Bridge/Amqp/Transport/Connection.php

+                if (\is_array($queue['bindings'] ?? false)) {
+                    foreach ($queue['bindings'] as $title => $individualBinding) {
+                        if (0 < \count($invalidBindingsOptions = array_diff(array_keys($individualBinding), self::AVAILABLE_BINDINGS_OPTIONS))) {
+                            throw new \InvalidArgumentException(sprintf('Invalid bindings option(s) "%s" passed to the AMQP Messenger transport in "%s". Each "bindings" option only accepts "key" and "arguments"', implode('", "', $invalidBindingsOptions), $title));


Could we say something like "Valid options for each 'bindings' are: %s", implode('", ", self::AVAILABLE_BINDINGS_OPTIONS?

Sure, it's been updated.

Nyholm · 2020-10-08T20:21:21Z

src/Symfony/Component/Messenger/Bridge/Amqp/Transport/Connection.php

        'binding_keys',
        'binding_arguments',
-        'flags',


Will all DSN strings that work in 5.1 still work after merging this PR?

Yes, this is fully backwards compatible. Only deprecation notices are introduced for the binding_keys and binding_arguments.

…ided

inferont

@Nyholm Please see my updates.

inferont · 2020-10-09T20:12:12Z

src/Symfony/Component/Messenger/Bridge/Amqp/Transport/Connection.php

        'binding_keys',
        'binding_arguments',
-        'flags',


Yes, this is fully backwards compatible. Only deprecation notices are introduced for the binding_keys and binding_arguments.

inferont · 2020-10-09T20:22:07Z

src/Symfony/Component/Messenger/Bridge/Amqp/Transport/Connection.php

+                if (\is_array($queue['bindings'] ?? false)) {
+                    foreach ($queue['bindings'] as $title => $individualBinding) {
+                        if (0 < \count($invalidBindingsOptions = array_diff(array_keys($individualBinding), self::AVAILABLE_BINDINGS_OPTIONS))) {
+                            throw new \InvalidArgumentException(sprintf('Invalid bindings option(s) "%s" passed to the AMQP Messenger transport in "%s". Each "bindings" option only accepts "key" and "arguments"', implode('", "', $invalidBindingsOptions), $title));


Sure, it's been updated.

fabpot · 2022-08-07T09:56:07Z

Based on @Nyholm's comments, and the lack of traction for this feature, it may be better to close it.

…ple-bindings # Conflicts: # src/Symfony/Component/Messenger/Bridge/Amqp/Transport/Connection.php

…rations

githoober · 2023-01-08T17:37:27Z

@sroze , @Nyholm,

Hi everyone,

There is still interest in merging this PR. I have updated it and made sure its tests are passing. But I am getting the following error that I don't fully understand.

Would you be able to point me in the right direction?

Thanks.

githoober · 2023-01-13T18:36:22Z

I understand that the PHPUnit tests that are failing right now are failing because of deprecation warnings. How can this be fixed?

37233 - Support multiple bindings for a queue

74339e7

inferont requested a review from sroze as a code owner October 8, 2020 16:01

carsonbot added Status: Needs Review Messenger Feature labels Oct 8, 2020

inferont changed the title ~~[Messenger] support for multiple bindings on the same queue #37722~~ [Messenger] support for multiple bindings on the same queue Oct 8, 2020

inferont mentioned this pull request Oct 8, 2020

[Messenger] support for multiple bindings on the same queue #37722

Closed

Nyholm reviewed Oct 8, 2020

View reviewed changes

Tweak to exception message when invalid options for bindings are prov…

b5e2f0e

…ided

inferont requested a review from Nyholm October 9, 2020 20:22

inferont commented Oct 9, 2020

View reviewed changes

nicolas-grekas added this to the 5.x milestone Oct 12, 2020

fabpot modified the milestones: 5.4, 6.1 Nov 16, 2021

fabpot modified the milestones: 6.1, 6.2 May 20, 2022

nicolas-grekas modified the milestones: 6.2, 6.3 Nov 5, 2022

Oleg Namaka and others added 5 commits January 6, 2023 14:58

Merge remote-tracking branch 'origin/master' into 37233-support-multi…

2bf09a7

…ple-bindings # Conflicts: # src/Symfony/Component/Messenger/Bridge/Amqp/Transport/Connection.php

Update the branch to target symfony 6.3

4b4dd05

Fix the failing test, remove unused variables, add missing type decla…

b204b07

…rations

Merge branch 'symfony:6.3' into 37233-support-multiple-bindings

5b6f842

Remove unused variables

4c1e3da

Oleg Namaka added 3 commits January 12, 2023 16:55

Improve handling of new and deprecated options, add more test coverage

768b1ca

Temporarily remove deprecation logic

2275b22

Restore deprecation triggering

35d6478

nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023

nicolas-grekas modified the milestones: 6.4, 7.1 Nov 15, 2023

xabbuh modified the milestones: 7.1, 7.2 May 15, 2024

fabpot modified the milestones: 7.2, 7.3 Nov 20, 2024

fabpot modified the milestones: 7.3, 7.4 May 26, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

[Messenger] support for multiple bindings on the same queue #38485

[Messenger] support for multiple bindings on the same queue #38485

inferont commented Oct 8, 2020

Uh oh!

Nyholm commented Oct 8, 2020

Uh oh!

inferont commented Oct 8, 2020

Uh oh!

Nyholm left a comment

Uh oh!

Nyholm Oct 8, 2020

Uh oh!

inferont Oct 9, 2020

Uh oh!

Nyholm Oct 8, 2020

Uh oh!

inferont Oct 9, 2020

Uh oh!

inferont left a comment

Uh oh!

inferont Oct 9, 2020

Uh oh!

inferont Oct 9, 2020

Uh oh!

fabpot commented Aug 7, 2022

Uh oh!

githoober commented Jan 8, 2023

Uh oh!

githoober commented Jan 13, 2023

Uh oh!

Uh oh!

Uh oh!

[Messenger] support for multiple bindings on the same queue #38485

Are you sure you want to change the base?

[Messenger] support for multiple bindings on the same queue #38485

Conversation

inferont commented Oct 8, 2020

Uh oh!

Nyholm commented Oct 8, 2020

Uh oh!

inferont commented Oct 8, 2020

Uh oh!

Nyholm left a comment

Choose a reason for hiding this comment

Uh oh!

Nyholm Oct 8, 2020

Choose a reason for hiding this comment

Uh oh!

inferont Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

Nyholm Oct 8, 2020

Choose a reason for hiding this comment

Uh oh!

inferont Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

inferont left a comment

Choose a reason for hiding this comment

Uh oh!

inferont Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

inferont Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

fabpot commented Aug 7, 2022

Uh oh!

githoober commented Jan 8, 2023

Uh oh!

githoober commented Jan 13, 2023

Uh oh!

Uh oh!