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

DynamoDB Enhanced Client support for Immutable objects #1801

Closed
bmaizels opened this issue Apr 30, 2020 · 11 comments
Closed

DynamoDB Enhanced Client support for Immutable objects #1801

bmaizels opened this issue Apr 30, 2020 · 11 comments
Labels
2.0 New dynamodb-enhanced feature-request A feature should be added or improved. kotlin

Comments

@bmaizels
Copy link
Contributor

This issue represents the desire for the DynamoDB Enhanced Client to be able to support Java idiomatic immutable data classes for mapping purposes. At time of writing only mutable 'bean'-like classes with standard getters and setters are supported.

Full support for immutables would likely include :

  • An implementation of TableSchema that supported the direct compile-time declaration of attributes for an immutable object, like the StaticTableSchema.
  • Would support the common Java idiom of a separate Builder class for the immutable object.
  • An implementation of TableSchema that supported introspection of an annotated class to build a static table schema, like the BeanTableSchema.
  • Would support inherited immutable (bonus if it can support mutable too) classes with abstract super like StaticTableSchema does.
  • Would support composed immutable (bonus if it can support mutable too) classes with flattened attributes like StaticTableSchema does.
@bmaizels bmaizels added feature-request A feature should be added or improved. needs-triage This issue or PR still needs to be triaged. labels Apr 30, 2020
@bmaizels bmaizels added 2.0 New and removed needs-triage This issue or PR still needs to be triaged. labels Apr 30, 2020
@bmaizels
Copy link
Contributor Author

So, I have a work-in-progress POC going on. It has ended up looking like this:

    private final TableSchema<ImmutableRecord> immutableTableSchema =
        TableSchema.builder(ImmutableRecord.class, ImmutableRecord.Builder.class)
                   .newItemBuilder(ImmutableRecord::builder, ImmutableRecord.Builder::build)
                   .addAttribute(String.class, a -> a.name("id")
                                                     .getter(ImmutableRecord::id)
                                                     .setter(ImmutableRecord.Builder::id)
                                                     .tags(primaryPartitionKey()))
                   .addAttribute(String.class, a -> a.name("attribute1")
                                                     .getter(ImmutableRecord::attribute1)
                                                     .setter(ImmutableRecord.Builder::attribute1))
                   .addAttribute(int.class, a -> a.name("attribute2")
                                                  .getter(ImmutableRecord::attribute2)
                                                  .setter(ImmutableRecord.Builder::attribute2))
                   .extend(TableSchema.builder(SuperRecord.class, SuperRecord.Builder.class)
                                      .addAttribute(String.class, a -> a.name("attribute3")
                                                                        .getter(SuperRecord::attribute3)
                                                                        .setter(SuperRecord.Builder::attribute3))
                                      .build())
                   .build();

I have been able to successfully implement 'extend' to work with an abstract immutable tableschema as long as it has a builder that is a superclass of the child builder. This is not completely intuitive to someone building these data classes, but this is how I accomplished it on the other side :

 // Lots of code cut for brevity
   public static class ImmutableRecord extends SuperRecord {
        public ImmutableRecord(Builder b) {
            super(b);
            this.id = b.id;
            this.attribute1 = b.attribute1;
            this.attribute2 = b.attribute2;
        }

        public static class Builder extends SuperRecord.Builder<Builder> {
            public Builder id(String id) { ... }
            public Builder attribute1(String attribute1) { ... }
            public Builder attribute2(int attribute2) { ... }
            public ImmutableRecord build() {
                return new ImmutableRecord(this);
            }
        }
    }

    public static class SuperRecord {
        public SuperRecord(Builder<?> b) {
            this.attribute3 = b.attribute3;
        }

        public String attribute3() {
            return attribute3;
        }

        public static class Builder<T extends Builder<T>> {
            private String attribute3;

            public T attribute3(String attribute3) {
                this.attribute3 = attribute3;
                return (T) this;
            }
        }
    }

Anyone have a better idea how we should support abstract immutables? Does anyone care? :)

@ntkoopman
Copy link

Would be nice if this can support records (when they get finalized)

@bmaizels
Copy link
Contributor Author

@ntkoopman Can you explain what you mean?

@bmaizels
Copy link
Contributor Author

So I had everything working in my POC except 'flatten'. Everything up to this point had been relatively straight forward once I grasped the idea of introducing a second (builder) class and the notion of type-asymmetry between mapping to an object and mapping from an object, however 'flatten' was implemented in a way that undermined that type-asymmetry as it needed to both read the nested object and mutate it as it converted a new flattened attribute. There was just no way to extend this same approach to work in the world of immutables so a complete rethink was necessary.

The good news is I have it working, this time instead of flattening and transforming the setters of individual attributes, the entire flattened tableschema is captured in a tree and there is something akin to a routing table for attributes so when it is translating it knows which attributes belong to a 'sub' table-schema and collects them all together and then delegates the translation entirely to that table-schema. The added benefit here is that any type of TableSchema can be nested (eg: have a flattened StaticTableSchema within an ImmutableTableSchema). This is a big win, so I have decided to go ahead with the third phase of the refactor:-

