Fix sqs message attribute#140
Conversation
* Support SQS message group id * derive Generic instances * use ApplicativeDo * haddock; changelog
JackKelly-Bellroy
left a comment
There was a problem hiding this comment.
Beware copying from Amazonka too closely, it's constrained by what botocore says about data types when its generator emits the Haskell record definitions. We had to write amazonka-dynamodb-attributevalue to provide an actual sum type to make DynamoDB more ergonomic to use. Would it be worth doing similar here?
| binaryValue :: Maybe ByteString, | ||
| stringListValues :: [Text], | ||
| binaryListValues :: [ByteString], | ||
| dataType :: Text |
There was a problem hiding this comment.
Is this actually a sum type and should it be modeled as such?
There was a problem hiding this comment.
That's a good point, but dataType is slightly more complex than a closed enum. It can be String, Number, Binary, or one of the three plus .custom-label, for example String.hello-world. We could have something like:
data LabeledMessageAttributeValue = LableledMessageAttributeValue
{ dataTypeLabel :: Text,
value :: MessageAttributeValue
}
data MessageAttributeValue = String Text | Binary ByteString | Number ScientificDo you have any suggestion about how to name these things?
There was a problem hiding this comment.
From a discussion @kokobd and I had over voice: we decided on the call it's probably best to keep this mapping as close to what SQS invokes, so library users can see how to use it. But now, looking at the snippet you've posted above, I think something like that would be quite workable, perhaps with customTypeLabel :: Maybe Text:
data MessageAttribute = MessageAttribute
{ customTypeLabel :: Maybe Text
, value :: MessageAttributeValue
}
data MessageAttributeValue
= Binary ByteString
| Number Text
| String TextThere was a problem hiding this comment.
I agree that we should make types ADT/Haskell-like where they fit. It's unfortunate that this is so often made so difficult :)
From here:
Number – Number attributes can store positive or negative numerical values. A number can have up to 38 digits of precision, and it can be between 10^-128 and 10^+126.
So... kind of odd. More precision than an f64, less exponent range than an f128.
Since we can't match the domain exactly, we'll need to make it larger. I think my lean would be to use a Number128 or a Scientific. Both are strictly larger, but not quite so large as Text. Other than that, this last recommendation looks good to me, and I'm not too attached to any specific number type approach.
Based on #139. The diff will be more clear once #139 is merged.
The current type of
messageAttributesinSQSEventis not correct. It shouldn't beMap Text Text. This PR adds a newMessageAttributeValuetype similar to amazonka's version.