Refactor gcp-fixture multipart parser #125828

mhl-b · 2025-03-28T07:34:33Z

fixture.gcs.GoogleCloudStorageHttpHandler#parseMultipartRequestBody is giving hard times to understand how parsing works. This PR tries to follow https://www.rfc-editor.org/rfc/rfc2046.html#page-19 as a guidance for multipart content parsing.

elasticsearchmachine · 2025-03-28T07:34:56Z

Pinging @elastic/es-distributed-coordination (Team:Distributed Coordination)

test/fixtures/gcs-fixture/src/main/java/fixture/gcs/MultipartUpload.java

nicktindall

Much nicer 👍

Might be worth adding some tests given that it's non-trivial?

DaveCTurner

Please could you add some unit tests for this? I.e. something test/fixtures/gcs-fixture/src/test/....

test/fixtures/gcs-fixture/src/main/java/fixture/gcs/MultipartUpload.java

DaveCTurner · 2025-03-28T07:54:46Z

test/fixtures/gcs-fixture/src/main/java/fixture/gcs/GoogleCloudStorageHttpHandler.java

-                    );
-                    writeBlobVersionAsJson(exchange, newBlobVersion);
-                } else {
-                    throw new AssertionError(


I would like to keep the behaviour where an invalid body causes a test failure rather than just an IllegalStateException

Throw AssertionError errors from the MultipartUpload.parseBody now.

I was sort of hoping to see tests that ensured that this kind of malformed body would throw an IAE from the parser. Throwing AssertionError from the parser itself basically means you can only test the happy path (or else catch AssertionError in tests which is best avoided if possible).

Co-authored-by: David Turner <[email protected]>

mhl-b · 2025-03-28T08:00:11Z

It's covered by all tests that use fixture. Metrics, retries, rest tests. There are some failures that I will address.

DaveCTurner · 2025-03-28T08:02:25Z

It's covered by all tests that uses fixture. Metrics, retries, rest tests. There are some failures that I will address.

I don't think that's true, these tests only cover the specific outputs that the GCS client sends. I suspect very few of them send multiple parts, and none of them send anything invalid.

…rt-refactor

mhl-b · 2025-03-30T00:15:22Z

There are tests that validate multipart upload parsing, directly and indirectly:

fixture.gcs.GoogleCloudStorageHttpHandlerTests#testIfGenerationMatch_MultipartUpload
org.elasticsearch.repositories.gcs.GoogleCloudStorageBlobContainerRetriesTests#testWriteBlobWithRetries
org.elasticsearch.repositories.blobstore.testkit.analyze.RepositoryAnalysisSuccessIT#testRepositoryAnalysis

Also added new tests for the MultipartUpload#readUntilDelimiter and MultipartUpload#skipUntilDelimiter.

DaveCTurner · 2025-03-31T07:08:53Z

...est/java/org/elasticsearch/repositories/gcs/GoogleCloudStorageBlobContainerRetriesTests.java

-                assertThat(content.get().v1(), equalTo(blobContainer.path().buildAsString() + "write_blob_max_retries"));
-                if (Objects.deepEquals(bytes, BytesReference.toBytes(content.get().v2()))) {
+                MultipartUpload multipartUpload = MultipartUpload.parseBody(exchange, exchange.getRequestBody());
+                assertTrue(multipartUpload.content().length() > 0);


It's legitimate to upload an empty blob comprising a single empty part isn't it? Certainly is in S3. We should be covering this case in these tests.

This test explicitly set content length to be at least 1 byte long. Empty content is not expected.

protected static byte[] randomBlobContent() { return randomBlobContent(1); }

It's legitimate to send 0 byte blob. It will produce two body parts, json metadata and empty part with headers.

I think we could do away with that assertion (it might trip in the event someone changes randomBlobContent() down the track). It's not important for the test. I think the previous assertion (assertThat(content, isPresent());) was only there because the old parseMultipartRequestBody returned Optional.empty() when the parse failed.

I don't have strong feelings about it but given we check that the content equals the uploaded bytes further down it seems redundant?

Changed test with min-size 0 and removed content length assertion b4869f5. Manually tested with 0 sized blob, it works.

DaveCTurner · 2025-03-31T07:14:39Z

test/fixtures/gcs-fixture/src/main/java/fixture/gcs/GoogleCloudStorageHttpHandler.java

-                    );
-                    writeBlobVersionAsJson(exchange, newBlobVersion);
-                } else {
-                    throw new AssertionError(


I was sort of hoping to see tests that ensured that this kind of malformed body would throw an IAE from the parser. Throwing AssertionError from the parser itself basically means you can only test the happy path (or else catch AssertionError in tests which is best avoided if possible).

DaveCTurner · 2025-03-31T07:19:24Z

test/fixtures/gcs-fixture/src/test/java/fixture/gcs/GoogleCloudStorageHttpHandlerTests.java

@@ -758,6 +762,8 @@ private static Headers rangeHeader(long start, long end) {
        return Headers.of("Range", Strings.format("bytes=%d-%d", start, end));
    }

+    private static final String MULTIPART_BOUNDARY = "__END_OF_PART__a607a67c-6df7-4b87-b8a1-81f639a75a97__";


Could we randomize this? I know it was fixed before, within the scope of createGzipCompressedMultipartUploadBody, but even that was a bit odd.

throwing AssertionError from the parser itself basically means you can only test the happy path (or else catch AssertionError in tests which is best avoided if possible).

True, at the same time the input we have depends on GCP SDK, not something we can easily change to produce unhappy path. Even if we can hack GCP client, then we would test hacked client against our own fixture, further away from the real world.

We're generating this boundary, we can set it to whatever we want. Let's use randomness to reflect that.

Oh sorry you're replying to a different thread 🤦

Yeah I don't want to hack the GCP client to produce invalid things, I just want some unit tests to check we're not being unduly lenient with our parsing.

Ah, sorry, for the boundary, yes. For some reason I cannot comment directly on the handling assertion comment. Dropped it here. Will do the boundary part.

I think fixed boundary string is fine in this test. With random boundary and random content we need to sanitize content, it should not contain boundary within. New parser is agnostic to boundary string, a new test added for a generic multipart request. So I propose to leave this test as it is.

I disagree, sorry, using the literal UUID a607a67c-6df7-4b87-b8a1-81f639a75a97 in this test implies to the reader that this UUID is in some way special. But it isn't, we could use any value here. We can keep the __END_OF_PART__ prefix to distinguish it from any string that could appear elsewhere in the body, but I'd really much prefer to have a random value here to help the reader understand that the boundary UUID is not required to be a fixed value.

No problem at all. Added random boundary string. 905620d

I removed static MULTIPART_BOUNDARY in following commit.

…rt-refactor

…sticsearch into gcp-fixture-multipart-refactor

nicktindall

LGTM

mhl-b · 2025-04-11T23:40:46Z

@DaveCTurner are you ok with current version? There is still block on PR.

…rt-refactor

…sticsearch into gcp-fixture-multipart-refactor

DaveCTurner

LGTM thanks Mikhail

refactor gcp-fixture multipart parser

2fb738d

mhl-b added >test Issues or PRs that are addressing/adding tests :Distributed Coordination/Snapshot/Restore Anything directly related to the `_snapshot/*` APIs Team:Distributed Coordination Meta label for Distributed Coordination team v9.1.0 labels Mar 28, 2025

mhl-b requested review from nicktindall and DaveCTurner March 28, 2025 07:34

nicktindall reviewed Mar 28, 2025

View reviewed changes

test/fixtures/gcs-fixture/src/main/java/fixture/gcs/MultipartUpload.java Outdated Show resolved Hide resolved

nicktindall approved these changes Mar 28, 2025

View reviewed changes

DaveCTurner requested changes Mar 28, 2025

View reviewed changes

DaveCTurner reviewed Mar 28, 2025

View reviewed changes

mhl-b and others added 2 commits March 28, 2025 00:57

remove ring-buffer

28d4481

naming

4ef1236

Co-authored-by: David Turner <[email protected]>

mhl-b added 2 commits March 28, 2025 13:31

Merge remote-tracking branch 'upstream/main' into gcp-fixture-multipa…

5105d4c

…rt-refactor

add tests

0dfddd6

Merge branch 'main' into gcp-fixture-multipart-refactor

1008194

DaveCTurner reviewed Mar 31, 2025

View reviewed changes

mhl-b and others added 5 commits April 3, 2025 16:55

Merge remote-tracking branch 'upstream/main' into gcp-fixture-multipa…

771e069

…rt-refactor

merge main

4eb8b23

generic multipart parser

3381864

Merge branch 'gcp-fixture-multipart-refactor' of github.com:mhl-b/ela…

f8eb315

…sticsearch into gcp-fixture-multipart-refactor

[CI] Auto commit changes from spotless

f3210e4

mhl-b requested review from DaveCTurner and nicktindall April 10, 2025 21:53

mhl-b added 2 commits April 10, 2025 15:04

exceptions

9714b32

Merge branch 'gcp-fixture-multipart-refactor' of github.com:mhl-b/ela…

019a04f

…sticsearch into gcp-fixture-multipart-refactor

nicktindall approved these changes Apr 11, 2025

View reviewed changes

mhl-b added 3 commits April 10, 2025 19:24

test zero-sized blob

b4869f5

sanitize content

47912f5

Merge branch 'main' into gcp-fixture-multipart-refactor

8de1110

mhl-b added 5 commits April 14, 2025 14:06

Merge remote-tracking branch 'upstream/main' into gcp-fixture-multipa…

1505b48

…rt-refactor

Merge branch 'gcp-fixture-multipart-refactor' of github.com:mhl-b/ela…

b7ef093

…sticsearch into gcp-fixture-multipart-refactor

random multipart boundary

905620d

cleanup

efda0e7

Merge branch 'main' into gcp-fixture-multipart-refactor

797d27d

DaveCTurner approved these changes Apr 15, 2025

View reviewed changes

mhl-b merged commit 5a7a425 into elastic:main Apr 15, 2025
17 checks passed

afoucret pushed a commit to afoucret/elasticsearch that referenced this pull request Apr 16, 2025

Refactor GCS fixture multipart parser (elastic#125828)

cfbb61a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor gcp-fixture multipart parser #125828

Refactor gcp-fixture multipart parser #125828

mhl-b commented Mar 28, 2025

elasticsearchmachine commented Mar 28, 2025

nicktindall left a comment •

edited

Loading

DaveCTurner left a comment

DaveCTurner Mar 28, 2025

mhl-b Mar 30, 2025

DaveCTurner Mar 31, 2025

mhl-b commented Mar 28, 2025 •

edited

Loading

DaveCTurner commented Mar 28, 2025

mhl-b commented Mar 30, 2025

DaveCTurner Mar 31, 2025

mhl-b Mar 31, 2025 •

edited

Loading

mhl-b Mar 31, 2025

nicktindall Apr 11, 2025 •

edited

Loading

mhl-b Apr 11, 2025 •

edited

Loading

DaveCTurner Mar 31, 2025

DaveCTurner Mar 31, 2025

mhl-b Mar 31, 2025

DaveCTurner Mar 31, 2025

DaveCTurner Mar 31, 2025

mhl-b Mar 31, 2025

mhl-b Apr 10, 2025

DaveCTurner Apr 14, 2025

mhl-b Apr 14, 2025

nicktindall left a comment

mhl-b commented Apr 11, 2025

DaveCTurner left a comment

Refactor gcp-fixture multipart parser #125828

Refactor gcp-fixture multipart parser #125828

Conversation

mhl-b commented Mar 28, 2025

elasticsearchmachine commented Mar 28, 2025

nicktindall left a comment • edited Loading

Choose a reason for hiding this comment

DaveCTurner 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

mhl-b commented Mar 28, 2025 • edited Loading

DaveCTurner commented Mar 28, 2025

mhl-b commented Mar 30, 2025

Choose a reason for hiding this comment

mhl-b Mar 31, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nicktindall Apr 11, 2025 • edited Loading

Choose a reason for hiding this comment

mhl-b Apr 11, 2025 • edited Loading

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

Choose a reason for hiding this comment

nicktindall left a comment

Choose a reason for hiding this comment

mhl-b commented Apr 11, 2025

DaveCTurner left a comment

Choose a reason for hiding this comment

nicktindall left a comment •

edited

Loading

mhl-b commented Mar 28, 2025 •

edited

Loading

mhl-b Mar 31, 2025 •

edited

Loading

nicktindall Apr 11, 2025 •

edited

Loading

mhl-b Apr 11, 2025 •

edited

Loading