DDB Enhanced adding ChainConverterProvider #1746

cenedhryn · 2020-03-30T17:46:38Z

Description

Adds a 'chained' AttributeConverterProvider that can hold any number of user-supplied AttributeConverterProviders and will match converters using strict precedence, first-come-first-served.
It's also possible to supply an empty list to completely override the default AttributeConverterProvider provided by the library, in which case the user must add an AttributeConverter for each attribute in the schema
Both features available for StaticTableSchema and for DynamoDbBeans (BeanTableSchema).

For reference: Issue 35

Note: README updated separately in PR 1731.

Motivation and Context

Consistent behavior over both bean and static table schema
User-friendly syntax for bean

Testing

Unit tested

Screenshots (if appropriate)

Types of changes

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

Checklist

codecov-io · 2020-03-30T18:26:04Z

Codecov Report

Merging #1746 into master will increase coverage by 0.46%.
The diff coverage is 94.87%.

@@             Coverage Diff              @@
##             master    #1746      +/-   ##
============================================
+ Coverage     75.81%   76.28%   +0.46%     
  Complexity      182      182              
============================================
  Files          1069     1073       +4     
  Lines         32030    32235     +205     
  Branches       2489     2516      +27     
============================================
+ Hits          24285    24589     +304     
+ Misses         6511     6411     -100     
- Partials       1234     1235       +1

Flag	Coverage Δ	Complexity Δ
#unittests	`76.28% <94.87%> (+0.46%)`	`182.00 <0.00> (ø)`

