-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
feat(storage): allow specifying includeTrailingDelimiter #5617
Conversation
21a4344
to
733f824
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! A couple comments.
storage/storage.go
Outdated
@@ -1588,6 +1588,11 @@ type Query struct { | |||
// which returns all properties. Passing ProjectionNoACL will omit Owner and ACL, | |||
// which may improve performance when listing many objects. | |||
Projection Projection | |||
|
|||
// If true, objects that end in exactly one instance of delimiter have |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see this is copied from the API docs, but it should be rephrased based on how this is exposed via ObjectIterator in this library and based on godoc standards. I would say something like:
IncludeTrailingDelimiter controls how objects which end in a single instance of Delimiter (for example, if Query.Delimiter = "/" and the object name is "foo/bar/") are included in the results. By default, these objects only show up as prefixes. If IncludeTrailingDelimiter is set to true, they will also be included as objects and their metadata will be populated in the returned ObjectAttrs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
// If true, objects that end in exactly one instance of delimiter have | ||
// their metadata included in items[] in addition to the relevant part of | ||
// the object name appearing in prefixes[]. | ||
IncludeTrailingDelimiter bool | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also write an integration test to verify that this behaves as expected with real objects? Example for Query.Projection is here:
google-cloud-go/storage/integration_test.go
Line 1902 in 9f79ac4
func testObjectsIterateWithProjection(t *testing.T, bkt *BucketHandle) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I read through the integration test scrolls but the evil wizard frightened me. I tried again with some magical potions 🍷 but this quest seems to require some serious XP. Is there a cheat code to avoid grinding?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this use case we can sidestep most of the suggested configuration:
(cd storage/ && GCLOUD_TESTS_GOLANG_KEY=/path/to/creds.json GCLOUD_TESTS_GOLANG_PROJECT_ID=proj go test -v -run TestIntegration_Objects)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added test.
733f824
to
a3978ec
Compare
a3978ec
to
b0a25e2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks very much for the contribution!
References fsouza/fake-gcs-server#676.