[7.x] Ensure queues are only suffixed once

[roberto-aguilar]

This is a follow up of #31784. We found a problem with the following scenario:

If the SQS_SUFFIX is defined as production and staging in each environment respectively and the vapor manifest looks like this:

environments:
    production:
        queues:
            - default-production
            - emails-production
    staging:
        queues:
            - default-staging
            - emails-staging

The first queue is going to be picked as the default SQS queue and assigned in the config/queue.php

'sqs' => [
    'driver' => 'sqs',
    'queue' => 'default-production', // or default-staging depending of the environment
    'suffix' => '-production', // or -staging depending of the environment
],

If you send a job to a queue explicitly, it is going to be resolved correctly:

// Depending of the environment, this resolves the queue as:
//
// https://sqs.someregion.amazonaws.com/account-id/emails-production
// https://sqs.someregion.amazonaws.com/account-id/emails-staging
dispatch(new SendWelcomeEmail($user))->onQueue('emails');

// This explicit default queue usage also resolves as:
//
// https://sqs.someregion.amazonaws.com/account-id/default-production
// https://sqs.someregion.amazonaws.com/account-id/default-staging
dispatch(new SendWelcomeEmail($user))->onQueue('default');

But if you try to rely on the default queue, it suffixes the queue twice:

// This resolves the queue as
// https://sqs.someregion.amazonaws.com/account-id/default-production-production
// https://sqs.someregion.amazonaws.com/account-id/default-staging-staging
dispatch(new SendWelcomeEmail($user));

This happens because at runtime in Vapor, we don't control the default queue name and because it is already suffixed, it gets suffixed twice during the queue name resolution.

This PR fixes that by ensuring queues get only suffixed once, and does it in a non breaking way for applications not relying on this suffix feature.

// With this PR, default queues are resolved correctly
//
// https://sqs.someregion.amazonaws.com/account-id/default-production
// https://sqs.someregion.amazonaws.com/account-id/default-staging
dispatch(new SendWelcomeEmail($user));

[GrahamCampbell]

Hmm, I don't think this is right. Surely we wanna always apply the suffix again. What if the queue name genuinely happens to end with the suffix?

[taylorotwell]

Why would you put the suffix in your Vapor manifest at all? Let the framework add it using the environment variable.

[roberto-aguilar]

@taylorotwell

I need to declare the queues with the suffix on the manifest because otherwise the queues will not be created at all by Vapor.

For example:

environments:
    production:
        queues:
            - default
            - emails
    staging:
        queues:
            - default
            - emails

That manifest is going to create only two queues for the whole project and the goal is to have two queues per environment.

Also, i'm not sure if Vapor would assign the two lambda functions to each queue in this case, which would defeat the purpose of having separate queues per environment on the first place, but will also be dangerous considering that the same job would be triggered on production and staging. Otherwise, if only one is assigned, either the production or the staging lambda will not be triggered at all because it was never assigned to the queue.

That why this declaration is necessary:

environments:
    production:
        queues:
            - default-production
            - emails-production
    staging:
        queues:
            - default-staging
            - emails-staging

This let Vapor rightly create the four queues and assign the right lambdas to each one.

[roberto-aguilar]

@GrahamCampbell

That's a fair point, i don't have a clean solution for that scenario.

What i think is that will be more likely to have the scenario that i'm presenting where the queues are already suffixed and you need to prevent the double suffix than a queue that is suffixed and you need to enforce the double suffix. But being fair, that's just my opinion and i don't have data to support it, i may be wrong.

[roberto-aguilar]

Thinking more about it and given that this is a problem that only happens in Vapor due the lack of control of the SQS_QUEUE environment variable, i would be open to move this PR to laravel/vapor-core and make this check in the VaporQueue class.

Would you be open to that, @taylorotwell?

[taylorotwell]

You do have control over that environment variable. Any environment variable you specify will override any Vapor provided ones.

[roberto-aguilar]

I didn't know about that, thanks for clarifying that environment variables precedence. Since i have been bite by how the reserved environment variables behave, i have tried to stay away of variables that Vapor manage for me.

Is just that i keep thinking, wouldn't it improve the develop experience a lot to let Vapor continue managing the SQS environment variables himself instead of making the developers define just the default queue name for the sole purpose of avoiding the double suffix?

If a suffix is provided, it is because the queues are already named that way, it will be hardly unexpected behavior to see the framework preventing a double suffix problem than to see it applied twice and have the need to define the queue name without a suffix.

[taylorotwell]

Have you considered using the Str::finish method?

[roberto-aguilar]

It looks way better now, thank you for the suggestion! ❤️