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

[1.x] Allow to override the default mappings provided by DsColumnMapping #186

Merged
merged 17 commits into from
Sep 27, 2023

Conversation

armiol
Copy link
Contributor

@armiol armiol commented Sep 22, 2023

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:

protected void setupCustomMapping(
            ImmutableMap.Builder<Class<?>, ColumnTypeMapping<?, ? extends Value<?>>> builder);

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 ImmutableMap was passed, preventing from having duplicate keys.

Now, the API for SPI users implies overriding another method:

@SPI
protected ImmutableMap<Class<?>, ColumnTypeMapping<?, ? extends Value<?>>> customMapping()

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 DsColumnMapping for 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-specific null is written for such values to the corresponding property of respective Datastore Entity:

/**
 * A mapping similar to the default one,
 * but telling to store {@link Timestamp}s as {@code null}s.
 */
public final class CustomMapping extends DsColumnMapping {

    @Override
    protected ImmutableMap<Class<?>, ColumnTypeMapping<?, ? extends Value<?>>> customMapping() {
        return ImmutableMap.of(Timestamp.class, ofNullableTimestamp());
    }

    @SuppressWarnings("UnnecessaryLambda" /* For brevity */)
    private static ColumnTypeMapping<Timestamp, Value<?>> ofNullableTimestamp() {
        return timestamp -> {
            if (timestamp.equals(Timestamp.getDefaultInstance())) {
                return NullValue.of();
            }
            return TimestampValue.of(
                    ofTimeSecondsAndNanos(timestamp.getSeconds(), timestamp.getNanos())
            );
        };
    }
}

Updates to TestDatastoreStorageFactory

Additionally, this changeset updates the API of TestDatastoreStorageFactory utility to allow access to two features:

  1. Creation of a test-only storage factory with some custom mapping.
  2. Accessing "raw" DatastoreWrapper via public API, which is useful to verify the actual content of Datastore entities written.

See TestDatastoreStorageFactory for detail, and have a look at the usage example in DsProjectionColumnsTest.

The library version is set to 1.9.1.

@armiol armiol self-assigned this Sep 22, 2023
@armiol armiol changed the title Make Timestamp.getDefaultValue() stored as Datastore-specific null by default Allow to override the default mappings provided by DsColumnMapping Sep 26, 2023
@armiol armiol marked this pull request as ready for review September 26, 2023 17:09
@armiol
Copy link
Contributor Author

armiol commented Sep 26, 2023

@yevhenii-nadtochii PTAL. I feel it won't hurt to have another pair of eyes on this.

@armiol armiol changed the title Allow to override the default mappings provided by DsColumnMapping [1.x] Allow to override the default mappings provided by DsColumnMapping Sep 26, 2023
Copy link
Contributor

@yevhenii-nadtochii yevhenii-nadtochii left a 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. */,
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment on lines 61 to 63
"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")
Copy link
Contributor

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.

Copy link
Contributor Author

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.

@armiol
Copy link
Contributor Author

armiol commented Sep 27, 2023

@yevhenii-nadtochii

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.

For this library, this is not a workaround. DsColumnMapping.setupCustomMapping() is made final, so nobody is now able to override it. The only extension point is DsColumnMapping.customMapping(), which is why it is now explicitly marked with @SPI.

Also, if such is a wish, it is possible to create a descendant from AbstractColumnMapping, in spirit of port-based architecture, and use it to configure DatstoreStorageFactory. It has not changed, but generally speaking that would be strange to see from Spine users.

@armiol armiol merged commit 43e6946 into v1 Sep 27, 2023
2 checks passed
@armiol armiol deleted the reproduce-timestamp-column-clearance-issue branch September 27, 2023 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants