Skip to content
This repository has been archived by the owner on Jan 19, 2022. It is now read-only.

Add support for SQS custom data types #410

Closed
AbdulWahedHaiderzadah1217 opened this issue Feb 7, 2019 · 8 comments
Closed

Add support for SQS custom data types #410

AbdulWahedHaiderzadah1217 opened this issue Feb 7, 2019 · 8 comments
Labels
component: sqs SQS integration related issue type: enhancement A general enhancement
Milestone

Comments

@AbdulWahedHaiderzadah1217

Currently custom data types are silently ignored during transformation from MessageAttributes to MessageHeaders in QueueMessageUtils. Either custom data types should be supported fully or an Exception should be thrown.
If somebody uses SQS custom data type, which currently can only hold String or Binary values, it will get omitted in QueueMessageUtils and finding out why can be very time consumeing since it is omitted silently.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Feb 7, 2019
@maciejwalkowiak maciejwalkowiak added the component: sqs SQS integration related issue label May 29, 2020
@maciejwalkowiak maciejwalkowiak added type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged labels Jun 6, 2020
@maciejwalkowiak maciejwalkowiak added this to the 2.3 milestone Jun 6, 2020
@maciejwalkowiak
Copy link
Contributor

@AbdulWahedHaiderzadah1217 how do you see this support should look like?

@voytech
Copy link
Contributor

voytech commented Jun 28, 2020

Maybe when there is a custom data type like e.g. binary.png - a value for header should be just wrapper like: CustomTypeMessageAttribute with fields: type: Binary, typeLabel: png, value - wrapped value handled in the same way as it is currently for basic Binary (or String or Number) type.
This way we are not loosing this header value ( current implementation would skip it). We are also not loosing info about custom type extension ?

@voytech
Copy link
Contributor

voytech commented Jun 29, 2020

@maciejwalkowiak, @AbdulWahedHaiderzadah1217 if You are happy with solution with CustomTypeMessageAttribute wrapper then I can make it working that way...

@voytech
Copy link
Contributor

voytech commented Jul 16, 2020

@maciejwalkowiak, I'd like to help with that one. IMHO its quite important to have it done, as lack of proper handling of custom data types may force people to workarounds. I could create initial PR. I think that for this one, I will have to extend documentation a bit to mention about CustomTypeMessageAttribute (if decide to go in that direction). Let me know If I should/can grab this one.

@maciejwalkowiak
Copy link
Contributor

Go for it @voytech. I can't tell for sure now but the idea with wrapper sounds legit.

@voytech
Copy link
Contributor

voytech commented Jul 17, 2020

After thinking a bit more, I am not sure if using CustomTypeMessageAttribute as value wrapper is the way to go:

  1. Solution used for Number type currently takes type labels only as a guidance for resolving value to Java number types (eventually label is lost). This prevents from switching to CustomTypeMessageAttribute (it would be breaking change then)
  2. Using CustomTypeMessageAttribute still is possible for Binary and String (only when label suffix detected) and I have submitted PR with that approach, but I think it is inconsistent with how Number type with labels are handled. Most preferred solution would be if all types with label are treated the same way - and as long as they can not use CustomTypeMessageAttribute approach - Maybe dropping CustomTypeMessageAttribute and using unwrapped raw Binary (ByteBuffer) or String value straight as header value (with skipping type label hint from MessageAttribute) is the way to go (for 2.3) ?
  3. On the major version change, contract for handling MessageAttributes could be more aligned to what AWS API offers. I mean - MessageAttributes could be mapped in header value always as MessageAttributeValue instance, no matter whether it is of type with custom label or no. This would reflect things from AWS API fully without loosing anything - but can be done only as a breaking change, so not for 2.3 version.

@maciejwalkowiak
Copy link
Contributor

What if we go with 2. but with addition that for Number where custom type does not match any of the known ones we already support, we return CustomTypeMessageAttribute?

@voytech
Copy link
Contributor

voytech commented Jul 24, 2020

You mean:

  1. To use CustomTypeMessageAttribute only for unsupported type labels for Numbers ? And for the rest (Binary, String) drop CustomTypeMessageAttribute and use just value?

Or:

  1. In addition to CustomTypeMessageAttribute for Binary and String, to add also CustomTypeMessageAttribute for Number where possible (for unknown/unresolvable type labels)

I think both are equally ok (should be better than is now) The only thing that I personally don't like here - is lack of consistency in that sometimes we are using CustomTypeMessageAttribute for MessageAttributes with labeled types, but there are situations for Number with labelled type where we cannot use CustomTypeMessageAttribute (number type labels for primitives/Java Number classses already supported).

Scenario:
What if someone wants to publish to SQS (from app without spring-cloud-aws integration) and he is willing to use custom type labels. The consumer app uses spring-cloud-aws, and someone sees that there is a wrapper 'CustomTypeMessageAttribute'. Suppose he expects CustomTypeMessageAttribute for all MessageAttributes with labelled types - I think it becomes tricky in situation where someone accidentally will use the same scheme (or its superset) for labelling Number type as the scheme automatically supported by spring-cloud-aws now. If he fails to discover that case - he would cast everything to CustomTypeMessageAttribute, but sometimes it will be just a java Number e.g.

Because of above - for 2.3 - I think I would prefer not to use CustomTypeMessageAttribute at all. I would just support Binary and String with custom type labels not fully - by unwrapping ByteBuffer for Binary and String value, putting them straight as header value (with loosing custom type label info). For Numbers support is present.
(minimal PR partially solving issue: not omitting attribs with custom types + lack of full support) - #637)

Then I think I would consider (on major version e.g. 3) to add MessageAttributeValue (not CustomTypeMessageAttribute) for all MessageAttributes. And getMessageAttribute method on SqsMessageHeaders (If SqsMessageHeaders are going to stay).

voytech added a commit to voytech/spring-cloud-aws that referenced this issue Jul 25, 2020
…ributes. Fix spring-atticgh-410 [v2].

This is another approach to solve spring-atticgh-410.
First approach is here: spring-attic#633.
Original issue and discussion is here: spring-attic#410
voytech added a commit to voytech/spring-cloud-aws that referenced this issue Jul 27, 2020
voytech added a commit to voytech/spring-cloud-aws that referenced this issue Jul 28, 2020
voytech added a commit to voytech/spring-cloud-aws that referenced this issue Jul 28, 2020
maciejwalkowiak pushed a commit to maciejwalkowiak/spring-cloud-aws that referenced this issue Oct 15, 2020
maciejwalkowiak pushed a commit to maciejwalkowiak/spring-cloud-aws that referenced this issue Oct 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
component: sqs SQS integration related issue type: enhancement A general enhancement
Development

No branches or pull requests

4 participants