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

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

Draft
wants to merge 1 commit into
base: main-staging
Choose a base branch
from

Conversation

mstern02
Copy link

@mstern02 mstern02 commented Apr 9, 2024

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)
onefish@example.com Fish One
twofish@example.com null
redfish@example.com 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

@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.

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.

DynamoDB Object persistence Model does not respect required keyword
2 participants