Impacted Files	Coverage Δ	Complexity Δ
...nced/dynamodb/mapper/annotations/DynamoDbBean.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (?)`
.../enhanced/dynamodb/AttributeConverterProvider.java	`100.00% <100.00%> (ø)`	`0.00 <0.00> (ø)`
...odb/internal/converter/ChainConverterProvider.java	`100.00% <100.00%> (ø)`	`0.00 <0.00> (?)`
.../internal/converter/ConverterProviderResolver.java	`100.00% <100.00%> (ø)`	`0.00 <0.00> (?)`
...ssdk/enhanced/dynamodb/mapper/BeanTableSchema.java	`86.84% <100.00%> (+0.60%)`	`0.00 <0.00> (ø)`
...dk/enhanced/dynamodb/mapper/StaticTableSchema.java	`90.17% <100.00%> (+0.08%)`	`0.00 <0.00> (ø)`
...oviders/DefaultEndpointDiscoveryProviderChain.java	`66.66% <0.00%> (-33.34%)`	`0.00% <0.00%> (ø%)`
...modb/internal/converter/TypeConvertingVisitor.java	`59.37% <0.00%> (-28.13%)`	`0.00% <0.00%> (ø%)`
...sdk/services/s3control/S3ControlConfiguration.java	`46.87% <0.00%> (-18.13%)`	`6.00% <0.00%> (ø%)`
...re/amazon/awssdk/awscore/retry/AwsRetryPolicy.java	`85.71% <0.00%> (-14.29%)`	`0.00% <0.00%> (ø%)`
... and 65 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e37aa66...03632cc. Read the comment docs.

bmaizels · 2020-03-31T16:42:22Z

...java/software/amazon/awssdk/enhanced/dynamodb/internal/converter/ChainConverterProvider.java

+        return new ChainConverterProvider(providers);
+    }
+
+    public Queue<AttributeConverterProvider> chainedProviders() {


I think an [immutable] List would be the most appropriate collection to return here.

I might have gone overboard with utilizing the same patterns as for the extension chains :-)

bmaizels · 2020-03-31T16:43:50Z

...java/software/amazon/awssdk/enhanced/dynamodb/internal/converter/ChainConverterProvider.java

+        return this.providerChain.stream()
+                                 .filter(provider -> provider.converterFor(enhancedType) != null)
+                                 .map(p -> p.converterFor(enhancedType))
+                                 .findFirst().orElse(null);


nit: linebreak before second dot [.]

bmaizels · 2020-03-31T16:44:50Z

...java/software/amazon/awssdk/enhanced/dynamodb/internal/converter/ChainConverterProvider.java

+ */
+@SdkInternalApi
+public final class ChainConverterProvider implements AttributeConverterProvider {
+    private final Deque<AttributeConverterProvider> providerChain;


I'd have chosen ArrayList here as it's simpler and perfectly well suited to the only way we use it which is to iterate through its items in order.

bmaizels · 2020-03-31T16:48:39Z

...-enhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/mapper/BeanTableSchema.java

-        return converterClasses.length > 0 ?
-               Optional.of((AttributeConverterProvider) newObjectSupplierForClass(converterClasses[0]).get()) :
-               Optional.empty();
+    private static List<AttributeConverterProvider> converterProviderAnnotation(DynamoDbBean dynamoDbBean) {


The name of this method confuses me a bit. When I read it, I think that it's going to return the annotation object itself. Maybe call it 'createConverterProvidersFromAnnotation' ? or something like that.

I chose the name because of the prior art of naming similar methods in the same class, called from that particular method. I am somewhat on the same page as you, but then I'd rename all of the methods.

Yes feel free to rename anything that doesn't make sense, I was just looking at your changes.

bmaizels · 2020-03-31T16:54:06Z

...nhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/mapper/StaticTableSchema.java

+        private List<AttributeConverterProvider> attributeConverterProviders =
+            Collections.singletonList(ConverterProviderResolver.defaultConverterProvider());


As a general pattern I avoid having the Builder own the defaults to things, I prefer to see that in the constructor of the object. It can be tested and is more reliable there. Ideally the builder should do absolutely nothing just collect information and pass it to the constructor.

There is prior art here:

aws-sdk-java-v2/services-custom/dynamodb-enhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/internal/client/DefaultDynamoDbEnhancedClient.java

Lines 153 to 154 in c72d2b5

private List<DynamoDbEnhancedClientExtension> dynamoDbEnhancedClientExtensions =

new ArrayList<>(ExtensionResolver.defaultExtensions());

What should take precedence?

There's also prior art for the other way

aws-sdk-java-v2/services-custom/dynamodb-enhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/mapper/StaticTableSchema.java

Line 85 in c72d2b5

this.attributeConverterProvider = builder.attributeConverterProvider != null ?

The reality is we started off inconsistent and I believe we gravitated (at least I did) towards the way I'm now recommending.

Yes. I like that pattern better too, for the reasons you wrote.

Then I must set the list to null by default in the builder. Which is the best choice, null or using the default?

bmaizels · 2020-03-31T16:55:33Z

...nhanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/mapper/StaticTableSchema.java

+         * Calling this method will override the default attribute converter provider
+         * {@link DefaultAttributeConverterProvider}, which provides standard converters
+         * for most primitive and common Java types, so that provider must included in the supplied list if it is to be
+         * used. Providing an empty list here will cause no providers to get loaded.


I think this would be a good place to insert a code example of how to 'best' use the default one (since there are a number of different valid ways to instantiate it). I think we should encourage people to use the most friendly one.

That'd be not using the annotation at all. Right?!

StaticTableSchema should not use the annotation. I was thinking the defaultProvider() method on the base interface if it's still there, otherwise DefaultAttributeConverterProvider.create() or whatever.

Sorry, I was still in the bean space. Yes.

bmaizels · 2020-03-31T16:56:51Z

.../src/main/java/software/amazon/awssdk/enhanced/dynamodb/mapper/annotations/DynamoDbBean.java

+    Class<? extends AttributeConverterProvider>[] converterProviders()
+            default { NoAttributeConverterProviderOverride.class };


Maybe we should mention in the javadoc that passing an empty list as converterProviders results in no converters at all.

Yes, I'll add that

bmaizels · 2020-03-31T16:59:22Z

.../src/main/java/software/amazon/awssdk/enhanced/dynamodb/mapper/annotations/DynamoDbBean.java

+    final class NoAttributeConverterProviderOverride implements AttributeConverterProvider {
+        @Override
+        public <T> AttributeConverter<T> converterFor(EnhancedType<T> enhancedType) {
+            return null;


I think 'throw UnsupportedOperationException("This is not a real converter [or something to that effect]") ' would be more appropriate

You're right, will update

Actually let's just get rid of this and use the real DefaultAttributeConverterProvider

bmaizels · 2020-03-31T17:00:26Z

.../src/main/java/software/amazon/awssdk/enhanced/dynamodb/mapper/annotations/DynamoDbBean.java

+    Class<? extends AttributeConverterProvider>[] converterProviders()
+            default { NoAttributeConverterProviderOverride.class };
+
+    final class NoAttributeConverterProviderOverride implements AttributeConverterProvider {


By inheritance, this class is effectively public (which is why you're able to use it elsewhere in the code). That means we should either javadoc it or move it out of here and into internal namespace. I think I favor the latter but open to discussion.

Offline agreed to just use DefaultAttriburteConverterProvider instead (which will be a public class in this PR)

bmaizels · 2020-04-01T16:23:28Z

...rc/main/java/software/amazon/awssdk/enhanced/dynamodb/DefaultAttributeConverterProvider.java

@@ -80,7 +77,7 @@
 * Given an input, this will identify a converter that can convert the specific Java type and invoke it. If a converter cannot
 * be found, it will invoke a "parent" converter, which would be expected to be able to convert the value (or throw an exception).
 */
-@SdkInternalApi
+@SdkPublicApi


I think we should spruce up the javadoc a bit to celebrate this class' new status as 'public'. At a minimum mention (preferably with examples) how to use it in a StaticTableSchema builder and a BeanTableSchema annotation.

bmaizels · 2020-04-01T16:30:38Z

.../src/main/java/software/amazon/awssdk/enhanced/dynamodb/mapper/annotations/DynamoDbBean.java

+ * converter provided by the table schema.
+ * <p>
+ * Note:
+ * <ul>


I think it'd be nice to give an example here of using a single custom converter provider with default as fallback (as we did in the StaticTableSchema example).

sonarqubecloud · 2020-04-01T21:56:00Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities (and 0 Security Hotspots to review)
1 Code Smell

98.1% Coverage
0.0% Duplication

Remove createBucketAlreadyExists test

bmaizels reviewed Mar 31, 2020

View reviewed changes

bmaizels reviewed Apr 1, 2020

View reviewed changes

bmaizels approved these changes Apr 1, 2020

View reviewed changes

DDB Enhanced adding ChainConverterProvider

077b0a3

cenedhryn force-pushed the salande/enhanced-ddb-chained-converter-providers branch from e2e5624 to 077b0a3 Compare April 1, 2020 21:34

cenedhryn merged commit d4ea84f into master Apr 1, 2020

cenedhryn deleted the salande/enhanced-ddb-chained-converter-providers branch April 1, 2020 23:20

aws-sdk-java-automation pushed a commit that referenced this pull request Oct 6, 2021

Merge pull request #1746 from aws/zoewangg-patch-2

161a971

Remove createBucketAlreadyExists test

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DDB Enhanced adding ChainConverterProvider #1746

DDB Enhanced adding ChainConverterProvider #1746

cenedhryn commented Mar 30, 2020 •

edited

Loading

codecov-io commented Mar 30, 2020

bmaizels Mar 31, 2020

cenedhryn Mar 31, 2020

bmaizels Mar 31, 2020

bmaizels Mar 31, 2020

bmaizels Mar 31, 2020

cenedhryn Mar 31, 2020

bmaizels Mar 31, 2020

bmaizels Mar 31, 2020

cenedhryn Mar 31, 2020

bmaizels Mar 31, 2020

cenedhryn Mar 31, 2020

cenedhryn Mar 31, 2020

bmaizels Mar 31, 2020

cenedhryn Mar 31, 2020

bmaizels Mar 31, 2020

cenedhryn Mar 31, 2020

bmaizels Mar 31, 2020

cenedhryn Mar 31, 2020

bmaizels Mar 31, 2020

cenedhryn Mar 31, 2020

bmaizels Mar 31, 2020

bmaizels Mar 31, 2020

cenedhryn Mar 31, 2020

bmaizels Apr 1, 2020

bmaizels Apr 1, 2020

sonarqubecloud bot commented Apr 1, 2020 •

edited

Loading

		private List<AttributeConverterProvider> attributeConverterProviders =
		Collections.singletonList(ConverterProviderResolver.defaultConverterProvider());

	private List<DynamoDbEnhancedClientExtension> dynamoDbEnhancedClientExtensions =
	new ArrayList<>(ExtensionResolver.defaultExtensions());

		Class<? extends AttributeConverterProvider>[] converterProviders()
		default { NoAttributeConverterProviderOverride.class };

DDB Enhanced adding ChainConverterProvider #1746

DDB Enhanced adding ChainConverterProvider #1746

Conversation

cenedhryn commented Mar 30, 2020 • edited Loading

Description

Motivation and Context

Testing

Screenshots (if appropriate)

Types of changes

Checklist

codecov-io commented Mar 30, 2020

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sonarqubecloud bot commented Apr 1, 2020 • edited Loading

cenedhryn commented Mar 30, 2020 •

edited

Loading

sonarqubecloud bot commented Apr 1, 2020 •

edited

Loading