Surface API Review Document API [Review only] #3786
Conversation
* Adding new Interface EnhancedDocument * Fix review comments from Anna-karin and David * Addresed Zoe's comment
* DefaultEnhancedDocument implementation * Updated Null check in the conveter itself while iterating Arrays of AttributeValue * handled review comments * Update test cases for JsonAttributeCOnverter * Removed ctor and added a builder * Removed ctor for toBuilder
* DocumentTableSchema Implementation * Handle review comments -1 * Handle review comments 2
| } | ||
|
|
||
| /** | ||
| * Question 1 : Should we add API to input {@link TableMetadata} in DocumentTableSchema.Builder ? |
There was a problem hiding this comment.
I'm not sure what this means. Maybe a code snippet may help?
There was a problem hiding this comment.
two way door we can add latter , I will document why it is not required now
| } | ||
|
|
||
| /** | ||
| *- Does EnhancedDocument.Builder needs add(key, Object, attributeConverterProvider) |
There was a problem hiding this comment.
I'd say no. Seems error-prone since it takes a generic Object in the middle. Is there any reason we want to add it?
There was a problem hiding this comment.
not required , more details
| /** | ||
| *- Does EnhancedDocument.Builder needs add(key, Object, attributeConverterProvider) | ||
| * | ||
| * - Should we refer bytes as binary or byte in apis example addBinary(byte[] byte) or addByte(byte[] byte) |
There was a problem hiding this comment.
I'd say we should stick with SdkBytes
| * | ||
| * - Should we refer bytes as binary or byte in apis example addBinary(byte[] byte) or addByte(byte[] byte) | ||
| * | ||
| * - Map<String, Object> getRawMap(String attributeName); ==> Can we remove the use of Object ? <Zoe> |
There was a problem hiding this comment.
Same as List we cannot remove this because DDB maps can be of different type and need not be fixed to one particular type.
| * | ||
| * - Map<String, Object> getRawMap(String attributeName); ==> Can we remove the use of Object ? <Zoe> | ||
| * | ||
| * - getJsonPretty. Do we actually need this? |
| * - Object get(String attributeName); ==> Do we need this | ||
| * - We don't have jsonPretty utility elsewhere. Do we need it? | ||
| * | ||
| * - Builder add(String attributeName, Object value); ==> Using raw type is a bit code smell and error prone to me.Should it |
There was a problem hiding this comment.
+1, add(String attributeName, Object value) is not type safe and would defeat the purpose of all other methods IMO
There was a problem hiding this comment.
we might need it for adding beans, though, where we don't know the type but we have a configured attribute converter.
There was a problem hiding this comment.
Yeah, just realized it now 😅 . I wonder how we can prevent people from using add(String attributeName, Object value) for everything. Maybe we can rename it to something like addCustomType?
There was a problem hiding this comment.
putGeneric() ? putCustom() ? poll
putObject
| */ | ||
|
|
||
| DynamoDbTable<EnhancedDocument> table = | ||
| enhancedClient.table(tableName, TableSchema.fromDocumentSchemaBuilder() |
There was a problem hiding this comment.
fromDocumentSchemaBuilder to be consistent with other Builders in tableSchema
.../software/amazon/awssdk/enhanced/dynamodb/functionaltests/document/APISurfaceAreaReview.java
Show resolved
Hide resolved
| * {@link TableSchema} | ||
| * Changes : New API added | ||
| * Type : Interface | ||
| * package :software.amazon.awssdk.enhanced.dynamodbt |
| String tableName = getConcreteTableName("table-name"); | ||
| DynamoDbTable<EnhancedDocument> table = enhancedClient.table( | ||
| tableName, | ||
| TableSchema.fromDocumentSchemaBuilder() |
There was a problem hiding this comment.
maybe documentSchemaBuilder? I think the others have from* because they take a bean/annotated class.
| DynamoDbTable<EnhancedDocument> table = enhancedClient.table( | ||
| tableName, | ||
| TableSchema.fromDocumentSchemaBuilder() | ||
| .primaryKey("sampleHashKey", AttributeValueType.S) |
There was a problem hiding this comment.
partitionKey to match the annotation?
There was a problem hiding this comment.
+1, partition key also seems to be the term mostly used in other parts of the enhanced client.
| tableName, | ||
| TableSchema.fromDocumentSchemaBuilder() | ||
| .primaryKey("sampleHashKey", AttributeValueType.S) | ||
| .sortKey("sampleSortKey", AttributeValueType.S) |
There was a problem hiding this comment.
do we support secondary sort keys or global secondary indexes?
There was a problem hiding this comment.
Added the sortIndexKey same as StaticTableMetada will add functional test for this.
| .attributeConverterProviders(CustomAttributeConverterProvider.create(), | ||
| AttributeConverterProvider.defaultProvider()) |
There was a problem hiding this comment.
Do these apply to input documents (e.g. for putItem) or just output documents (e.g. from getItem)?
There was a problem hiding this comment.
Good question , thinking of it applies to only output documents like getItems. The input Documents need to specify the converters if any , explicitly in their builders
There was a problem hiding this comment.
Modified the code the merge Table Schema and Documents attribute converters
| * No attributeConverters supplied, in this case it uses the {@link DefaultAttributeConverterProvider} and does not error | ||
| */ | ||
| EnhancedDocument simpleDoc = EnhancedDocument.builder() | ||
| .addString("HashKey", "abcdefg123") |
There was a problem hiding this comment.
nit: s/add/put/ to match our naming conventions for map-like structures? "add" would be for lists/collections.
There was a problem hiding this comment.
add replaced with put
| assertThatExceptionOfType(IllegalStateException.class).isThrownBy(() -> simpleDoc.getBoolean("sdkByteSet")) | ||
| .withMessage("Value of attribute sdkByteSet of type " | ||
| + "EnhancedType(java.util.Collections$UnmodifiableRandomAccessList) " | ||
| + "cannot be converted into a Boolean value."); |
There was a problem hiding this comment.
we could do getString on almost anything though, right, since our StringConverter is pretty open to doing whatever?
There was a problem hiding this comment.
yes we use getString on any type .
Add functional test
...hanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/document/EnhancedDocument.java
Show resolved
Hide resolved
...hanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/document/EnhancedDocument.java
Outdated
Show resolved
Hide resolved
...hanced/src/main/java/software/amazon/awssdk/enhanced/dynamodb/document/EnhancedDocument.java
Show resolved
Hide resolved
| /** | ||
| * Gets the EnhancedType for the specified attribute key | ||
| * | ||
| * @param attributeName Name of the attribute. | ||
| * @return type of the specified attribute in the current item; or null if the attribute either doesn't exist or the attribute | ||
| * value is null. | ||
| */ | ||
| EnhancedType<?> getTypeOf(String attributeName); |
There was a problem hiding this comment.
Interesting, how do we know this?
There was a problem hiding this comment.
We first to a get("key") which returns the value as String, Number Boolean ,Map, List etc and the we do a EnhancedType.of(value.class)
There was a problem hiding this comment.
Bump on this. I don't think we should be able to know the type of an attribute. That should be up to the caller to tell us what to convert it to.
There was a problem hiding this comment.
Agree, Will remove this API .
|
|
||
| assertThat(simpleDoc.getSdkBytes("sdkByte")).isEqualTo(SdkBytes.fromUtf8String("a")); | ||
| assertThat(simpleDoc.getBoolean("booleanKey")).isTrue(); | ||
| assertThat(simpleDoc.getJson("jsonKey")).isEqualTo("{\"1\": [\"a\", \"b\", \"c\"],\"2\": 1}"); |
There was a problem hiding this comment.
Sorry it's not clear from the example, how is this different/better for JSON than add/getString()?
There was a problem hiding this comment.
getString just gets a Value of type Sting , where as getJson only works on attributes of type Map and converts to json key value pairs
| } | ||
|
|
||
| /** | ||
| *- Does EnhancedDocument.Builder needs add(key, Object, attributeConverterProvider) |
There was a problem hiding this comment.
Yeah I like this idea. How would it affect the table schema or table objects? Will they be updated?
There was a problem hiding this comment.
No , TableSchema is final
| * | ||
| * - Map<String, Object> getRawMap(String attributeName); ==> Can we remove the use of Object ? <Zoe> | ||
| * | ||
| * - getJsonPretty. Do we actually need this? |
| * | ||
| * - getJsonPretty. Do we actually need this? | ||
| * | ||
| * - Object get(String attributeName); ==> Do we need this |
There was a problem hiding this comment.
Java doc , about custom classes as maps
…h attribute converters are added to it (#3780)
| static DocumentTableSchema.Builder fromDocumentSchemaBuilder(){ | ||
| return DocumentTableSchema.builder(); | ||
| } |
There was a problem hiding this comment.
Should this be called documentSchemaBuilder?
There was a problem hiding this comment.
Yes, will update the code
| if (json == null) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Should we fail, instead? Same everywhere else we're doing this below.
There was a problem hiding this comment.
Added Validate.paramNotNull(json, "json") where null checks were done
| } | ||
| return DefaultEnhancedDocument.builder() | ||
| .json(json) | ||
| .attributeConverterProviders(DefaultAttributeConverterProvider.create()) |
There was a problem hiding this comment.
Hear me out: After playing with this a lot, I don't think we should have any attribute converters enabled on an EnhancedDocument by default.
We should do "late resolution" of the attribute converters in the SDK. We'd only use those converters when the get* methods are called on the EnhancedDocument. That allows us to attach the attribute converters the customer has configured on their table schema before we do any of the get* calls.
A drawback of that is if they create an enhanced document and try to do get* calls on it, it will fail because they have no attribute converters configured, but that isn't the primary use-case... and they can pretty easy just fix that by adding the default converters.
There was a problem hiding this comment.
I will remove it from DefaultEnhancedDocument.
But I think we require to implictly default out the converters for methods like fromJson and fromAttributeValueMap .
Input needed
- Pass providers in the static factory methods
However , if we still plan to make user to supply defaultProvider then which one do you think is a better option
If we plan to still provide it forfromJsonandfromAttributeValueMapthen we have following option
- static EnhancedDocument fromJson(String json, AttributeConverterProvider providers)
- static EnhancedDocument fromAttributeValueMap(Map<String, AttributeValue> attributeValueMap, AttributeConverterProvider providers)
- static EnhancedDocument fromMap(Map<String, Object> attributes, AttributeConverterProvider providers)
Or
- Remove the static factory method and just provide the Builders for it
EnhancedDocument.builder()
.json("json")
.addAttributeConverterProvider(AttributeConverterProvider.defaultProvider())
.build();
EnhancedDocument.builder()
.atributeValueMap(attributeValueMap)
.addAttributeConverterProvider(AttributeConverterProvider.defaultProvider())
.build();
- What about we throw a Validation error if User doesnot specify any AttributeConverterProvider while building the EnhancedDocument and in the error message we suggest user to specify the defaultConverter.
There was a problem hiding this comment.
I'm not sure why fromMap and fromJson would require an attribute converter. Let me play with it a bit to see if I can understand...
| return null; | ||
| } | ||
| DefaultEnhancedDocument.DefaultBuilder defaultBuilder = DefaultEnhancedDocument.builder(); | ||
| attributes.entrySet().forEach(key -> defaultBuilder.add(key.getKey(), key.getValue())); |
There was a problem hiding this comment.
Nit: Why not just attributes.forEach((k, v) -> ...)?
| * @return the value of the specified attribute in the current document as SdkBytes; or null if the attribute either | ||
| * doesn't exist or the attribute value is null. | ||
| */ | ||
| SdkBytes getSdkBytes(String attributeName); |
There was a problem hiding this comment.
Should we just call this getBytes? The customer might not know what SdkBytes are. Best prior art I could find is in transfer manager where ResumableFileDownload, Upload and Transfer have a method called serializeToBytes that returns SdkBytes and fromBytes that takes SdkBytes.
I couldn't find any methods where we call it "SdkBytes".
| private Boolean getBool(String attributeName) { | ||
| Object object = get(attributeName); | ||
| if (object instanceof Boolean) { | ||
| return (Boolean) object; | ||
| } | ||
| if (object instanceof String || object instanceof SdkNumber) { | ||
| if ("1".equals(object.toString())) { | ||
| return true; | ||
| } | ||
| if ("0".equals(object.toString())) { | ||
| return false; | ||
| } | ||
| return Boolean.valueOf((String) object); | ||
| } | ||
| throw new IllegalStateException("Value of attribute " + attributeName + " of type " + getTypeOf(attributeName) | ||
| + " cannot be converted into a Boolean value."); | ||
| } |
There was a problem hiding this comment.
BooleanAttributeConverter handles this already. We should be using attribute converters for every conversion we do.
| if (!isNullValueAdded(attributeName, value)) { | ||
| attributeValueMap.put(attributeName, AttributeValue.fromS(value)); | ||
| } |
There was a problem hiding this comment.
We definitely should be using attribute value converters for these methods as well.
There was a problem hiding this comment.
Changed the code. Removed for other instances too
| /** | ||
| * Converts AttributeValue to simple Java Objects like String, SdkNumber, SdkByte. | ||
| */ | ||
| public static Object toSimpleValue(AttributeValue value) { |
There was a problem hiding this comment.
Use the attribute converters, instead!!
There was a problem hiding this comment.
Same as above when we want to get List/Map of generics the AttributeConveters fails to get the converterFor(genericTypeOfInnerElement) because of "type erasure."
| * Utilities for working with {@link AttributeValue} and {@link EnhancedDocument} types. | ||
| */ | ||
| @SdkInternalApi | ||
| public final class DocumentUtils { |
There was a problem hiding this comment.
Almost this whole class looks like a case of "you should be using attribute converters instead"
There was a problem hiding this comment.
The problem with Attribute Converters is we cannot use AttributeConverters for List/Map Generics.
Example : in order to convert the List <List> the EnhancedType should be defined as new new EnhancedType<Map<String, List<List<String>>>>(){} however when want to pass a generic List or even when we want to get a generic List of List or nested Map List we loose the generic information because of "type erasure." Thus we need these additional API to extract the type of inner elements by inspecting it individually.
I was not able to solve the issue of "type erasure." with Attribute converters.
There was a problem hiding this comment.
Attribute converters are able to get around type erasure by using enhanced types. Let me do some prototyping to see if I can better understand the problems.
| * @param attributeName the name of the attribute that represents the partition key | ||
| * @param attributeValueType the {@link AttributeValueType} of the partition key | ||
| */ | ||
| public Builder primaryKey(String attributeName, AttributeValueType attributeValueType) { |
There was a problem hiding this comment.
partitionKey to match the static and bean table schemas?
| static DocumentTableSchema.Builder fromDocumentSchemaBuilder(){ | ||
| return DocumentTableSchema.builder(); | ||
| } |
There was a problem hiding this comment.
Yes, will update the code
| } | ||
| return DefaultEnhancedDocument.builder() | ||
| .json(json) | ||
| .attributeConverterProviders(DefaultAttributeConverterProvider.create()) |
There was a problem hiding this comment.
I will remove it from DefaultEnhancedDocument.
But I think we require to implictly default out the converters for methods like fromJson and fromAttributeValueMap .
Input needed
- Pass providers in the static factory methods
However , if we still plan to make user to supply defaultProvider then which one do you think is a better option
If we plan to still provide it forfromJsonandfromAttributeValueMapthen we have following option
- static EnhancedDocument fromJson(String json, AttributeConverterProvider providers)
- static EnhancedDocument fromAttributeValueMap(Map<String, AttributeValue> attributeValueMap, AttributeConverterProvider providers)
- static EnhancedDocument fromMap(Map<String, Object> attributes, AttributeConverterProvider providers)
Or
- Remove the static factory method and just provide the Builders for it
EnhancedDocument.builder()
.json("json")
.addAttributeConverterProvider(AttributeConverterProvider.defaultProvider())
.build();
EnhancedDocument.builder()
.atributeValueMap(attributeValueMap)
.addAttributeConverterProvider(AttributeConverterProvider.defaultProvider())
.build();
- What about we throw a Validation error if User doesnot specify any AttributeConverterProvider while building the EnhancedDocument and in the error message we suggest user to specify the defaultConverter.
| if (json == null) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Added Validate.paramNotNull(json, "json") where null checks were done
| return null; | ||
| } | ||
| DefaultEnhancedDocument.DefaultBuilder defaultBuilder = DefaultEnhancedDocument.builder(); | ||
| attributes.entrySet().forEach(key -> defaultBuilder.add(key.getKey(), key.getValue())); |
| * @return value of the specified attribute in the current document as a number; or null if the attribute either doesn't exist | ||
| * or the attribute value is null | ||
| */ | ||
| SdkNumber getSdkNumber(String attributeName); |
| * @return Map value of the specified attribute in the current document as EnhancedDocument or null if the attribute either | ||
| * doesn't exist or the attribute value is null. | ||
| */ | ||
| EnhancedDocument getMapAsDocument(String attributeName); |
| * @param value The boolean value that needs to be set. | ||
| * @return Builder instance to construct a {@link EnhancedDocument} | ||
| */ | ||
| Builder addBoolean(String attributeName, boolean value); |
| /** | ||
| * Appends an attribute of name attributeName with specified Set of {@link String} values to the document builder. | ||
| * | ||
| * @param attributeName Name of the attribute that needs to be added in the Document. | ||
| * @param values Set of String values that needs to be set. | ||
| * @return Builder instance to construct a {@link EnhancedDocument} | ||
| */ | ||
| Builder addStringSet(String attributeName, Set<String> values); | ||
|
|
||
| /** | ||
| * Appends an attribute of name attributeName with specified Set of {@link Number} values to the document builder. | ||
| * | ||
| * @param attributeName Name of the attribute that needs to be added in the Document. | ||
| * @param values Set of Number values that needs to be set. | ||
| * @return Builder instance to construct a {@link EnhancedDocument} | ||
| */ | ||
| Builder addNumberSet(String attributeName, Set<Number> values); | ||
|
|
||
| /** | ||
| * Appends an attribute of name attributeName with specified Set of {@link SdkBytes} values to the document builder. | ||
| * | ||
| * @param attributeName Name of the attribute that needs to be added in the Document. | ||
| * @param values Set of SdkBytes values that needs to be set. | ||
| * @return Builder instance to construct a {@link EnhancedDocument} | ||
| */ | ||
| Builder addSdkBytesSet(String attributeName, Set<SdkBytes> values); |
There was a problem hiding this comment.
No ,since Bytes , number, String are separtaely represented as Bs,Ns and SS we need these explicit APIs
| attributeValueMap = Collections.unmodifiableMap(objectMapToAttributeMap(builder.attributeValueMap, | ||
| attributeConverterProviders)); |
| .attributeConverterProviders(CustomAttributeConverterProvider.create(), | ||
| AttributeConverterProvider.defaultProvider()) |
There was a problem hiding this comment.
Modified the code the merge Table Schema and Documents attribute converters
joviegas
force-pushed
the
feature/master/joviegas_local_ddb_tests
branch
from
f023ef9 to
c9078fc
Compare
…3811) * TableSchema API to create table and functional tests * Surface API Review * Surface API Review - compilation issues * Surface API Review - Review comments * Surface API Review comments from Matt * Compilation issue and toStringMethod for JsonNode * Updated after handling Matt's comments * Functional Test added * Update in test cases * Removed functional tests , will create new PR for this * Review comments handled * Explicutly adding the dependency in th pom.xml * Removed @code from @snippet line in javadoc
* Remove extra spaces in Json and make it same as Items as in V1 * Moved Json string helper functions to seperate class
joviegas
force-pushed
the
feature/master/joviegas_local_ddb_tests
branch
from
c395ac2 to
b60c4fc
Compare
| .putString("direct_attr", "sample_value") | ||
| .putWithType("custom_attr", customObject, | ||
| EnhancedType.of(CustomClassForDocumentAPI.class)) | ||
| .putList( |
There was a problem hiding this comment.
putMapOfType is not here, but it should probably be putMap to be consistent with putList
| ) | ||
| .build(); | ||
|
|
||
| assertThat(customDoc.toJson()).isEqualTo("{\"direct_attr\": \"sample_value\", \"custom_attr\": {\"string\": \"str\"," |
There was a problem hiding this comment.
Looks like there is still space. Does this PR contain the change to remove it?
| CustomAttributeForDocumentConverterProvider.create(), | ||
| defaultProvider() | ||
| ) | ||
| .putString("direct_attr", "sample_value") |
There was a problem hiding this comment.
What's the expected behavior if users pass null?
EnhancedDocument simpleDoc = EnhancedDocument.builder()
.attributeConverterProviders(defaultProvider())
.putString("test", null).build();
System.out.println(simpleDoc.toJson());
Right now it treats it as null and prints {"test": null}
| .putNull("nullKey") | ||
| .putNumber("numberKey", 2.0) | ||
| .putBytes("sdkByte", SdkBytes.fromUtf8String("a")) | ||
| .putBoolean("booleanKey", true) |
There was a problem hiding this comment.
Should it be Builder putBoolean(String attributeName, boolean value);?
| EnhancedType.of(CustomClassForDocumentAPI.class)) | ||
| .build(); | ||
|
|
||
| // Note that Converter Not found exception is thrown even though we are accessing the string attribute |
There was a problem hiding this comment.
Should we fail at .build()?
|
|
||
| assertThat(simpleDoc.isPresent("HashKey")).isTrue(); | ||
| // No Null pointer or doesnot exist is thrown | ||
| assertThat(simpleDoc.isPresent("HashKey2")).isFalse(); |
There was a problem hiding this comment.
Looks like if an attribute doesn't exist, it will just return null. Is this the expected behavior? If so, we need to update the javadoc.
|
|
SonarCloud Quality Gate failed. |
|
These comments were addressed offline and in the final merge of this feature in the mainline. |
Motivation and Context
Modifications
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn installsucceedsscripts/new-changescript and following the instructions. Commit the new file created by the script in.changes/next-releasewith your changes.License