DynamoDB-enhanced: Add support for polymorphic subtypes to mapper

[bmaizels]

Should resolve https://github.com/aws/aws-sdk-java-v2/issues/1870

Motivation and Context

Will assist services that are attempting to implement single-table design in DynamoDB to be able to use the enhanced client to read entities of different subtypes (and schemata) with a single query.

Description

Changes are fully documented in README.md edit.

Testing

Unit tests and functional tests.

japicmp threw a false positive as two new methods were added to a public interface, but a default implementation was provided for both of them. I attempted to update the version of japicmp to 15.4, and that enabled this module to pass, but a different module then flagged the same rule. After discussion with @millems decision was made to disable the rule globally for now.

Types of changes

• [ ] Bug fix (non-breaking change which fixes an issue)
• [X] New feature (non-breaking change which adds functionality)

Checklist

• [X] I have read the CONTRIBUTING document
• [X] Local run of mvn install succeeds
• [X] My code follows the code style of this project
• [X] My change requires a change to the Javadoc documentation
• [X] I have updated the Javadoc documentation accordingly
• [X] I have read the README document
• [X] I have added tests to cover my changes
• [X] All new and existing tests passed
• [X] A short description of the change has been added to the CHANGELOG
• [ ] My change is to implement 1.11 parity feature and I have updated LaunchChangelog

License

• [X] I confirm that this pull request can be released under the Apache 2 license

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
16 Code Smells

95.0% Coverage
0.2% Duplication

[siwonred]

Is there any update? I'm really looking forward to this PR :-)

[bmaizels]

Sorry I've been away from this a while. One of the action items I took away from an internal review was to write an actual proof of concept test case that proves this is easy to use for single table design. As it turns out, there are some problems with it. The biggest problem is the current design forces you to map the subtype discriminator as a string attribute in your table which must be persisted in the database. I initially thought there was a workaround to this by using the afterRead and beforeWrite hooks in the extension framework to fudge it, but when I tried to actually build a POC around it, it turned out this simply isn't possible and even if it was it would not be meeting our usability goals. I had to go back to the drawing board a bit, and figure out a more flexible way of discriminating the entity and came up with a few proposals. I'd love to get feedback around which people prefer and find more intuitive.

Any of these options could be potentially combined, but increases the risk of usage confusion. Option 1b is the status quo.

Option 1a : Mapped discriminator type; discriminator values on subclass

@DynamoDbBean
@DynamoDbSupertype(subtypes = {Cat.class, Dog.class})
public class Animal {
  // ...
  
  @DynamoDbSubtypeDiscriminator
  public String getType() {
    // ...
  }
  
  public void setType(String type) {}
}

@DynamoDbBean
@DynamoDbSubtype("CAT")
class Cat {
  // ...
}

Option 1b : Mapped discriminator type; discriminator values on superclass

@DynamoDbBean
@DynamoDbSupertype({
   @Subtype(discriminatorValue = "CAT", subtypeClass = Cat.class),
   @Subtype(discriminatorValue = "DOG", subtypeClass = Dog.class) })
public class Animal {
  // ...
  
  @DynamoDbSubtypeDiscriminator
  public String getType() {
    // ...
  }
  
  public void setType(String type) {}
}

@DynamoDbBean
class Cat {
  // ...
}

Option 2a : Unmapped discriminator attribute; discriminator values on subclass

@DynamoDbBean
@DynamoDbSupertype(subtypes = {Cat.class, Dog.class},
                   discriminatorAttribute = "type")
public class Animal {
  // ...
}

@DynamoDbBean
@DynamoDbSubtype("CAT")
class Cat {
  // ...
}

Option 2b : Unmapped discriminator attribute; discriminator values on superclass

@DynamoDbBean
@DynamoDbSupertype(
   subtypes = {
     @Subtype(discriminatorValue = "CAT", subtypeClass = Cat.class),
     @Subtype(discriminatorValue = "DOG", subtypeClass = Dog.class)},
   discriminatorAttribute = "type")
public class Animal {
  // ...
}

@DynamoDbBean
class Cat {
  // ...
}

Option 3a : Discriminator function; discriminator values on subclass

@DynamoDbBean
@DynamoDbSupertype(subtypes = {Cat.class, Dog.class},
                   discriminatedBy = AnimalDiscriminator.class)
public class Animal {
  // ...
}

public class AnimalDiscriminator implements DynamoDbEntityDiscriminator {
   @Override
   public String computeDiscriminatorValue(Map<String, AttributeValue> entity) {
     // ...
   }
}

@DynamoDbBean
@DynamoDbSubtype("CAT")
class Cat {
  // ...
}

Option 3b : Discriminator function; discriminator values on superclass

@DynamoDbBean
@DynamoDbSupertype(
   subtypes = {
     @Subtype(discriminatorValue = "CAT", subtypeClass = Cat.class),
     @Subtype(discriminatorValue = "DOG", subtypeClass = Dog.class)},
   discriminatedBy = AnimalDiscriminator.class)
public class Animal {
  // ...
}

public class AnimalDiscriminator implements DynamoDbEntityDiscriminator {
   @Override
   public String computeDiscriminatorValue(Map<String, AttributeValue> entity) {
     // ...
   }
}

@DynamoDbBean
class Cat {
  // ...
}

[JudahBrick]

I like option 1b

[DavidSeptimus]

@bmaizels it sure has been a while. Thanks for providing an update.

With respect to your proposals, I'm generally in favor of the 'b' variant -- keeping the implementation pattern in alignment with the pattern used by Jackson's polymorphism implementation. In addition, by having all of the polymorphism configuration applied to the superclass, we're making it clear that the polymorphism configuration belongs to the superclass alone. It makes sense that if a consumer instantiates a separate TableSchema/Table using the subclass, none of that polymorphism configuration is applicable.

As for 1, 2, or 3:

• 1 is (in my opinion) the least optimal for a couple of key reasons:
  1. It relies on the consumer to correctly implement the type property in accordance with the intent of the library writer.
  2. Regardless of the correctness of type's implementation, there's a risk that any consumers of the polymorphic entity will mistake type for a typical mutable bean property and treat it as such in their code.
• 2 would be a solid default option that requires minimal effort for consumers to implement and keeps the discriminator attribute out of the entity class by default.
• 3 makes sense as an advanced option for anyone wanting to implement their own discriminator (e.g. extracting the type from a composite attribute as mentioned by @miguelcss above).

My vote would be for 2b + 3b and have the initialization code throw an exception if both discriminatorAttribute and discriminatedBy are defined in a single @DynamoDbSupertype declaration.

[bmaizels]

@DavidSeptimus I'm sold on 2 and 3 being generally better and more useful than 1, and I also agree about variant 'b' being conceptually more sound for the reasons you articulated very clearly. There's a complication with using a named attribute (option 2), and that is when converting from object to map there is no longer an explicit way to determine the entity type other than through class introspection. With option 1 this was not a problem because the entity type was mapped on the object and the mapper knew how to read it. There's an additional risk here that the actual class of the object is itself a child of the class that is a declared subtype, in which case I'll have to make sure the class chain is traversed until a match is found. No big deal really just a 'gotcha' I'll have to write behavior/tests for. I think it's time for me to get coding again and go for options 2b and 3b combined. Watch this space and thanks for your patience. By the way if anyone is frustrated by this and needs a good workaround I was linked a good blog that explains the workaround I would recommend: https://davidagood.com/dynamodb-enhanced-client-java-heterogeneous-item-collections/

[kaldav]

Is there any update? I'm really looking forward to this PR too!

[mpordomingo]

Hi all, are there any updates on this feature? I'm keen on using it too!