8000 [Messenger] [Sqs] Add `AddFifoStamp` middleware by tyx · Pull Request #48095 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Messenger] [Sqs] Add AddFifoStamp middleware #48095

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
Merged
merged 1 commit into from
Oct 19, 2023
Merged

[Messenger] [Sqs] Add AddFifoStamp middleware #48095

merged 1 commit into from
Oct 19, 2023

Conversation

tyx
Copy link
Contributor
@tyx tyx commented Nov 3, 2022 •
edited
Loading
Q A
Branch? 6.4
Bug fix? no
New feature? yes
Deprecations? no
License MIT
Doc PR on my way

Currently it is not very easy to use fifo stamp as you need to go down to the envelope when you often handle only message in your app.

We also think that the message should be responsible to choose its own groupId.

So we come with this very simple middleware and 2 interfaces. We use it in our own system since 3 months.

It allows to keep messenger concept outside our messages while continuing to benefit from it.

Example

To group message by user for example

Current

class SmsNotification {
    public function __construct(private string $userId) {}
}
//....
    $bus->dispatch(new SmsNotification($userId), [
        new AmazonSqsFifoStamp($userId)
    ]);

After

class SmsNotification implements WithMessageGroupId {
    public function __construct(private string $userId) {}
    
    public function messageGroupId() 
    {
        return $this->userId;
    }
}
//....
    $bus->dispatch(new SmsNotification($userId));

Usage

It can be enabled with this kind of config

# config/packages/messenger.yaml
framework:
  messenger:
    buses:
      event_bus:
        default_middleware:
          allow_no_handlers: true
        middleware:
          - 'Symfony\Component\Messenger\Bridge\AmazonSqs\Middleware\AddFifoStampMiddleware'

@carsonbot carsonbot added this to the 6.2 milestone Nov 3, 2022
@carsonbot carsonbot changed the title [Messenger][Sqs] Add AddFifoStamp middleware [Messenger] [Sqs] Add AddFifoStamp middleware Nov 3, 2022
@tyx tyx force-pushed the feat/fifo-stamp-middleware branch from 8c0a310 to 161d613 Compare November 3, 2022 13:50
@tyx tyx mentioned this pull request Nov 3, 2022
Closed
@welcoMattic welcoMattic modified the milestones: 6.2, 6.3 Nov 3, 2022
@carsonbot
Copy link

Hey!

I think @filkaris has recently worked with this code. Maybe they can help review this?

Cheers!

Carsonbot

@tyx
Copy link
Contributor Author
tyx commented Dec 13, 2022

Hello there !

I'll have some time soon to write some documentation but I'd like to be sure that the feature is interesting :D

@nicolas-grekas nicolas-grekas modified the milestones: 6.3, 6.4 May 23, 2023
@nicolas-grekas nicolas-grekas added the ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" label Oct 16, 2023
Nyholm
Nyholm approved these changes Oct 18, 2023
View reviewed changes
Copy link
Member
@Nyholm Nyholm left a comment

Choose a reason for hiding this comment

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

Thank you

I like this PR.

What are the pros/cons to use one interface instead of two? (and make the return type nullable)

@tyx
Copy link
Contributor Author
tyx commented Oct 18, 2023 •
edited
Loading

What are the pros/cons to use one interface instead of two? (and make the return type nullable)

Hi @Nyholm !

In fact, deduplication is "optionnal" in sqs world. For example in our situation we rarely use it.

According to the documentation

Providing the message deduplication ID
The producer should provide message deduplication ID values for each message in the following scenarios:
Messages sent with identical message bodies that Amazon SQS must treat as unique.
Messages sent with identical content but different message attributes that Amazon SQS must treat as unique.
Messages sent with different content (for example, retry counts included in the message body) that Amazon SQS must treat as duplicates.

https://docs.aws.amazon.com/en_gb/AWSSimpleQueueService/latest/SQSDeveloperGuide/using-messagededuplicationid-property.html

