Improve SQS Transport Resolver to match standard configuration #98
Conversation
|
I added some tests and refactor a class with an interface. |
|
Hi @mnapoli Could you please take a moment to review this PR when you have a chance? Thanks a lot in advance! Best, |
|
|
||
| use InvalidArgumentException; | ||
|
|
||
| interface SQSMessengerTransportConfigurationInterface |
There was a problem hiding this comment.
Is there a reason to introduce a new interface?
If there's no specific reason I'd rather keep the API surface as small as possible to facilitate maintenance
There was a problem hiding this comment.
This interface would have allowed me to implement these changes with less code overhead. It could still be used to implement a more complex strategy, such as changing the transport based on the message content or other parameters.
There was a problem hiding this comment.
OK thanks for the details, let's remove it until it's actually needed please
| private const TRANSPORT_PROTOCOL = 'sqs://'; | ||
|
|
||
| public function __construct( | ||
| private MessengerTransportConfiguration $configurationProvider |
There was a problem hiding this comment.
What happens to the existing class? I see it's no longer used in this PR.
- should we delete the class?
- are we preserving backwards compatibility with the current behavior?
There was a problem hiding this comment.
The MessengerTransportConfiguration class is still used by the SnsTransportNameResolver class for transport via SNS. It should be preserved.
There was a problem hiding this comment.
Thanks! what about the second question?
There was a problem hiding this comment.
In cases where the user did not use the queue URL or manually specified the transport name for processing the event, there will be no impact.
Previously, omitting the manual transport name and using only the SQS queue URL in the transport DSN would have raised an InvalidArgumentException.
Now, the transport matching the SQS queue that originated the event will be returned automatically. This change should only affect users who deliberately caused errors, which is unlikely to be a valid use case.
|
|
||
| use InvalidArgumentException; | ||
|
|
||
| final class SQSMessengerTransportConfiguration implements SQSMessengerTransportConfigurationInterface |
There was a problem hiding this comment.
If this new class is only ever used in SqsTransportNameResolver (as far as I can tell), can you inline the logic in SqsTransportNameResolver?
The bridge is already more complex than it should be, making it hard to review and merge improvements (like this PR) because it requires time to re-understand everything. I'd prefer we actually simplify it, even if it means we have larger classes.
There was a problem hiding this comment.
I'm trying to do it during the day or tomorrow
|
Hi @mnapoli, Would you mind taking a final look at this PR when you get a moment? I've implemented all the requested changes, and I’d really appreciate your approval to get it merged. This will help me reduce the number of Lambdas used across my projects and consequently lower the security scan costs associated with their deployment. Let me know if anything else is needed! Thanks a lot in advance, Best, |
|
Thanks let's give this a try |
2 participants
This PR updates the transport resolution logic to support SQS DSNs formatted according to the Symfony documentation (e.g., https://sqs.eu-west-3.amazonaws.com/123456789012/messages). This allows seamless configuration when using multiple SQS queues and transports.