Skip to content

[Messenger] Comply with Amazon SQS requirements for message body #54920

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

Merged
merged 1 commit into from
Jun 9, 2024

Conversation

VincentLanglet
Copy link
Contributor

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #49844
License MIT

@carsonbot carsonbot added this to the 5.4 milestone May 14, 2024
@VincentLanglet VincentLanglet force-pushed the amazonSerializer branch 5 times, most recently from f61a786 to b1f0c65 Compare May 14, 2024 10:58
@VincentLanglet
Copy link
Contributor Author

Unit Tests / Unit Tests (8.2)

are failing because they are using

Installing symfony/messenger (6.4.x-dev 34b7fd3): Cloning 34b7fd38c9

and not the PhpSerializer were I added shouldBeEncoded.

And fabbot.io is failing with unrelated changes.

@VincentLanglet VincentLanglet requested a review from OskarStark May 15, 2024 09:39
@VincentLanglet VincentLanglet force-pushed the amazonSerializer branch 4 times, most recently from 9b7f731 to 48b3af7 Compare May 16, 2024 06:58
@@ -1332,6 +1333,10 @@ private function addWebLinkSection(ArrayNodeDefinition $rootNode, callable $enab

private function addMessengerSection(ArrayNodeDefinition $rootNode, callable $enableIfStandalone)
{
$defaultSerializer = class_exists(AmazonSqsSerializer::class)
Copy link
Member

Choose a reason for hiding this comment

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

Hum, I missed this part: changing the serializer service for all transports.
I don't think that's the correct approach.
Instead, I see two ways:

  1. patch the native serializer
  2. fix the encoding in AmazonSqsSender, just after $encodedMessage = $this->serializer->encode($envelope);

Option 2. looks the most appropriate to me.

Copy link
Contributor Author

@VincentLanglet VincentLanglet May 16, 2024

Choose a reason for hiding this comment

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

Yeah, I wasn't sure about the best strategy for this.

The solution 1 leads to an impact for all the user of PHPSerializer even if they don't use AmazonSQS. There is no reason to follow AmazonSQS needs if you don't use it (so I tried to follow #49844 (comment))

The solution 2 looks interesting, but if I add an extra base64_encode call after $encodedMessage = $this->serializer->encode($envelope);, how can I be sure it will be decoded by the serializer (since any custom Serializer can be used) ?

So I thought changing the defaut serializer was the right way for AmazonSqs users.

Copy link
Member

Choose a reason for hiding this comment

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

The solution 2 looks interesting, but if I add an extra base64_encode call, how can I be sure it will be decoded by the serializer (since any custom Serializer can be used) ?

If a custom serializer is used, it has to comply with Amazon's requirements, so this extra logic we're talking about shouldn't be triggered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could add a Header to tell AmazonSqsReceiver it require an extra base64_decode.

WDYT @nicolas-grekas should I add the commit: https://github.com/VincentLanglet/symfony/compare/amazonSerializer...VincentLanglet:symfony:amazonSerializer-2?expand=1 or should I let the PR like this ?

@VincentLanglet
Copy link
Contributor Author

Do you have time for a new review @OskarStark @nicolas-grekas ?

Thanks a lot.

@fabpot fabpot force-pushed the amazonSerializer branch from a1a6e79 to 0d441bf Compare June 9, 2024 06:58
@fabpot
Copy link
Member

fabpot commented Jun 9, 2024

Thank you @VincentLanglet.

@fabpot fabpot merged commit 4d0aeed into symfony:5.4 Jun 9, 2024
9 of 12 checks passed
@fabpot fabpot mentioned this pull request Jun 28, 2024
This was referenced Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants