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(datastore): Adding reserve IDs support #9027

Merged
merged 2 commits into from
Jan 23, 2024

Conversation

bhshkh
Copy link
Contributor

@bhshkh bhshkh commented Nov 16, 2023

When an entity is created in Datastore, a key is auto-allocated to the entity if none is provided at the entity creation time.
This PR adds ReserveIDs method using which a user can prevent the supplied keys' IDs from being auto-allocated by Cloud Datastore. This will remove the feature gap in Go client library. Java and NodeJS both support reserveIDs

The implementation is similar to AllocateIDS

// AllocateIDs accepts a slice of incomplete keys and returns a
// slice of complete keys that are guaranteed to be valid in the datastore.
func (c *Client) AllocateIDs(ctx context.Context, keys []*Key) ([]*Key, error) {
if keys == nil {
return nil, nil
}
req := &pb.AllocateIdsRequest{
ProjectId: c.dataset,
Keys: multiKeyToProto(keys),
}
resp, err := c.client.AllocateIds(ctx, req)
if err != nil {
return nil, err
}
return multiProtoToKey(resp.Keys)
}

@bhshkh bhshkh requested review from a team as code owners November 16, 2023 23:34
@product-auto-label product-auto-label bot added size: s Pull request size is small. api: datastore Issues related to the Datastore API. labels Nov 16, 2023
@bhshkh bhshkh linked an issue Nov 16, 2023 that may be closed by this pull request
@@ -1220,6 +1220,21 @@ func TestIntegration_Projection(t *testing.T) {
})
}

func TestIntegration_ReserveIDs(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.

Are there any assertions you want to make here about either the request or the response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

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

Are there any assert statements that could be added to make sure the code did "prevent the supplied keys' IDs from being auto-allocated by Cloud Datastore"? Perhaps by making a read call against the keys and ensuring the result of the read is nil?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The result of read is nil with and without reserving IDs.

Choose a reason for hiding this comment

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

I see. Maybe I misunderstood the meaning of "prevent the supplied keys' IDs from being auto-allocated by Cloud Datastore". Is there any test with an assert statement that ensures calling reserveIds will "prevent the supplied keys' IDs from being auto-allocated by Cloud Datastore"? Is there anything we are doing to ensure that behaviour is captured?

Copy link
Contributor Author

@bhshkh bhshkh Jan 23, 2024

Choose a reason for hiding this comment

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

I could add a test like this but 1000 operations does not exhaust the possibility that the id could be allocated later. So, not a definite test:

	reservedKeyIDs := []int64{1234567, 8900000, 7654897}
	keys := make([]*Key, len(reservedKeyIDs))
	for i := range reservedKeyIDs {
		keys[i] = IDKey("ReserveIDs", reservedKeyIDs[i], nil)
	}
	err := client.ReserveIDs(ctx, keys)
	if err != nil {
		t.Fatalf("ReserveIDs failed: %v", err)
	}

	type value struct{ N int }
	v := &value{N: 5}
	for i := 0; i < 1000; i++ {
		autoAllocatedKey, err := client.Put(ctx, IncompleteKey("ReserveIDs", nil), v)
		if err != nil {
			t.Fatal(err)
		}

		if slices.Contains(reservedKeyIDs, autoAllocatedKey.ID) {
			t.Fatalf("Reserved ID auto-allocated by Datastore: %v\n", autoAllocatedKey.ID)
		}
	}

Ideally, Datastore service should return an error if reservation failed. So, just checking for error should suffice, right?

Choose a reason for hiding this comment

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

Ok. I see your point and understand that this could be very difficult to test since the allocated key for client.Put is basically random. This LGTM.

@product-auto-label product-auto-label bot added the stale: old Pull request is old and needs attention. label Dec 17, 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 Jan 16, 2024
@bhshkh bhshkh enabled auto-merge (squash) January 23, 2024 21:03
@bhshkh bhshkh merged commit 2d66de0 into googleapis:main Jan 23, 2024
9 checks passed
@bhshkh bhshkh deleted the feature/ds-reserve-ids branch January 23, 2024 23:08
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: s Pull request size is small. stale: extraold Pull request is critically old and needs prioritization.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

datastore: enable restoring of entities with id keys
3 participants