Allow string value for select for enhanced query request #5869
Conversation
...d/src/test/java/software/amazon/awssdk/enhanced/dynamodb/model/QueryEnhancedRequestTest.java
Outdated
Show resolved
Hide resolved
| /** | ||
| * Determines the attributes to be returned in the result. See {@link Select} for examples and constraints. | ||
| * If not found, returns {@link Select#UNKNOWN_TO_SDK_VERSION}. | ||
| * @param select the selection criteria as a string | ||
| * @return a builder of this type | ||
| */ |
There was a problem hiding this comment.
nit: this API doc seems to be copied from a getter, not setter? Can we update it
| * @return a builder of this type | ||
| */ | ||
| public Builder select(String select) { | ||
| this.select = Select.fromValue(select); |
There was a problem hiding this comment.
I think we need to change the select variable to string and pass it to the low level client to make it actually work. If customers pass an unsupported string, it'd be initialized as UNKNOWN_TO_SDK_VERSION
There was a problem hiding this comment.
Changed the implementation accordingly, please re-check - thanks!
| } | ||
|
|
||
| @Test | ||
| public void query_withInvalidStringSelect_returnsUnknown() { |
There was a problem hiding this comment.
Does this need to be an integration tests? Can we add functional tests (tests against dynamodb local) instead? BasicQueryTest?
There was a problem hiding this comment.
I have added also tests against dynamodb local in the new update commit - let me know if the current understanding is correct.
| @@ -122,7 +122,7 @@ public Boolean scanIndexForward() { | |||
| * Returns the value of select, or null if it doesn't exist. | |||
| * @return | |||
| */ | |||
| public Select select() { | |||
| public String select() { | |||
There was a problem hiding this comment.
This would be a breaking change, can we expose a new method selectAsString() instead? This is how it's done in low level QueryRequest as well https://sdk.amazonaws.com/java/api/latest/software/amazon/awssdk/services/dynamodb/model/QueryRequest.html#selectAsString()
| @@ -307,6 +307,17 @@ public Builder scanIndexForward(Boolean scanIndexForward) { | |||
| * @return a builder of this type | |||
| */ | |||
| public Builder select(Select select) { | |||
| this.select = select.toString(); | |||
There was a problem hiding this comment.
Can we add null check?
this.select = select == null ? null : select.toString();
…enum
Expected behaviour: allow adding a String instead of Select enum value in the projection expression and validate the options against the low level DynamoDB enum.
Motivation and Context
#5547
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