-
Notifications
You must be signed in to change notification settings - Fork 0
[1.x] Allow to override the default mappings provided by DsColumnMapping
#186
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
Set a proper name for the test.
Timestamp.getDefaultValue() stored as Datastore-specific null by defaultDsColumnMapping
|
@yevhenii-nadtochii PTAL. I feel it won't hurt to have another pair of eyes on this. |
DsColumnMappingDsColumnMapping
yevhenii-nadtochii
left a comment
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
But I would add somewhere in docs to DsColumnMapping.customMapping() that this method is a workaround for AbstractColumnMapping.setupCustomMapping(...). This word pops up when I see these two similar-named methods and their docs, but it is not said explicitly.
| /** | ||
| * Returns the default mapping from {@link Timestamp} to {@link TimestampValue}. | ||
| */ | ||
| @SuppressWarnings({"ProtoTimestampGetSecondsGetNano" /* In order to create exact value. */, |
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 would move "ProtoTimestampGetSecondsGetNano" to a new line.
And the same with the suppression below.
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.
| "allow clearing the column values if they get Proto's default values, " + | ||
| "by having a custom column mapping " + | ||
| "returning Datastore's `null` for respective values") |
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 would simplify this name to clear column values using Datastore's 'null', and move the rest to method's docs.
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.
Simplified a bit. But still, we need to be a bit more precise here, as the idea of the test is to check the configuration with the specific column mapping.
For this library, this is not a workaround. Also, if such is a wish, it is possible to create a descendant from |
This PR reproduces and addresses core-java#1536.
Previously, it was not possible to clear columns of some Proto types, since it was not possible to distinguish between meaningful "default values" set to columns on purpose, and those "default values" which in fact signalized of "no value set".
Changing the API of
DsColumnMapping❗ This is a breaking change.
Previously, SPI users could only extend
DsColumnMapping's behavior by overriding this method:It was good enough if one wanted to append their mapping rules. But such an approach did not allow to re-define the existing (framework-default) mapping — since a builder of
ImmutableMapwas passed, preventing from having duplicate keys.Now, the API for SPI users implies overriding another method:
This one allows to return an immutable map of mappings, combining them into the final result, giving priority to those values which were provided by SPI users.
Please see the documentation for
DsColumnMappingfor more details.How to use it
Referring to the original issue, end-users may configure how
Timestamp.getDefaultValue()is stored for some column. In the example below, a Datastore-specificnullis written for such values to the corresponding property of respective DatastoreEntity:Updates to
TestDatastoreStorageFactoryAdditionally, this changeset updates the API of
TestDatastoreStorageFactoryutility to allow access to two features:DatastoreWrappervia public API, which is useful to verify the actual content of Datastore entities written.See
TestDatastoreStorageFactoryfor detail, and have a look at the usage example inDsProjectionColumnsTest.The library version is set to
1.9.1.