-
Notifications
You must be signed in to change notification settings - Fork 109
feat: Add datastore mode data transforms #1369
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
Conversation
…into 404540305-datastore-mode-data-transforms
|
LGTM ! |
| export type PropertyTransform = { | ||
| property: string; | ||
| setToServerValue: boolean; | ||
| increment: any; |
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.
do these have to be any?
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.
maximum/minimum/increment/appendMissingElements/removeAllFromArray proto values can technically accept any type so there really are tons of combinations. In this feature we use encodeValue function to take the user supplied value and encode it into the generic proto IValue format. The proto supports the generic IValue type so to match we allow users to supply any type to support all the different input possibilities.
src/entity.ts
Outdated
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| export type Entity = any; | ||
| // TODO: Call out this interface change |
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.
What is this TODO here for?
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 think this TODO was just a note for me to mention this in the PR. The idea here is if the user provides a transforms property for the entities they pass in then the compiler will force this property to be a PropertyTransform[] which is technically a breaking change.
The alternative is that we could not change this interface, but instead cast the transforms property as soon as it is passed into the save function.
| import IValue = google.datastore.v1.IValue; | ||
| import ServerValue = google.datastore.v1.PropertyTransform.ServerValue; | ||
|
|
||
| export function buildPropertyTransforms(transforms: PropertyTransform[]) { |
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.
nit: this would benefit from some comments
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 some comments.
| assert.strictEqual(entity, undefined); | ||
| }); | ||
| }); | ||
| describe('Datastore mode data transforms', () => { |
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.
Does this single test really cover the whole feature? (I dont see anything with setToServerValue: false for example. And aren't non-int values supported?)
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.
We can add tests to cover setToServerValue: false and minimum/maximum etc. for strings. There are tons of permutations/combinations and I don't think we can cover them all, but we can definitely cover these.
maximum/minimum/increment/appendMissingElements/removeAllFromArray proto values can technically accept any type so there really are tons of combinations. In this feature we use encodeValue function to take the user supplied value and encode it into the generic proto IValue format. The proto supports the generic IValue type so to match we allow users to supply any type to support all the different input possibilities.
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.
Ok. These edge cases have been added now
…into 404540305-datastore-mode-data-transforms
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

Summary:
This PR adds support for datastore mode data transforms. A new option is provided for the first argument in the
savemethod and code is added that will build the entity proto so that the backend receives requests that will do Datastore Mode data transforms.API change
Here is an example of a call that the user can now make with the
savemethod. They can now providetransformsthat will do data transforms for acommitoperation:Notes about the API surface:
maximum,minimum,increment,setToServerValueare encoded togoogle.datastore.v1.IValueso they can representanyvalue sincegoogle.datastore.v1.IValueis an encoded version of any value type. Likewise,appendMissingElementsandremoveAllFromArraycan accept any array ofanyvalues since they are encoded into thegoogle.datastore.v1.IArrayValuetype.Alternatives:
We could support an API where transforms are indexed by the property:
Pros:
Cons:
Changes:
src/entity.ts: These are just interface changes. Technically they would affect users that currently supply atransformproperty, but I doubt any users currently use atransformproperty. We could remove this file and the PR would still be good.src/index.ts: The save method is modified to attach propertyTransforms to the entity protosrc/utils/entity/buildPropertyTransforms.ts: This file is added to build the propertyTransforms from the user'stransformsarray.system-test/datastore: A test is added that performs asaveoperation with transforms and ensures the encoded entity proto is correct, the transforms returned are correct and the backend data after the update is correct.Next Steps:
We should add support for users that wish to do transforms with transactions.
Requests for the reviewer:
src/entity.tsfile.