Skip to content

Use DI & SRP to make Amazon SQS Messenger more extensible #38640

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

Closed
jacekwilczynski opened this issue Oct 20, 2020 · 6 comments · Fixed by #38846
Closed

Use DI & SRP to make Amazon SQS Messenger more extensible #38640

jacekwilczynski opened this issue Oct 20, 2020 · 6 comments · Fixed by #38846

Comments

@jacekwilczynski
Copy link
Contributor

jacekwilczynski commented Oct 20, 2020

Description

This issue is here in case #38639 is rejected and I have to implement ListableReceiverInterface in my application code with decoration or inheritance. However, it might be useful in other scenarios as well.

The amazon-sqs-messenger component instantiates dependencies in dependants, which is a problem for extending the component.

  • AmazonSqsReceiver and AmazonSqsSender are instantiated in AmazonSqsTransport. -> Please allow passing them via the constructor. Can make the parameters optional and fall back to the default receiver&sender implementations not to force other users to change their calls.
  • The Connection::fromDsn static method contains configuration processing logic that could be used also for other purposes than instantiating a Connection. For example, someone may want to use an identical instance of SqsClient to the one hidden within the Connection, or even the same instance - so one would create the SqsClient on their own and pass it to the Connection's main constructor. -> Please extract a method (or a class) to handle just configuration processing without instantiating any services. See example below.

An extreme solution would be to make all private fields and methods in all classes protected and mark Connection as not @internal but I guess what is private and internal is private and internal for a reason so I'm not really asking for that (though it wouldn't hurt me :-)).

I can write the implementation but would like to get an approval of the ticket first.

Example

In a custom implementation of AmazonSqsTransportFactoryInterface::createTransport:

Before

$transport = new AmazonSqsTransport(Connection::fromDsn($dsn, $options), $serializer);

After (not necessarily exactly that):

[$connectionConfiguration, $clientConfiguration] = ConfigurationFactory::processConfiguration($dsn, $options);
$client = new SqsClient($clientConfiguration);
$connection = new Connection($connectionConfiguration, $client);

$transport = new AmazonSqsTransport(
    $connection,
    $serializer,
    new MyExtendedSqsReceiver($connection, $serializer, $client), // now I can use the client to send some `receiveMessage` requests
    new AmazonSqsSender($connection, $serializer)
);

The Connection::fromDsn method could still exist but be refactored to use ConfigurationFactory::processConfiguration. This way, the setup shown in the "before" code snippet would still work.

@chalasr
Copy link
Member

chalasr commented Oct 22, 2020

I'm 👍 to make it easier to decorate some pieces of the transports. Are you willing to work on the topic?

@jacekwilczynski
Copy link
Contributor Author

I'm 👍 to make it easier to decorate some pieces of the transports. Are you willing to work on the topic?

Yes, with pleasure :-) I'll get to it after I make a PR for #38639.

@natepage
Copy link
Contributor

Hi guys, would that feature allow us to extend/override the Connection as well?

I believe that most of the time we create the SQS queue upfront, we configure messenger transport using a DSN then we deploy the code but still.. Each time we send the first message to SQS we have to request it to get the queue URL 😄 I know it's no big deal but still I think that it would be nice to bypass that request to improve the process.

Is that something this feature could help with, or what I'm talking about is simply updating the existing Connection to have an queue_url option to avoid requesting it in the same spirit as auto_setup: false to prevent creating the queue automatically?

@chalasr
Copy link
Member

chalasr commented Oct 27, 2020

@natepage Connection should remain an internal implementation detail.
About your need, the transport already supports passing a SQS queue url as dsn since #37306, but it still calls getQueueUrl() internally.
Feel free to open a dedicated issue or PR for that, otherwise I'll do it asap.

@natepage
Copy link
Contributor

@chalasr Thank you for your reply, I'm gonna create a dedicated issue, I'm happy to work on a PR but I would prefer asking some questions first about how you would see that implemented, cheers.

@natepage
Copy link
Contributor

@chalasr I've created the issue #38849

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment