fix: allow range headers to be set in getObject request when offset is 0 (#706) #707

christopher-fredregill · 2018-10-02T05:56:16Z

fix: allow range headers to be set in getObject request when offset is 0 (#706)

fixes #706

christopher-fredregill · 2018-10-02T06:34:19Z

Some notes for anyone reviewing this:

The test that's failing in Travis also fails on master when run against a minio/minio:latest docker endpoint. I've verified it does not fail when run against the following earlier images:

#    image: minio/minio:RELEASE.2018-09-25T21-34-43Z # passes
#    image: minio/minio:RELEASE.2018-09-12T18-49-56Z # passes
#    image: minio/minio:RELEASE.2018-08-02T23-11-36Z # passes

2018-09-17T09-22-31Z appears to be what's running on play.minio.io, but there isn't a named docker image corresponding to that version in DockerHub...

It seems like there may be some sort of regression in one of the more recent versions of minio, but I haven't really tracked that down just yet. In any case, it doesn't look like the test failure is really caused by anything specifically added by my branch. Here is a docker-compose file snippet that can be used to spin up a test environment to show the different behaviors:

version: '3.7'
services:
  minio:
    image: minio/minio:latest # test fails
#    image: minio/minio:RELEASE.2018-09-25T21-34-43Z # passes
#    image: minio/minio:RELEASE.2018-09-12T18-49-56Z # passes
#    image: minio/minio:RELEASE.2018-08-02T23-11-36Z # passes
    command: server /data
    ports:
      - 9000:9000
    volumes:
      - ./data/:/data/

Please see https://github.com/minio/minio-java/blob/master/api/src/main/java/io/minio/MinioClient.java#L1627 in the original implementation. When the length is null, but the offset is valid, something like Range: bytes=X- (where X is the user-supplied offset) gets added to the request headers. This always results in a 416/The requested range is not satisfiable error response, in the test suite. You'll notice that, as a workaround, I've placed the failing test in a try/catch block, but I am wondering if there is any reason why we should not simply require that a valid length be given. Please advise as to how best to handle this.
None of the test cases I found when I started looking into this problem were actually capable of detecting the error, so I modified one of the functional test cases to make some stronger assertions about the response from a partial getObject request. I'm open to other suggestions around how to verify the functionality in question.

harshavardhana

It is not a regression we fixed a compatibility issue, the PR here is changing certain assumptions.

getObject_test6() is for an empty object and for empty objects according to S3 protocol one should return InvalidRange and it is a valid exception.

harshavardhana · 2018-10-02T08:02:21Z

api/src/main/java/io/minio/MinioClient.java

      throw new InvalidArgumentException("length should be greater than zero");
    }

    Map<String,String> headerMap = new HashMap<>();


this is an incomplete implementation follow this to be implement all variations

https://github.com/minio/minio-go/blob/master/api-get-options.go#L101

getObject() in minio-java does accept offset and length not range i.e. not start/end values. With this existing fix is correct.

There are three allowed behaviors

bytes=N-M

bytes=N-

bytes=-M

In minio-java the last one is not supported this is what I meant by complete implementation @balamurugana

The meaning of offset and length are different than range i.e. offset=-8 and length=4 means move to 8 byte position before current position and read 4 bytes. This is not possible to be translated into range. Either offset/length need to be replaced with start/end or leave the current behaviour as is (not possible to support byte=-M)

balamurugana · 2018-10-02T15:01:50Z

api/src/main/java/io/minio/MinioClient.java

      if (length != null) {
        headerMap.put("Range", "bytes=" + offset + "-" + (offset + length - 1));
      } else {
+        // results in a 416 with response message "The requested range is not satisfiable"


This comment is not needed

balamurugana · 2018-10-02T15:02:41Z

functional/FunctionalTest.java

      String objectName = getRandomName();
-      try (final InputStream is = new ContentInputStream(3 * MB)) {
-        client.putObject(bucketName, objectName, is, 3 * MB, nullContentType);
+      try (final InputStream is = new ContentInputStream(1024)) {


Add new tests than changing existing ones.

I've moved the new test case (with the assertions about the bytes that should be received from the partial getObject request) into getObject_test8 in the functional suite. I had to make minor updates to two other functional tests that were leaving behind objects in the shared bucket and causing listObject_test5 to fail in error; those other tests should now be cleaning up after themselves.

christopher-fredregill · 2018-10-03T05:56:34Z

@harshavardhana @balamurugana can you please take another look?

harshavardhana

Everything else LGTM @christopher-fredregill - thanks

harshavardhana · 2018-10-03T05:57:46Z

functional/FunctionalTest.java

+      try {
+        client.getObject(bucketName, objectName).close();
+      } catch (final ErrorResponseException e) {
+        // current implementation results in request headers="Range: bytes=0-"; response is a 416 with the given message


We can remove this comment, this is an expected response.

api/src/main/java/io/minio/MinioClient.java

balamurugana · 2018-10-03T11:15:56Z

functional/FunctionalTest.java

+        if (!e.toString().contains("message=The requested range is not satisfiable")) {
+          throw e;
+        }
+      }


Unable to understand this change. Why is it required for this test?

The original test catches and re-throws any exception it encounters.

I modified the behavior of the client, such that a known 416 exception will always be encountered here.

The test now has to know in advance that it will see this kind of exception and accept it, or else it will simply fail.

This is why I included comments explaining this in both the client code and in the fictional test code.

Some sort of change is necessary here... maybe you can propose an alternative solution, if this doesn’t suit you.

The problem is we send offset requests by default now - after this PR change even for empty objects which is not allowed in S3

This exception check is needed, or we make sure in this test Range is not set for empty objects.

functional/FunctionalTest.java

api/src/main/java/io/minio/MinioClient.java

…s 0 (minio#706) fixes minio#706

harshavardhana · 2018-10-04T04:33:30Z

We could add a test to generate the 416 range not satisfiable with offset=1 for a zero byte object @christopher-fredregill upto you though. This PR is okay to be merged.

nitisht requested review from balamurugana and harshavardhana October 2, 2018 05:56

harshavardhana requested changes Oct 2, 2018

View reviewed changes

balamurugana reviewed Oct 2, 2018

View reviewed changes

christopher-fredregill force-pushed the hacktoberfest_fix_issue_706 branch 3 times, most recently from 7799e85 to 04c45c1 Compare October 3, 2018 05:30

harshavardhana requested changes Oct 3, 2018

View reviewed changes

christopher-fredregill force-pushed the hacktoberfest_fix_issue_706 branch from 04c45c1 to e4bcf70 Compare October 3, 2018 07:48

harshavardhana previously approved these changes Oct 3, 2018

View reviewed changes

harshavardhana mentioned this pull request Oct 3, 2018

GetObject to N bytes when offset is 0 #709

Closed

sinhaashish mentioned this pull request Oct 3, 2018

Unable to get first X bytes of object #706

Closed

balamurugana requested changes Oct 3, 2018

View reviewed changes

balamurugana reviewed Oct 4, 2018

View reviewed changes

api/src/main/java/io/minio/MinioClient.java Show resolved Hide resolved

christopher-fredregill dismissed harshavardhana’s stale review via ce617c0 October 4, 2018 02:29

christopher-fredregill force-pushed the hacktoberfest_fix_issue_706 branch from e4bcf70 to ce617c0 Compare October 4, 2018 02:29

fix: allow range headers to be set in getObject request when offset i…

d0d9f67

…s 0 (minio#706) fixes minio#706

christopher-fredregill force-pushed the hacktoberfest_fix_issue_706 branch from ce617c0 to d0d9f67 Compare October 4, 2018 02:37

balamurugana approved these changes Oct 4, 2018

View reviewed changes

harshavardhana approved these changes Oct 4, 2018

View reviewed changes

harshavardhana merged commit a5b145f into minio:master Oct 4, 2018

fix: allow range headers to be set in getObject request when offset is 0 (#706) #707

fix: allow range headers to be set in getObject request when offset is 0 (#706) #707

Uh oh!

Conversation

christopher-fredregill commented Oct 2, 2018

Uh oh!

christopher-fredregill commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

harshavardhana left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopher-fredregill commented Oct 3, 2018

Uh oh!

harshavardhana left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

harshavardhana commented Oct 4, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

christopher-fredregill commented Oct 2, 2018 •

edited

Loading