Skip to content

Support SQS messages with Type Binary and Number #61

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
wants to merge 1 commit into from

Conversation

candrews
Copy link
Contributor

The SQS documentation indicates that messages with "Type" of "Number" and "Binary" are valid, see https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGuide/sqs-message-attributes.html

This PR adds support for these "Type" values. According to https://docs.aws.amazon.com/AWSSimpleQueueService/latest/SQSDeveloperGuide/sqs-message-attributes.html

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.

That means that this library cannot internally handle values as the largest JMS allowed numeric type (which is Double see https://docs.oracle.com/javaee/6/api/javax/jms/Message.html ) is not big enough to handle such values. Therefore, this PR treats "Number" values internally as strings, converting them upon request when methods such as Message.getDouble(String) are invoked.

The same approach is used for "Binary" - the value is held in a String. ByteBuffer or byte[], for example, as those are not JMS permitted data types, from https://docs.oracle.com/javaee/6/api/javax/jms/Message.html :

The getObjectProperty method only returns values of class Boolean, Byte, Short, Integer, Long, Float, Double, and String.

See #60

@candrews
Copy link
Contributor Author

@robin-aws pointed out that the suggested change for type "Binary" is incorrect - it gets the Binary data from the "StringValue" when binary data is actually stored in the "BinaryValue" field.

@ghost
Copy link

ghost commented Dec 14, 2018

Is this project still being maintained? The Number support is really a SEVERE issue, because even amazon console allows to add Number without a custom data type string after it.

Please merge.

@robin-aws
Copy link
Collaborator

Hi @StoreBot - it is absolutely still maintained! I'm almost finished addressing some internal build issues that have made accepting pull requests difficult in the past, at which point I'll tackle this issue as it's clearly blocking several people.

@candrews and I have discussed this offline and as per his comment above the PR it still needs some tweaking. I will review the related issues/PRs and propose a solution next week.

@robin-aws
Copy link
Collaborator

@candrews - could you separate this into two PRs? After reviewing the JMS specs I'm happy with the "Number" fix, given the allowances for converting to and from String values, and the fact that attributes are not really strongly typed (i.e. there's no way to ask for "the" type of an attribute). The "Binary" one needs more thought though.

@candrews
Copy link
Contributor Author

could you separate this into two PRs?

The binary commit builds on the work done in the number commit - if I separate the 2 commits into 2 PRs, there will be conflicts. So I pulled out the number commit into a new PR, #71, and I left the number commit here in this PR, so when you merge the number commit, this PR will still work for the binary change and not have conflicts.

@robin-aws
Copy link
Collaborator

I've merged #71, so this PR will just be for the binary commit now.

@candrews
Copy link
Contributor Author

I've rebased this PR fixing the conflict.
@robin-aws what do you, is this merge-able?

@robin-aws
Copy link
Collaborator

Hi Craig,

No I'm afraid not, it won't actually work - as per your comment above (May 22), "it gets the Binary data from the "StringValue" when binary data is actually stored in the "BinaryValue" field." I had to re-read the history to recall that myself. :)

To support this through JMS, there will have to be some kind of configuration within this client to specify how to read the ByteBuffer from getBinaryValue() as a String. Perhaps it could default to Base64? I'm open to suggestion here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants