Skip to content
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

api: PutObjectSignV2 should support uploading 0byte objects. #762

Merged
merged 1 commit into from
Jul 16, 2017

Conversation

harshavardhana
Copy link
Member

@harshavardhana harshavardhana commented Jul 14, 2017

Fixes #761

@deekoder deekoder requested review from vadmeste and krisis July 14, 2017 21:40
Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a small test here ?

@harshavardhana harshavardhana force-pushed the v2-sign branch 3 times, most recently from 2a174da to d7a13a5 Compare July 14, 2017 23:03
@harshavardhana
Copy link
Member Author

Tests have been added @vadmeste @krisis - please review.

@@ -141,7 +141,7 @@ func (c Client) putObjectMultipartNoStream(bucketName, objectName string, reader
}

// Verify if we uploaded all the data.
if size > 0 {
if size >= 0 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

putObjectNoChecksum is called when size is zero. So, this check is not needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason this is done is multipart should also work for size == 0 independent of its upper layer usage.

// object size is '0' set default parts.
if objectSize == 0 {
return 1, 0, 0, nil
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

putObjectNoChecksum is called when size is zero. So, this check is not needed.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code should work accordingly regardless of how its stitched together

@@ -3596,6 +3596,64 @@ func testUserMetadataCopyingV2() {
testUserMetadataCopyingWrapper(c)
}

// Test put object with 0 byte object.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should have a test for V4 too.

Copy link
Member Author

@harshavardhana harshavardhana Jul 15, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

V4 we already have PutObjectStreaming uploads a 0byte file.

@harshavardhana
Copy link
Member Author

@vadmeste can you take a look PR is updated with unit test as requested.

Copy link
Member

@vadmeste vadmeste left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and tested

@deekoder deekoder merged commit bd8e1d8 into minio:master Jul 16, 2017
@harshavardhana harshavardhana deleted the v2-sign branch July 16, 2017 19:03
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.

4 participants