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

fix(datastore): fix NoIndex for array property #7674

Merged
merged 7 commits into from
Aug 8, 2023

Conversation

aaron0x
Copy link
Contributor

@aaron0x aaron0x commented Apr 1, 2023

When converting proto to Entity with array property, ExcludeFromIndexes appears in the values level, not the top level.

@aaron0x aaron0x requested review from bhshkh and a team as code owners April 1, 2023 08:46
@google-cla
Copy link

google-cla bot commented Apr 1, 2023

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@product-auto-label product-auto-label bot added size: m Pull request size is medium. api: datastore Issues related to the Datastore API. labels Apr 1, 2023
@aaron0x aaron0x force-pushed the fix/datastore-array-property branch from 631d389 to 79c74f6 Compare April 1, 2023 10:52
When converting proto to Entity with array property, excludeFromIndexes appears in the values level, not the top level.
@aaron0x aaron0x force-pushed the fix/datastore-array-property branch from 6df785f to bb7a332 Compare April 7, 2023 14:00
@product-auto-label product-auto-label bot added the stale: old Pull request is old and needs attention. label May 2, 2023
@product-auto-label product-auto-label bot added stale: extraold Pull request is critically old and needs prioritization. and removed stale: old Pull request is old and needs attention. labels Jun 1, 2023
@bhshkh bhshkh added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Jul 30, 2023
@kokoro-team kokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 30, 2023
@bhshkh bhshkh enabled auto-merge (squash) July 30, 2023 15:32
@bhshkh bhshkh linked an issue Jul 30, 2023 that may be closed by this pull request
@bhshkh bhshkh added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Jul 31, 2023
@kokoro-team kokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Jul 31, 2023
Comment on lines 656 to 658
if !testutil.Equal(want, dst) {
t.Errorf("NoIndex should be correct: compare:\ngot: %#v\nwant: %#v", dst, want)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if !testutil.Equal(want, dst) {
t.Errorf("NoIndex should be correct: compare:\ngot: %#v\nwant: %#v", dst, want)
}
cmpProperties := func(p1, p2 Property) bool {
return p1.Name < p2.Name
}
if !testutil.Equal(want.Properties, dst.Properties, cmpopts.SortSlices(cmpProperties)) {
t.Errorf("NoIndex should be correct: Property:\ngot: %#v\nwant: %#v", dst, want)
}
if !testutil.Equal(want.Key, dst.Key) {
t.Errorf("NoIndex should be correct: Key:\ngot: %#v\nwant: %#v", dst, want)
}

@@ -602,6 +602,62 @@ func TestTimezone(t *testing.T) {
}
}

func TestLoadArrayIndex(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This fails intermittently with error:

=== RUN   TestLoadArrayIndex
    load_test.go:657: NoIndex should be correct: compare:
        got:  &datastore.Entity{Key:(*datastore.Key)(0xc0005457c0), Properties:[]datastore.Property{datastore.Property{Name:"non-indexed", Value:[]interface {}{"3", "4"}, NoIndex:true}, datastore.Property{Name:"indexed", Value:[]interface {}{"1", "2"}, NoIndex:false}}}
        want: &datastore.Entity{Key:(*datastore.Key)(0xc0004b1b80), Properties:[]datastore.Property{datastore.Property{Name:"indexed", Value:[]interface {}{"1", "2"}, NoIndex:false}, datastore.Property{Name:"non-indexed", Value:[]interface {}{"3", "4"}, NoIndex:true}}}
--- FAIL: TestLoadArrayIndex (0.00s)

Please see suggested changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks for the suggestion.

auto-merge was automatically disabled August 2, 2023 01:16

Head branch was pushed to by a user without write access

@bhshkh bhshkh added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Aug 8, 2023
@kokoro-team kokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Aug 8, 2023
@bhshkh bhshkh enabled auto-merge (squash) August 8, 2023 17:30
@bhshkh bhshkh added kokoro:force-run Add this label to force Kokoro to re-run the tests. kokoro:run Add this label to force Kokoro to re-run the tests. labels Aug 8, 2023
@kokoro-team kokoro-team removed kokoro:run Add this label to force Kokoro to re-run the tests. kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Aug 8, 2023
@bhshkh bhshkh merged commit 01951e6 into googleapis:main Aug 8, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: datastore Issues related to the Datastore API. size: m Pull request size is medium. stale: extraold Pull request is critically old and needs prioritization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

datastore: array property always has NoIndex false
4 participants