Remove the Context/Operation Config Hierarchy, and Remove Table-level Configurations from DynamoDBOperationConfig #3422

ashovlin · 2024-08-05T16:17:34Z

Description

~~This allows users to set OverrideTableName on the DynamoDBContextConfig, not just the DynamoDBOperationConfig~~
- During review we decided against this.
~~While I was here I noticed that I didn't follow the hierarchy when adding DisableFetchingTableMetadata last year, so backfilled that too.~~

New changes after PR feedback:

Separate the hierarchy between DynamoDBContextConfig and DynamoDBOperationConfig, to remove the appearance that you can pass an operation config into the context. This came up several times with OverrideTableName.
Removed two table-level configurations from DynamoDBOperationConfig (and thus all of the new operation-specific config objects). These control how the context object caches table metadata, and should be specified there instead of at the operation level.

Motivation and Context

We've gotten a few requests over the years for the ability to override the DynamoDB table name via the object-persistence context config, instead of needing to do so on every operation config.

This is complicated by the fact that DynamoDBOperationConfig inherits from DynamoDBContextConfig. One could be passing an operation config into the context constructor, thinking that OverrideTableName would apply.

aws-sdk-net/sdk/src/Services/DynamoDBv2/Custom/DataModel/Configs.cs

Line 153 in 2b0190e

public class DynamoDBOperationConfig : DynamoDBContextConfig

Testing

Ran unit and integration tests in DynamoDBv2 sln locally.
Dry-run in progress.

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

ashovlin · 2024-08-05T16:19:50Z

sdk/src/Services/DynamoDBv2/Custom/DataModel/Configs.cs

@@ -152,12 +162,6 @@ public DynamoDBContextConfig()
 #endif
    public class DynamoDBOperationConfig : DynamoDBContextConfig


Should we break this hierarchy while we're here?

We're discouraging the use of DynamoDBOperationConfig in V4 via [Obsolete] #3421, but this did lead to the confusion that this is addressing in the first place.

I think it make sense to break the hierarchy. I assume we would copy over the properties from DynamoDBContextConfig to DynamoDBOperationConfig that made sense at the operation level.

Done in f478a1b

normj · 2024-08-08T21:48:12Z

sdk/src/Services/DynamoDBv2/Custom/DataModel/Configs.cs

+        /// If you specify this on <see cref="DynamoDBContextConfig"/>, then it will apply to all 
+        /// objects/tables used with that <see cref="DynamoDBContext"/> object unless overriden by an operation-specific config.
+        /// </remarks>
+        public string OverrideTableName { get; set; }


Having this property on the context's config doesn't make sense to me since the context works with multiple tables. I know you call that out in the doc that it replaces all table mappings. I'm thinking it was a failure in our response to the GitHub issues to not point out that could accomplish what they wanted using the AWSConfigsDynamoDB.Context.TypeMappings property. Here is how I configured one of my application to use the table that was created during deployment as part of CloudFormation with CloudFormation naming the table.

var tableName = System.Environment.GetEnvironmentVariable(ENV_TABLE_NAME_POLL_DEFINITION); if (!string.IsNullOrEmpty(tableName)) { PollDefinitionTableName = tableName; AWSConfigsDynamoDB.Context.TypeMappings[typeof(PollDefinition)] = new Amazon.Util.TypeMapping(typeof(PollDefinition), tableName); Console.WriteLine($"Using table {tableName} for PollDefinition"); }

If we really want this capability on the context object I think we would have to make it a map of types to table names.

I wasn't too attached to this yet either, was mostly throwing it out there since there seems to be interest via the three issues. The only counterargument I can think of is if an application is only working with a single table, the current approach is simpler to override.

Should we just point users to AWSConfigsDynamoDB.Context.TypeMappings? and then break the hierarchy so it stops looking like you could be able to do this? Or maybe we can leave this PR out for a bit for feedback, I could go comment on the original issues.

Yeah I think we should point users to AWSConfigsDynamoDB.Context.TypeMappings. You are right that the single property would be useful in a single table design but then confusing in the other case. I don't have a feeling how common single table design is but I don't believe it would be the overwhelming majority to warrant putting in this confusing property for non single table users.

You could add a comment on the OverrideTableName for the operational config that if users want to make the override global they should use the AWSConfigsDynamoDB.Context.TypeMappings.

Done in f478a1b.

Plus as long as we do the breaking change to separate the hierarchy in V4, if we did want to add a context-level table mapping, that'd be an additive change in the future.

normj · 2024-08-08T21:50:31Z

sdk/src/Services/DynamoDBv2/Custom/DataModel/Configs.cs

@@ -152,12 +162,6 @@ public DynamoDBContextConfig()
 #endif
    public class DynamoDBOperationConfig : DynamoDBContextConfig


I think it make sense to break the hierarchy. I assume we would copy over the properties from DynamoDBContextConfig to DynamoDBOperationConfig that made sense at the operation level.

ashovlin · 2024-08-10T00:24:35Z

sdk/src/Services/DynamoDBv2/Custom/DataModel/Configs.cs

+        /// If you want to specify this globally instead of for each operation, you can use 
+        /// the <see cref="TableAlias"/> or <see cref="TypeMapping"/> collections 
+        /// on <see cref="AWSConfigsDynamoDB.Context"/>.
+        /// </remarks>


New note the suggests an alternative to the operation-level override.

normj · 2024-08-10T01:11:17Z

sdk/src/Services/DynamoDBv2/Custom/DataModel/Configs.cs

+        /// table metadata. It requires that the table's index schema be accurately described via the above methods, 
+        /// otherwise exceptions may be thrown and/or the results of certain DynamoDB operations may change.
+        /// </remarks>
+        public bool? DisableFetchingTableMetadata { get; set; }


Does this make sense at the operational level? Since only one operation invoke is going to do the cache load for a table would a user even now when the load is happening or not.

Maybe that's why I left it off originally... Agreed that you wouldn't need to set this past the first one.

I can remove it again, especially in parallel with #3430

Done in 295e52e

normj · 2024-08-10T01:44:34Z

sdk/src/Services/DynamoDBv2/Custom/DataModel/Configs.cs

+        /// For <see cref="MetadataCachingMode.TableNameOnly"/> the cache key will only consist of the table name. This reduces cache misses in contexts
+        /// where you are accessing tables with identical structure but using different credentials or endpoints (such as a multi-tenant application).
+        /// </remarks>
+        public MetadataCachingMode? MetadataCachingMode { get; set; }


Like DisableFetchingTableMetadata this property doesn't make sense at the operational level.

Done in 295e52e

… specified on a DynamoDBOperationConfig, instead of just on the DynamoDBContextConfig

…documentation

ashovlin commented Aug 5, 2024

View reviewed changes

dscpinheiro added the v4 label Aug 5, 2024

ashovlin requested review from 96malhar, philasmar and normj August 6, 2024 16:41

normj requested changes Aug 8, 2024

View reviewed changes

ashovlin force-pushed the shovlia/ddb-context-level-table-1421 branch from 7236fff to f478a1b Compare August 10, 2024 00:19

ashovlin requested a review from normj August 10, 2024 00:21

ashovlin commented Aug 10, 2024

View reviewed changes

normj reviewed Aug 10, 2024

View reviewed changes

ashovlin changed the title ~~fix: Honor DisableFetchingTableMetadata when specified on a DynamoDBOperationConfig, and remove the Context/Operation Config Hierarchy~~ Remove the Context/Operation Config Hierarchy, and Remove Table-level Configurations from DynamoDBOperationConfig Aug 10, 2024

ashovlin requested a review from normj August 10, 2024 02:18

normj approved these changes Aug 10, 2024

View reviewed changes

philasmar approved these changes Aug 12, 2024

View reviewed changes

ashovlin added 3 commits August 14, 2024 10:56

fix: Honor an OverrideTableName and DisableFetchingTableMetadata when…

1c1d7b5

… specified on a DynamoDBOperationConfig, instead of just on the DynamoDBContextConfig

Separate DynamoDBContextConfig and DynamoDBOperationConfig, clean up …

e11b6e5

…documentation

Remove table caching properties from operation specific configs.

cd2e44a

ashovlin force-pushed the shovlia/ddb-context-level-table-1421 branch from 295e52e to cd2e44a Compare August 14, 2024 15:19

ashovlin merged commit 15a8661 into v4-development Aug 14, 2024
1 check passed

ashovlin deleted the shovlia/ddb-context-level-table-1421 branch August 14, 2024 19:32

ashovlin mentioned this pull request Aug 14, 2024

V4 Development Tracker #3362

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the Context/Operation Config Hierarchy, and Remove Table-level Configurations from DynamoDBOperationConfig #3422

Remove the Context/Operation Config Hierarchy, and Remove Table-level Configurations from DynamoDBOperationConfig #3422

ashovlin commented Aug 5, 2024 •

edited

Loading

ashovlin Aug 5, 2024

normj Aug 8, 2024

ashovlin Aug 10, 2024

normj Aug 8, 2024

ashovlin Aug 8, 2024 •

edited

Loading

normj Aug 9, 2024

normj Aug 9, 2024

ashovlin Aug 10, 2024

normj Aug 8, 2024

ashovlin Aug 10, 2024

normj Aug 10, 2024

ashovlin Aug 10, 2024

ashovlin Aug 10, 2024

normj Aug 10, 2024

ashovlin Aug 10, 2024

		@@ -152,12 +162,6 @@ public DynamoDBContextConfig()
		#endif
		public class DynamoDBOperationConfig : DynamoDBContextConfig

Remove the Context/Operation Config Hierarchy, and Remove Table-level Configurations from DynamoDBOperationConfig #3422

Remove the Context/Operation Config Hierarchy, and Remove Table-level Configurations from DynamoDBOperationConfig #3422

Conversation

ashovlin commented Aug 5, 2024 • edited Loading

Description

Motivation and Context

Testing

Types of changes

Checklist

License

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashovlin Aug 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashovlin commented Aug 5, 2024 •

edited

Loading

ashovlin Aug 8, 2024 •

edited

Loading