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

Provide a clearer exception message when building a ReadBatch or WriteBatch without setting the mappedTableResource (table) #4267

Conversation

swar8080
Copy link
Contributor

@swar8080 swar8080 commented Aug 6, 2023

Currently, a NullPointerException is thrown if you forget to call the WriteBatch builder with mappedTableResource(MappedTableResource<T>). ReadBatch has the same problem

Motivation and Context

    DynamoDbEnhancedClient client ...
    DynamoDbTable<Customer> customerTable = client.table("customer-table", customerSchema);
    Customer customer = ...

    // This will fail with a NullPointerException, both for addPutItem* and addDeleteItem*
    client.batchWriteItem(builder -> builder.addWriteBatch(
        WriteBatch.builder(Customer.class)
            .addPutItem(customer)
            .build()
    ));

   // This works correctly
   client.batchWriteItem(builder -> builder.addWriteBatch(
        WriteBatch.builder(Customer.class)
            .mappedTableResource(customerTable)
            .addPutItem(customer)
            .build()
    ));

Modifications

Add a clearer error message in the NullPointerException letting the person know they forgot to set a mappedTableResource

Testing

Updated unit tests

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

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 added tests to cover my changes
  • All new and existing tests passed
  • I have added a changelog entry. Adding a new entry must be accomplished by running the scripts/new-change script and following the instructions. Commit the new file created by the script in .changes/next-release with your changes.

License

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

@swar8080 swar8080 requested a review from a team as a code owner August 6, 2023 18:13
@swar8080 swar8080 changed the title Make it clearer which fields are required when building a DynamoDB En… Make it clearer which fields are required when building a DynamoDB Enhanced Client WriteBatch Aug 6, 2023
@swar8080 swar8080 force-pushed the swar8080/dynamodb-enhanced-client-WriteBatch-builder-safety branch from 58b633f to 2fb4633 Compare August 6, 2023 18:31
@debora-ito
Copy link
Member

@swar8080 I understand the problem, but I don't think we should deprecate the current builder for this.
Do you think a better error message in case of a null mappedTableResource would be helpful?

@swar8080 swar8080 force-pushed the swar8080/dynamodb-enhanced-client-WriteBatch-builder-safety branch from 2fb4633 to eba498d Compare August 13, 2023 12:20
…teBatch without setting the mappedTableResource (table)
@swar8080 swar8080 force-pushed the swar8080/dynamodb-enhanced-client-WriteBatch-builder-safety branch from eba498d to 6f08eb3 Compare August 13, 2023 12:21
@swar8080 swar8080 changed the title Make it clearer which fields are required when building a DynamoDB Enhanced Client WriteBatch Provide a clearer exception message when building a ReadBatch or WriteBatch without setting the mappedTableResource (table) Aug 13, 2023
@swar8080
Copy link
Contributor Author

@debora-ito okay I just updated the NullPointerException's message and got rid of the other changes

@debora-ito debora-ito added the needs-review This issue or PR needs review from the team. label Aug 15, 2023
Copy link
Contributor

@dagnir dagnir left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the contribution!

@sonarcloud
Copy link

sonarcloud bot commented Sep 25, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication

@dagnir dagnir merged commit db9ea14 into aws:master Sep 26, 2023
1 of 7 checks passed
@dagnir
Copy link
Contributor

dagnir commented Sep 26, 2023

@all-contributors please add @swar8080 for code

@allcontributors
Copy link
Contributor

@dagnir

I've put up a pull request to add @swar8080! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-review This issue or PR needs review from the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants