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

feat(storage): allow specifying includeTrailingDelimiter #5617

Merged
merged 2 commits into from
Mar 1, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions storage/bucket.go
Original file line number Diff line number Diff line change
Expand Up @@ -1543,6 +1543,7 @@ func (it *ObjectIterator) fetch(pageSize int, pageToken string) (string, error)
req.StartOffset(it.query.StartOffset)
req.EndOffset(it.query.EndOffset)
req.Versions(it.query.Versions)
req.IncludeTrailingDelimiter(it.query.IncludeTrailingDelimiter)
if len(it.query.fieldSelection) > 0 {
req.Fields("nextPageToken", googleapi.Field(it.query.fieldSelection))
}
Expand Down
40 changes: 39 additions & 1 deletion storage/integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1341,6 +1341,7 @@ func TestIntegration_Objects(t *testing.T) {
"obj1",
"obj2",
"obj/with/slashes",
"obj/",
}
contents := make(map[string][]byte)

Expand Down Expand Up @@ -1395,6 +1396,43 @@ func TestIntegration_Objects(t *testing.T) {
t.Errorf("prefixes = %v, want %v", gotPrefixes, sortedPrefixes)
}
})
t.Run("testObjectsIterateSelectedAttrsDelimiterIncludeTrailingDelimiter", func(t *testing.T) {
query := &Query{Prefix: "", Delimiter: "/", IncludeTrailingDelimiter: true}
if err := query.SetAttrSelection([]string{"Name"}); err != nil {
t.Fatalf("selecting query attrs: %v", err)
}

var gotNames []string
var gotPrefixes []string
it := bkt.Objects(context.Background(), query)
for {
attrs, err := it.Next()
if err == iterator.Done {
break
}
if err != nil {
t.Fatalf("iterator.Next: %v", err)
}
if attrs.Name != "" {
gotNames = append(gotNames, attrs.Name)
} else if attrs.Prefix != "" {
gotPrefixes = append(gotPrefixes, attrs.Prefix)
}

if attrs.Bucket != "" {
t.Errorf("Bucket field not selected, want empty, got = %v", attrs.Bucket)
}
}

sortedNames := []string{"obj/", "obj1", "obj2"}
if !cmp.Equal(sortedNames, gotNames) {
t.Errorf("names = %v, want %v", gotNames, sortedNames)
}
sortedPrefixes := []string{"obj/"}
if !cmp.Equal(sortedPrefixes, gotPrefixes) {
t.Errorf("prefixes = %v, want %v", gotPrefixes, sortedPrefixes)
}
})

// Test Reader.
for _, obj := range objects {
Expand Down Expand Up @@ -1872,7 +1910,7 @@ func testObjectsIterateAllSelectedAttrs(t *testing.T, bkt *BucketHandle, objects
// verifying the returned results).
query := &Query{
Prefix: "",
StartOffset: "obj/with/slashes",
StartOffset: "obj/",
EndOffset: "obj2",
}
var selectedAttrs []string
Expand Down
8 changes: 8 additions & 0 deletions storage/storage.go
Original file line number Diff line number Diff line change
Expand Up @@ -1588,6 +1588,14 @@ type Query struct {
// which returns all properties. Passing ProjectionNoACL will omit Owner and ACL,
// which may improve performance when listing many objects.
Projection Projection

// 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.
IncludeTrailingDelimiter bool
}
Copy link
Contributor

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:

func testObjectsIterateWithProjection(t *testing.T, bkt *BucketHandle) {

Copy link
Contributor Author

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?

Copy link
Contributor Author

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added test.


// attrToFieldMap maps the field names of ObjectAttrs to the underlying field
Expand Down