So 2 interfaces reduce the boilercode to write by the end user (aka empty method deduplicationId)

@Nyholm
Copy link
Member
Nyholm commented Oct 18, 2023

So 2 interfaces reduce the boilercode to write by the end user

Yeah, true. Thank you

Please add a doc block and I am super happy with this PR.

@tyx tyx force-pushed the feat/fifo-stamp-middleware branch from 161d613 to ca2d0d7 Compare October 18, 2023 14:36
@OskarStark OskarStark changed the title [Messenger] [Sqs] Add AddFifoStamp middleware [Messenger][Sqs] Add AddFifoStamp middleware Oct 18, 2023
@carsonbot carsonbot changed the title [Messenger][Sqs] Add AddFifoStamp middleware [Messenger] [Sqs] Add AddFifoStamp middleware Oct 18, 2023
nicolas-grekas
nicolas-grekas reviewed Oct 18, 2023
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

Can you please add a before/after to the description of your PR? How can one do today, and your proposal?

nicolas-grekas
nicolas-grekas approved these changes Oct 19, 2023
View reviewed changes
Copy link
Member
@nicolas-grekas nicolas-grekas left a comment

Choose a reason for hiding this comment

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

One minor comment and GTM!

@nicolas-grekas
Copy link
Member
nicolas-grekas commented Oct 19, 2023 •
edited
Loading

One last thing: please update the PR description to show how the middleware could be enabled.

@tyx tyx force-pushed the feat/fifo-stamp-middleware branch from fe008fa to 707b5a4 Compare October 19, 2023 14:26
@keradus keradus mentioned this pull request Oct 19, 2023
Merged
nicolas-grekas added a commit that referenced this pull request Oct 19, 2023
@nicolas-grekas
minor #52174 DX: nullable_type_declaration (keradus)
6319e5d 
This PR was merged into the 6.4 branch.

Discussion
----------

DX: nullable_type_declaration

| Q             | A
| ------------- | ---
| Branch?       | 6.4
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | n/a
| License       | MIT

https://cs.symfony.com/doc/rules/language_construct/nullable_type_declaration.html#example-1

requested in #48095 (comment)

I do not put this rule in ruleset, as it requires PHP 8 - if you want to move it to ``@Symfony`` ruleset, let me know

There is concern how to have this rule configured and have PHPUnit bridge supporting 7.4, but I think I will leave it unresolved for now.

Commits
-------

6aff9d9 DX: nullable_type_declaration
@nicolas-grekas nicolas-grekas force-pushed the feat/fifo-stamp-middleware branch from 707b5a4 to c424da4 Compare October 19, 2023 14:58
@nicolas-grekas
Copy link
Member

Thank you @tyx.

@nicolas-grekas nicolas-grekas merged commit e942c1c into symfony:6.4 Oct 19, 2023
@nicolas-grekas nicolas-grekas mentioned this pull request Oct 19, 2023
Closed
@tyx tyx deleted the feat/fifo-stamp-middleware branch October 19, 2023 16:32
This was referenced Oct 21, 2023
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@Nyholm Nyholm Nyholm approved these changes

@keradus keradus keradus left review comments

@OskarStark OskarStark OskarStark approved these changes

@xabbuh xabbuh Awaiting requested review from xabbuh

@lyrixx lyrixx Awaiting requested review from lyrixx

@dunglas dunglas Awaiting requested review from dunglas

@yceruto yceruto Awaiting requested review from yceruto

@welcoMattic welcoMattic Awaiting requested review from welcoMattic

@chalasr chalasr Awaiting requested review from chalasr

@jderusse jderusse Awaiting requested review from jderusse

Assignees
No one assigned
Labels
Feature Messenger ❄️ Feature Freeze Important Pull Requests to finish before the next Symfony "feature freeze" Status: Reviewed
Projects
None yet
Milestone
6.4
Development

Successfully merging this pull request may close these issues.
7 participants
@tyx @carsonbot @Nyholm @nicolas-grekas @OskarStark @keradus @welcoMattic
0