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

DDB Enhanced:Added support to read Nested objects in attributesToProj… #2035

Merged
merged 1 commit into from Sep 23, 2020
Merged

DDB Enhanced:Added support to read Nested objects in attributesToProj… #2035

merged 1 commit into from Sep 23, 2020

Conversation

joviegas
Copy link
Contributor

@joviegas joviegas commented Sep 14, 2020

Description DDB Enhanced: Added support for Nested objects projection expressions RequestOnPR

Motivation and Context

Fix

  • Added a new Class NestedAttributeName , this can be used to represent Nested Variables

/**
* Public API methods for creating NestedAttributeName
*/

//Simple Attribute creation "foo"
NestedAttributeName s1 = AttributeName.builder().elements("foo").build();
s1 = NestedAttributeName.builder().addElement("foo").build()
s1 = NestedAttributeName.create("foo")

//Nested Attribute creation "foo.bar"
s2 = NestedAttributeName.builder().elements("foo", "bar").build();
s2 = NestedAttributeName.create("foo", "bar");


//Getter Output for s1
s1.elements() = ["foo"]
//Getter Output for s2
s2.elements() = ["foo", "bar"]

EnhancedRequest

/**
*SAMPLE Builder usage And GETTERS for EnhancedRequest
*/   

/***Case1*: One Attribute and attributesToProject(String) used**/
 final QueryEnhancedRequest q1 = 
 QueryEnhancedRequest.builder().attributesToProject("foo").build();
 
 q1.attributesToProject() = ["foo"].
 q1.nestedAttributeNameToProject() = ["foo"]
 
 /***Case2* :One Nested Attribute **/
 final QueryEnhancedRequest q1 = 
 QueryEnhancedRequest.builder()
    .addNestedAttributesToProject(NestedAttributeName.create("foo", "bar")).build();
 
 q1.attributesToProject() = ["foo.bar"].
 q1.nestedAttributeNameToProject() = [["foo","bar"]]
 
  /***Case3* :One Nested Attribute and One Non Nested Attribute **/
 final QueryEnhancedRequest q1 = 
 QueryEnhancedRequest.builder()
    .attributesToProject("bazz")
    .addNestedAttributesToProject(NestedAttributeName.create("foo", "bar")).build();
 
 q1.attributesToProject() = ["bazz","foo.bar"].
 q1.nestedAttributeNameToProject() = [["bazz"], ["foo","bar"]]
 
 
 

Testing

Added JUnit for following casses
1.Projection of a nested Attribute .
2. Projection of a nested Attribute and not nested attribute (using older builder method).
3. Projection of Multiple Attributes
4. Projection of Null attributes passing Null NestedAttributeName --> Returns all elements as per the doc.

  1. Tested getter for attributeForProject.

Screenshots (if appropriate)

Types of changes

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

Checklist

  • I have read the CONTRIBUTING document
  • Local run of mvn install succeeds
  • My code follows the code style of this project
  • My change requires a change to the Javadoc documentation
  • I have updated the Javadoc documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed
  • 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

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

/**
* @return List of nested attributes , each entry in the list represent one level of nesting.
*/
public List getNestedAttributeNames() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you returning a raw-type list here?

private String attributeToProject;

private Builder() {

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: remove this empty line.

private final String name;
private final List<String> nestedAttributeNames;

private AttributeName(String name, List<String> nestedAttributeNames) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: Our standard pattern is to have a constructor that takes an instance of a builder. Eg:

private AttributeName(Builder b) { ... }

Comment on lines 50 to 51
private final List<String> attributesToProject;
private final List<AttributeName> attributeNamesToProject;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My opinion is that we should deprecate the List and just have a List. That will simplify our behavioral logic around this.

@dagnir
Copy link
Contributor

dagnir commented Sep 14, 2020

Looks like the previous PR was closed; for future PR's please use the existing pull request when making updates so we can maintain the history

@@ -347,7 +360,101 @@ public Builder addAttributeToProject(String attributeToProject) {
return this;
}

/**
* <p>
* Sets a collection of the NestedAttributeNames to be retrieved from the database. These attributes can include
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Change 'sets' to 'adds'

@@ -192,7 +206,8 @@ public int hashCode() {
private Integer limit;
private Boolean consistentRead;
private Expression filterExpression;
private List<String> attributesToProject;
private List<String> attributesToProjectStringList;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

attributesToProjectStringList attribute is required to maintain the order of the older attributeToProject in cases of resets.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the order important? Conceptually I think of this as a 'set'.

Comment on lines 45 to 46
this.elements = nestedAttributeNames != null
? Collections.unmodifiableList(nestedAttributeNames) : null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having null elements should be invalid. Can we add a validator here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added for Null as well as Empty list

Comment on lines 137 to 139
return attributesToProject != null ? attributesToProject.stream().filter(Objects::nonNull)
.map(item -> String.join(".", item.elements())).collect(Collectors.toList()) : null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like we should add validation to the constructor of the class to avoid the possibility of having null elements in attributesToProject then there would be no need to filter them here.

@@ -192,7 +206,8 @@ public int hashCode() {
private Integer limit;
private Boolean consistentRead;
private Expression filterExpression;
private List<String> attributesToProject;
private List<String> attributesToProjectStringList;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the order important? Conceptually I think of this as a 'set'.

//If there is a reset then clear the attributesToProject List.
if (attributesToProjectStringList != null) {
attributesToProjectStringList
.forEach(attribute -> this.attributesToProject.remove(NestedAttributeName.create(attribute)));
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is really confusing. We don't want to treat these as separate lists. We should make it clear in the javadoc that addNestedAttributeToProject is adding a 'nested attribute' to the 'attributesToProject' list. Let's make it clear to the customer there is just one list, and let's treat it like one list.

Copy link
Contributor Author

@joviegas joviegas Sep 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

okay.

.addNestedAttributesToProject(NestedAttributeName.create("foo", "bar"))
.attributesToProject(additionalElement)
.build();
List<String> attributesToProjectStringAttributeOverwrittenNestedLast = Arrays.asList( "foo.bar", "three");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my other comment in the code, I think the 'correct' state here should be "three", and the nested attribute should be overwritten with the others.

@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2020

Codecov Report

Merging #2035 into master will increase coverage by 0.60%.
The diff coverage is 82.88%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2035      +/-   ##
============================================
+ Coverage     76.61%   77.22%   +0.60%     
  Complexity      225      225              
============================================
  Files          1113     1136      +23     
  Lines         33617    34841    +1224     
  Branches       2600     2726     +126     
============================================
+ Hits          25756    26906    +1150     
+ Misses         6581     6577       -4     
- Partials       1280     1358      +78     
Flag Coverage Δ Complexity Δ
#unittests 77.22% <82.88%> (+0.60%) 225.00 <0.00> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ Complexity Δ
...on/awssdk/codegen/maven/plugin/GenerationMojo.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
.../software/amazon/awssdk/codegen/CodeGenerator.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...are/amazon/awssdk/codegen/emitters/CodeWriter.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
.../amazon/awssdk/codegen/emitters/GeneratorTask.java 0.00% <0.00%> (ø) 0.00 <0.00> (?)
...zon/awssdk/codegen/emitters/PoetGeneratorTask.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...n/awssdk/codegen/emitters/SimpleGeneratorTask.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...egen/emitters/tasks/AsyncClientGeneratorTasks.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...ssdk/codegen/emitters/tasks/AwsGeneratorTasks.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
...itters/tasks/BaseExceptionClassGeneratorTasks.java 0.00% <ø> (ø) 0.00 <0.00> (ø)
...sdk/codegen/emitters/tasks/BaseGeneratorTasks.java 0.00% <0.00%> (ø) 0.00 <0.00> (ø)
... and 185 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 dc695de...d429e78. Read the comment docs.

…ect Enhanced Scan and Enhanced query requests
@sonarcloud
Copy link

sonarcloud bot commented Sep 21, 2020

SonarCloud Quality Gate failed.

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities (and Security Hotspot 0 Security Hotspots to review)
Code Smell A 0 Code Smells

86.6% 86.6% Coverage
8.2% 8.2% Duplication

warning The version of Java (1.8.0_252) you have used to run this analysis is deprecated and we will stop accepting it from October 2020. Please update to at least Java 11.
Read more here

@joviegas joviegas merged commit bc13f17 into aws:master Sep 23, 2020
aws-sdk-java-automation pushed a commit that referenced this pull request Sep 21, 2022
Add support for bucket ARN parsing for object versioning APIs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants