SQS: Revise stream element data model

[RustedBones]

Pull Request Checklist

• Have you read through the contributor guidelines and contributor advice?
• Have you signed the Lightbend CLA?
• Have you updated the documentation?
• Have you added tests for any changed functionality?

Fixes

Fixes #625

Purpose

Improve SQS batch operations

Background Context

After the AWS SDK update, changes in the AWS models can make the API simpler and cleaner:

• No need to propagate FifoIdentifiers anymore
• Separation between request metadata (HTTP request info) and content metadata

[ennru]

Thank you for modernising this with the latest possibilities from the AWS SDK.
As we now want to promise binary compatibility between releases, this PR failed the MiMa check. Do you see a way to keep the RC1 API around while improving this?

(run mimaReportBinaryIssues to get the MiMa report locally)

[ennru]

I've looked closer at your propsal and added the necessary MiMa exclusions, I agree that FifoMessageIdentifiers should go away.
SqsAckFlow.apply could be a non-API-breaking.

[ennru]

Great to remove the unused Future-wrapping.

[ennru]

This PR requires a rebase to incorporate changes from #1606.

[RustedBones]

Thanks @ennru.
By going through the PR once more, I realize I got rid of the FifoMessageIdentifiers a bit too early. The goal of this class was to propagate the GroupId and DeduplicationId to downstream in case the user wants to do some special operations depending on those values (like for the message, we propagate it downstream though SQS only gives the md5 as a response).

In order to pass all information to downstream, we can encapsulate the SendMessageRequest into the SqsPublishResult. Then we can safely remove the FifoMessageIdentifiers and this makes it more symmetric like the SqsAckFlow which also contains the MessageAction.

[ennru]

Great that you took another look.
I wonder if we wouldn't be able to get rid of the generics altogether if we'd have separate stream element types for the flat and for the grouped flows?

[ennru]

Let's give this the time it needs and release Alpakka 1.0 anyway. I've created #1621 to mark the API as "may change".

[ennru]

Picking this up again.

To reduce the risk for "ToManyTypeParametersException" I'd like to propose to introduce specific subclasses of SqsPublishResult:

final class SqsPublishSingleResult @InternalApi private[sqs] (
    responseMetadata: SqsResponseMetadata,
    metadata: SendMessageResponse,
    request: SendMessageRequest
) extends SqsPublishResult[SendMessageResponse](responseMetadata, metadata, request)

final class SqsPublishBatchResult @InternalApi private[sqs] (
    responseMetadata: SqsResponseMetadata,
    metadata: SendMessageBatchResultEntry,
    request: SendMessageRequest
) extends SqsPublishResult[SendMessageBatchResultEntry](responseMetadata, metadata, request)

Names can be improved probably, but the API looks simpler with this.

WDYT?

[RustedBones]

Are we risking a ToManyTypeParametersException since the constructor visibility is package?

If we do this we should also do the same with SqsAckResult with ChangeMessageVisibilityResponse, ChangeMessageVisibilityBatchResultEntry, DeleteMessageResponse, DeleteMessageBatchResultEntry and SqsAckResult[Nothing] (returned in a case of MessageAction.Ignore). That make all the type definitions a bit tedious IMHO.

[ennru]

Yes, you're right.

[RustedBones]

Thinking about it again, it's probably better to give a sealed abstract class along with the possible implementations for the SQS model

Right now, the output type from the SqsAckFlow.grouped is a SqsAckResult[SdkPojo] which is contains too many potential implementations due to the SdkPojo (not only used for SQS) and would oblige the users to always provide a default case when pattern matching to be exhaustive. Same problem applies with SqsResponse in a smaller scale.

In addition, it would also be nice to provide unapply on the SQS model for the scala users

[ennru]

Yes, that is what I am looking for. SdkPojo is too broad and we have just two specialisations of it.

[RustedBones]

I've tried to get rid of the type parameter from the publish and ack results to give a clean API:

SqsPublishFlow.apply: Flow[SendMessageRequest, PublishResult, NotUsed]
SqsPublishFlow.grouped: Flow[SendMessageRequest, PublishResultEntry, NotUsed]

SqsAckFlow.apply: Flow[MessageAction, SqsAckResult, NotUsed]
SqsAckFlow.grouped: Flow[MessageAction, SqsAckResultEntry, NotUsed]

Ignore message action is now filtered by the ack flows and don't forward any message downstream

In all the SqsResult there is an accessor to the SqsResponseMetadata but this field is not taken in account for the equals and toString as AWS does it on its own models.

There is now no traces of the 'unbounded' SqsResponse and SdkPojo types.

The only drawback is on the SqsAckResult and SqsAckResultEntry which both contains a def result: Any when the user doesn't pattern-match/down cast to the sub-class implementation.

[ennru]

This looks much better, the types are simple and self-explaining.

I'm a bit sceptic to filtering the Ignore messages, if that's what users need they can use filter. I foresee we'd like to add some pass-through support going forward and would need a message going through for that.

To leave out the field from equals makes sense, but you could add a little note on that.

[ennru]

Nice.

[RustedBones]

I've added Ignore to the SqsAck.
Instead of using Option for the result and requestMetadata, I return NotUsed and an empty metadata object.

[ennru]

We're getting there...

[2m]

LGTM

[ennru]

LGTM, but I might be part of this. I'll squash it locally and merge it.

[ennru]

Thank you for this long-coming improvement!