The third phase of the immutables refactor is to get StaticTableSchema to actually use the ImmutableTableSchema. What the StaticTableSchema does can now be described as a simplified variant of the ImmutableTableSchema where the builder type is the same as the item type and the 'build' action is essentially a no-op. In my head this works, will be putting it into practice later and hoping all the tests still pass.

@bmaizels
Copy link
Contributor Author

By the way if anyone wants to see what I'm upto here or play with the POC themselves and give immutable mapped objects a whirl, just check out this branch : https://github.com/aws/aws-sdk-java-v2/tree/bmaizels/ddbenhanced-immutables-poc

@ntkoopman
Copy link

I'm talking about https://openjdk.java.net/jeps/384 which is currently a preview feature in Java 14

@bstoney
Copy link

bstoney commented Jun 2, 2020

@bmaizels I cloned your POC and found it was straight forward to switch my lambda project from using BeanTableSchema to use the ImmutableTableSchema. I only needed to define the schema, build and all tests passed! Just a few pieces of feedback from my experience:

  • I am using classes and builders generated from Immutable annotated interfaces. It’s not clear if using an interface as a target for the table schema instead of a concrete immutable class was intended, but I would be keen to see support for this retained
  • Your note about the StaticTableSchema actually becoming a simple variant of the ImmutableTableSchema is neat and a great pro for the approach you’ve used
  • Defining the table schema for a composed immutable (or mutable) with a non-flattened structure is not obvious nor well documented, can you confirm if using an EnhancedType.documentOf attribute is the correct approach?
  • Defining the table schema by hand is quite verbose and I’d be interested how you see the introspected immutable table schema working? I’m assuming Java Bean introspection could work, but IMO doesn’t feel right
@Value.Style(
        builder = "new",
        defaults = @Value.Immutable(copy = false),
        visibility = Value.Style.ImplementationVisibility.PACKAGE,
        builderVisibility = Value.Style.BuilderVisibility.PACKAGE,
        jdkOnly = true,
        overshadowImplementation = true
)
@Value.Immutable
@JsonSerialize(as = Customer.class)
@JsonDeserialize(builder = Customer.Builder.class)
public interface Customer {
    String getAccountId();

    // ... snip

    String getName();

    class Builder extends ImmutableCustomer.Builder {
    }

    static Builder builder() {
        return new Builder();
    }

    static TableSchema<Customer> tableSchema() {
        return ImmutableTableSchema.builder(Customer.class, Customer.Builder.class)
                .newItemBuilder(Customer::builder, Customer.Builder::build)
                .addAttribute(String.class, a -> a.name("accountId")
                        .getter(Customer::getAccountId)
                        .setter(Customer.Builder::accountId)
                        .tags(primaryPartitionKey()))
                // ... snip
                .addAttribute(String.class, a -> a.name("name")
                        .getter(Customer::getName)
                        .setter(Customer.Builder::name)
                        .tags(secondaryPartitionKey("customers_by_name")))
                .build();
    }
}

Also somewhat related to my last comment, I have experimented with using an AttributeConverter to serialize/deserialize parts of the record model to simplify the table schema definition of a deeply composed DynamoDbBean structure. Do you have any feedback on whether this is a good solution? Pros: does not require schema updates if the model changes. Cons: serialization performance

@andrewparmet
Copy link

andrewparmet commented Jun 14, 2020

One thing I'd be interested to see is support for immutable Kotlin classes. These classes expose a single public constructor with each property, and no builder class. Today declaration of beans is wonderfully simple:

@DynamoDbBean
class DynamoEvent(
    @get:DynamoDbPartitionKey
    var objectId: UUID,

    @get:DynamoDbSortKey
    var version: Long,

    var created: Instant,

    // etc.
)

"Immutable support" in this context would mean changing each var declaration to val, which from the Java side simply removes the setter. Kotlin doesn't have built-in support for generating builders, and using annotation processors like Immutables, etc. might be a non-starter - given only those or manually rolling a builder as options, I might opt for my beans to remain mutable.

For reference, and I don't know exactly how it's been implemented or if it would be helpful, Jackson's Kotlin module does a good job for our other mapping use cases to immutable objects.

@bmaizels
Copy link
Contributor Author

Good news immutables fans! I found some time to finish up what I started here, and now have a feature preview branch that includes all the functionality I originally wanted to deliver for immutables including an annotated class version with introspection that can be used interchangably with the existing DynamoDb beans (you can have an immutable as a document inside a bean for example!). Please have a look and tell me what you think. I'll be working to release this as soon as possible.

#2012

@bmaizels
Copy link
Contributor Author

bmaizels commented Sep 2, 2020

This feature is now merged! Please give it a try and let us know what you think.

@bmaizels bmaizels closed this as completed Sep 2, 2020
@jehanzebqayyum
Copy link

Hi,
Great news. Could you please give an example on how this can be used in kotlin. Much appreciated thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.0 New dynamodb-enhanced feature-request A feature should be added or improved. kotlin
Projects
None yet
Development

No branches or pull requests

6 participants