Structured Message Decoder #44140

ibrahimrabab · 2025-02-11T00:53:13Z

Description

Please add an informative description that covers that changes made by the pull request and link all relevant issues.

If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.

All SDK Contribution checklist:

The pull request does not introduce [breaking changes]
CHANGELOG is updated for new features, bug fixes or other significant changes.
I have read the contribution guidelines.

General Guidelines and Best Practices

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

…sary comments

…om/Azure/azure-sdk-for-java into structuredMessageDecoder2

ibrandes · 2025-02-11T01:20:19Z

...e-common/src/main/java/com/azure/storage/common/implementation/StructuredMessageDecoder.java

+        return decodedContent.toByteArray();
+    }
+
+    public byte[] decodeFully() throws IOException {


maybe this can be an overload?

Yeah I'm curious if we even need this method. It will probably depend on how the download integration goes but if we don't have a strong reason for it now, let's remove it as we can always add it back if we decide we need it.

ibrandes · 2025-02-11T01:37:15Z

...e-common/src/main/java/com/azure/storage/common/implementation/StructuredMessageDecoder.java

+    private static final int DEFAULT_MESSAGE_VERSION = 1;
+    private static final int V1_HEADER_LENGTH = 13;
+    private static final int V1_SEGMENT_HEADER_LENGTH = 10;
+    private static final int CRC64_LENGTH = 8;
+


do we want to put these in a constants file to share between the encoder and decoder?

I like this idea. We should try to consolidate once the encoder PR gets merged.

ibrandes · 2025-02-11T01:42:00Z

...e-common/src/main/java/com/azure/storage/common/implementation/StructuredMessageDecoder.java

+
+    public StructuredMessageDecoder(ByteBuffer inputBuffer) {
+        this.buffer = inputBuffer.order(ByteOrder.LITTLE_ENDIAN);
+        readMessageHeader();


rather than reading the message header during initialization, I think it makes more sense to keep all reads in the decode method and just check if the message offset is 0

I agree. Mainly when we initialize the class, it is not intuitive that it would already start decoding/consuming the input buffer. I'd imagine more of the expectation would be that we consume from the input buffer when decode is explicitly called.

kyleknap

Nice! This looks like a good start. I just had feedback on the initial API we should try to shoot for. After we get the public interfaces in a spot we like, I plan to look more closely into the tests and more into the details of the decoding implementation in follow up reviews.

kyleknap · 2025-02-12T23:14:00Z

...e-common/src/main/java/com/azure/storage/common/implementation/StructuredMessageDecoder.java

+    private static final int CRC64_LENGTH = 8;
+
+    private int messageVersion;
+    private int messageLength;


We should probably keep this a long to match the type that is supported when using structured message. Technically we should even be treating it as an unsigned long for any comparisons as well.

kyleknap · 2025-02-12T23:20:58Z

...e-common/src/main/java/com/azure/storage/common/implementation/StructuredMessageDecoder.java

+        }
+
+        messageLength = (int) buffer.getLong();
+        flags = StructuredMessageFlags.fromValue(Short.toUnsignedInt(buffer.getShort()));


If we receive flags that are not part of our enumerated flags, I'm assuming this will be validated? It might be worth adding a test for it.

kyleknap · 2025-02-12T23:26:25Z

...e-common/src/main/java/com/azure/storage/common/implementation/StructuredMessageDecoder.java

+
+    public StructuredMessageDecoder(ByteBuffer inputBuffer) {
+        this.buffer = inputBuffer.order(ByteOrder.LITTLE_ENDIAN);
+        readMessageHeader();


I agree. Mainly when we initialize the class, it is not intuitive that it would already start decoding/consuming the input buffer. I'd imagine more of the expectation would be that we consume from the input buffer when decode is explicitly called.

kyleknap · 2025-02-12T23:37:39Z

...e-common/src/main/java/com/azure/storage/common/implementation/StructuredMessageDecoder.java

+    private long segmentCrc64 = 0;
+    private final Map<Integer, Long> segmentCrcs = new HashMap<>();
+
+    public StructuredMessageDecoder(ByteBuffer inputBuffer) {


In terms of the constructor, I'm thinking instead of accepting an input byte buffer, we'd accept the expected content length of the individual download and the decode() method would accept a byte buffer containing a part of the entire encoded structured message. The main reason that I'm suggesting this is that I suspect in order to integrate this with the client, we will be provided a flux of byte buffers so the content of the entire encoded structured message will be broken across multiple buffers. I imagine it will be similar in integration style to the client side encryption Decryptor class (note that it accepts a Flux<ByteBuffer> but we'll just want to accept ByteBuffer for now so that Reactor integration is decoupled from logic to do the actual decoding)

kyleknap · 2025-02-12T23:39:00Z

...e-common/src/main/java/com/azure/storage/common/implementation/StructuredMessageDecoder.java

+    }
+
+    private void readMessageHeader() {
+        if (buffer.remaining() < V1_HEADER_LENGTH) {


Then if we update the constructor to take an expected message size, we can raise this validation in the constructor similar to what Python does: https://github.com/Azure/azure-sdk-for-python/blob/b896d5c9ee202bd7c3cba51eb602562d41f4d804/sdk/storage/azure-storage-blob/azure/storage/blob/_shared/streams.py#L466

kyleknap · 2025-02-12T23:40:47Z

...e-common/src/main/java/com/azure/storage/common/implementation/StructuredMessageDecoder.java

+            throw new IllegalArgumentException("Unsupported structured message version: " + messageVersion);
+        }
+
+        messageLength = (int) buffer.getLong();


We should also do validation that the message length pulled from the message matches what was passed in as part of the constructor. We should be able to match Python here as well: https://github.com/Azure/azure-sdk-for-python/blob/b896d5c9ee202bd7c3cba51eb602562d41f4d804/sdk/storage/azure-storage-blob/azure/storage/blob/_shared/streams.py#L559-L561

kyleknap · 2025-02-12T23:44:09Z

...e-common/src/main/java/com/azure/storage/common/implementation/StructuredMessageDecoder.java

+        }
+    }
+
+    public byte[] decode(int size) {


In addition to updating the decode to accept a byte buffer instead of a size, let's have it return a byte buffer instead of byte array. I'm mainly suggesting this because the typical downstream download logic accepts byte buffers and we'd likely need this type I suspect to make the integration smoother (i.e. all logic is only working with byte buffers)

kyleknap · 2025-02-12T23:50:11Z

...e-common/src/main/java/com/azure/storage/common/implementation/StructuredMessageDecoder.java

+    }
+
+    public byte[] decode(int size) {
+        ByteArrayOutputStream decodedContent = new ByteArrayOutputStream();


One more public method we are going to need is a finalize method (I'm not too tied to that name if you have a better one) that will tell the decoder that we have finished decoding and have it validate that we've consumed the entire message. I'm suggesting this because I'm not sure if the case is handled where the structured message get cut off and we only process a portion of the structured message. If we call this finalize after feeding through all byte buffers, it would then throw an error if in fact we did not get complete the decoding process.

azure-sdk · 2025-02-14T00:47:05Z

API change check

APIView has identified API level changes in this PR and created following API reviews.

com.azure:azure-storage-common

ibrahimrabab added 2 commits February 10, 2025 16:19

Structured Message Decoder

2f7aa61

fixing parameters for testIncorrectMessageLength and removing unneces…

06c862c

…sary comments

github-actions bot added the Storage Storage Service (Queues, Blobs, Files) label Feb 11, 2025

ibrahimrabab added 2 commits February 10, 2025 16:59

Merge branch 'feature/storage/content-validation' of https://github.c…

1dab0ad

…om/Azure/azure-sdk-for-java into structuredMessageDecoder2

fixing parameters for invalidSegment test

a3fe4ee

ibrahimrabab changed the base branch from main to feature/storage/content-validation February 11, 2025 01:17

ibrandes reviewed Feb 11, 2025

View reviewed changes

kyleknap reviewed Feb 12, 2025

View reviewed changes

changing decodeFully to decode() as an overload method

a43e1eb

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Structured Message Decoder #44140

Structured Message Decoder #44140

ibrahimrabab commented Feb 11, 2025

ibrandes Feb 11, 2025

kyleknap Feb 12, 2025

ibrandes Feb 11, 2025

kyleknap Feb 12, 2025

ibrandes Feb 11, 2025

kyleknap Feb 12, 2025

kyleknap left a comment

kyleknap Feb 12, 2025

kyleknap Feb 12, 2025

kyleknap Feb 12, 2025

kyleknap Feb 12, 2025

kyleknap Feb 12, 2025

kyleknap Feb 12, 2025

kyleknap Feb 12, 2025

kyleknap Feb 12, 2025

azure-sdk commented Feb 14, 2025

Structured Message Decoder #44140

Are you sure you want to change the base?

Structured Message Decoder #44140

Conversation

ibrahimrabab commented Feb 11, 2025

Description

All SDK Contribution checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kyleknap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azure-sdk commented Feb 14, 2025