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.

Sign up for GitHub

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

Jump to bottom

fix(DynamoDBv2): check for properties marked required when loading objects via DynamoDBContext #3277

Closed
wants to merge 1 commit into from
Closed

fix(DynamoDBv2): check for properties marked required when loading objects via DynamoDBContext #3277

wants to merge 1 commit into from

Conversation

mstern02
Copy link

@mstern02 mstern02 commented Apr 9, 2024 •
edited
Loading

Description

Add conditionally compiled checks for RequiredMemberAttribute, which is generated by the compiler for class members marked with the required keyword.

Motivation and Context

C# 11 introduced the required keyword to signal to the compiler that class members must be initialized using an object initializer.

This is also used by System.Text.Json.JsonSerializer to perform data validation at runtime.

When retrieving a POCO from DynamoDB via DynamoDBContext this modifier is ignored, making it possible to create invalid objects accidentally.

Fixes #3276

Testing

  1. Create the following DynamoDB Table:
Email (string, pk) DisplayName (string)
[email protected] Fish One
[email protected] null
[email protected] no value
  1. Apply the following patch to the main branch using git apply:
diff --git a/testapp/Program.cs b/testapp/Program.cs
new file mode 100644
index 00000000000..c12fd3b4a30
--- /dev/null
+++ b/testapp/Program.cs
@@ -0,0 +1,16 @@
+using Amazon.DynamoDBv2;
+using Amazon.DynamoDBv2.DataModel;
+
+var client = new AmazonDynamoDBClient();
+var context = new DynamoDBContext(client);
+
+var users = await context.ScanAsync<User>([], new() {OverrideTableName = Environment.GetEnvironmentVariable("DDB_TABLE")}).GetRemainingAsync();
+
+Console.WriteLine(string.Join('\n', users.Select(user => user.DisplayName.ToUpper())));
+
+// Class has two properties, Email and DisplayName, that should never be null.
+class User {
+    [DynamoDBHashKey]
+    public required string Email {get; set;}
+    public required string DisplayName {get; set;}
+}
\ No newline at end of file
diff --git a/testapp/testapp.csproj b/testapp/testapp.csproj
new file mode 100644
index 00000000000..db7d421377c
--- /dev/null
+++ b/testapp/testapp.csproj
@@ -0,0 +1,20 @@
+<Project Sdk="Microsoft.NET.Sdk">
+
+  <ItemGroup>
+    <ProjectReference Include="..\sdk\src\Services\S3\AWSSDK.S3.NetStandard.csproj" />
+    <ProjectReference Include="..\sdk\src\Services\DynamoDBv2\AWSSDK.DynamoDBv2.NetStandard.csproj" />
+  </ItemGroup>
+
+  <ItemGroup>
+    <PackageReference Include="AWSSDK.IdentityManagement" Version="3.7.300.58" />
+    <PackageReference Include="AWSSDK.SecurityToken" Version="3.7.300.59" />
+  </ItemGroup>
+
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <TargetFramework>net8.0</TargetFramework>
+    <ImplicitUsings>enable</ImplicitUsings>
+    <Nullable>enable</Nullable>
+  </PropertyGroup>
+
+</Project>
  1. Run the example
cd ./testapp
DDB_TABLE="<name of table>" dotnet run
  1. After these changes the program should fail with an InvalidOperationException thrown in the library, whereas in the current version an unexpected NullReferenceException is thrown in the application code.

Screenshots (if appropriate)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • My code follows the code style of this project
  • My change requires a change to the documentation
  • I have updated the documentation accordingly
  • I have read the README document
  • I have added tests to cover my changes
  • All new and existing tests passed

License

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

@mstern02 mstern02 mentioned this pull request Apr 9, 2024
Open
@dscpinheiro dscpinheiro changed the base branch from main to main-staging April 10, 2024 15:42
@normj
Copy link
Member

normj commented Apr 19, 2024

My understanding of the change is this would prevent saving POCOs with the required keyword and that makes sense. But it looks like it is also blocking loading POCO that would violate the required keyword. That seems bad to give users no way to access their data. Maybe if there was a config option on the context config that would allow turning off the required check to give people a back door just in case something was going weird for them. I might be misunderstanding the situation for loading options today and it fails already because I don't think we are setting the required properties as part of the object initializer.

@normj
Copy link
Member

normj commented Jun 26, 2024

Closing for lack of activity.

@normj normj closed this Jun 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

DynamoDB Object persistence Model does not respect required keyword
2 participants
@mstern02 @normj