Skip to content

Fix DynamoDB property converter precedence when a global converter is… #4323

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

Open
aanton-git wants to merge 7 commits into
base: development
Choose a base branch
from
+274 −70
Open

Fix DynamoDB property converter precedence when a global converter is… #4323

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
eda8441
Fix DynamoDB property converter precedence when a global converter is…
aanton-git Feb 3, 2026
d5b2968
Update unit test
aanton-git Feb 4, 2026
35554de
Update Validate method to throw when globlal conveter and property sp…
aanton-git Feb 9, 2026
a358737
Small change, update tests
aanton-git Feb 10, 2026
6c2358f
Update PropertyStorageTests to set DynamoDb context in ClassInitializ…
aanton-git Feb 11, 2026
8ba9c51
fix merge conflicts
aanton-git Feb 11, 2026
698cc7d
Update changeLogMessages from change log file
aanton-git Mar 12, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions generator/.DevConfigs/214e45e8-60ef-4e07-8f70-7fb4dc554186.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
{
"services": [
{
"serviceName": "DynamoDBv2",
"type": "patch",
"changeLogMessages": [
"Fix DynamoDB property converter precedence when a global converter is registered."
Copy link
Member

@normj normj Mar 12, 2026

Choose a reason for hiding this comment

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

There is the small possibility users possibly unintentionally depended on the current behavior. I think making the fix is the right choice but for transparency can you add [Breaking Change] as a prefix to the log message.

Copy link
Author

@aanton-git aanton-git Mar 12, 2026

Choose a reason for hiding this comment

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

Updated

]
}
]
}
17 changes: 11 additions & 6 deletions sdk/src/Services/DynamoDBv2/Custom/DataModel/InternalModel.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,19 +268,24 @@ public void Validate(DynamoDBContext context)

this.Converter = Utils.InstantiateConverter(ConverterType, context) as IPropertyConverter;
}
else
{
if (context.ConverterCache.TryGetValue(MemberType, out IPropertyConverter converter) && converter != null)
{
if (!Utils.CanInstantiateConverter(converter.GetType()) || !Utils.ImplementsInterface(converter.GetType(), typeof(IPropertyConverter)))
throw new InvalidOperationException("Converter for " + PropertyName + " must be instantiable with no parameters and must implement IPropertyConverter");

if (!PolymorphicProperty && !ShouldFlattenChildProperties && !StoreAsEpoch && !StoreAsEpochLong && !IsAutoGeneratedTimestamp)
this.Converter = converter;
}
}

if (StoreAsEpoch && StoreAsEpochLong)
throw new InvalidOperationException(PropertyName + " must not set both StoreAsEpoch and StoreAsEpochLong as true at the same time.");

if (IsAutoGeneratedTimestamp)
Utils.ValidateTimestampType(MemberType);

IPropertyConverter converter;
if (context.ConverterCache.TryGetValue(MemberType, out converter) && converter != null)
{
this.Converter = converter;
}

foreach (var index in Indexes)
IndexNames.AddRange(index.IndexNames);
}
Expand Down
142 changes: 129 additions & 13 deletions sdk/test/Services/DynamoDBv2/IntegrationTests/DataModelTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -661,21 +661,23 @@ public async Task TestContext_CustomDateTimeConverter(bool retrieveDateTimeInUtc

await Context.SaveAsync(employee);

// Since we are adding a custom DateTimeUtcConverter, the expected time will always be in the UTC time zone.
// regardless of RetrieveDateTimeInUtc value.
// Since we are adding a custom DateTimeUtcConverter, the expected time will be in the UTC time zone regardless of RetrieveDateTimeInUtc value.
// for the properties that does not have property specific attributes like [DynamoDBProperty(StoreAsEpochLong = true)].

var expectedCurrTime = currTime.ToUniversalTime();
var expectedLongEpochTime = longEpochTime.ToUniversalTime();
var expectedLongEpochTimeBefore1970 = longEpochTimeBefore1970.ToUniversalTime();
var expectedCurrTimeStoreAsEpoch = retrieveDateTimeInUtc ? currTime.ToUniversalTime() : currTime.ToLocalTime();
var expectedLongEpochTime = retrieveDateTimeInUtc ? longEpochTime.ToUniversalTime() : longEpochTime.ToLocalTime();
var expectedLongEpochTimeBefore1970 = retrieveDateTimeInUtc ? longEpochTimeBefore1970.ToUniversalTime() : longEpochTimeBefore1970.ToLocalTime();

// Load
var storedEmployee = Context.Load<AnnotatedNumericEpochEmployee>(employee.CreationTime, employee.Name);
Assert.IsNotNull(storedEmployee);
ApproximatelyEqual(expectedCurrTime, storedEmployee.CreationTime);
ApproximatelyEqual(expectedCurrTime, storedEmployee.EpochDate2);
ApproximatelyEqual(expectedCurrTimeStoreAsEpoch, storedEmployee.CreationTime);
ApproximatelyEqual(expectedCurrTimeStoreAsEpoch, storedEmployee.EpochDate2);
ApproximatelyEqual(expectedCurrTime, storedEmployee.NonEpochDate1);
ApproximatelyEqual(expectedCurrTime, storedEmployee.NonEpochDate2);
Assert.IsNull(storedEmployee.NullableEpochDate1);
ApproximatelyEqual(expectedCurrTime, storedEmployee.NullableEpochDate2.Value);
ApproximatelyEqual(expectedCurrTimeStoreAsEpoch, storedEmployee.NullableEpochDate2.Value);
ApproximatelyEqual(expectedLongEpochTime, storedEmployee.LongEpochDate1);
ApproximatelyEqual(expectedLongEpochTimeBefore1970, storedEmployee.LongEpochDate2);
ApproximatelyEqual(expectedLongEpochTime, storedEmployee.NullableLongEpochDate1.Value);
Expand All @@ -690,12 +692,12 @@ public async Task TestContext_CustomDateTimeConverter(bool retrieveDateTimeInUtc
new QueryOperationConfig { Filter = filter }
).GetNextSetAsync()).First();
Assert.IsNotNull(storedEmployee);
ApproximatelyEqual(expectedCurrTime, storedEmployee.CreationTime);
ApproximatelyEqual(expectedCurrTime, storedEmployee.EpochDate2);
ApproximatelyEqual(expectedCurrTimeStoreAsEpoch, storedEmployee.CreationTime);
ApproximatelyEqual(expectedCurrTimeStoreAsEpoch, storedEmployee.EpochDate2);
ApproximatelyEqual(expectedCurrTime, storedEmployee.NonEpochDate1);
ApproximatelyEqual(expectedCurrTime, storedEmployee.NonEpochDate2);
Assert.IsNull(storedEmployee.NullableEpochDate1);
ApproximatelyEqual(expectedCurrTime, storedEmployee.NullableEpochDate2.Value);
ApproximatelyEqual(expectedCurrTimeStoreAsEpoch, storedEmployee.NullableEpochDate2.Value);
ApproximatelyEqual(expectedLongEpochTime, storedEmployee.LongEpochDate1);
ApproximatelyEqual(expectedLongEpochTimeBefore1970, storedEmployee.LongEpochDate2);
ApproximatelyEqual(expectedLongEpochTime, storedEmployee.NullableLongEpochDate1.Value);
Expand All @@ -706,12 +708,12 @@ public async Task TestContext_CustomDateTimeConverter(bool retrieveDateTimeInUtc
// Scan
storedEmployee = (await Context.ScanAsync<AnnotatedNumericEpochEmployee>(new List<ScanCondition>()).GetRemainingAsync()).First();
Assert.IsNotNull(storedEmployee);
ApproximatelyEqual(expectedCurrTime, storedEmployee.CreationTime);
ApproximatelyEqual(expectedCurrTime, storedEmployee.EpochDate2);
ApproximatelyEqual(expectedCurrTimeStoreAsEpoch, storedEmployee.CreationTime);
ApproximatelyEqual(expectedCurrTimeStoreAsEpoch, storedEmployee.EpochDate2);
ApproximatelyEqual(expectedCurrTime, storedEmployee.NonEpochDate1);
ApproximatelyEqual(expectedCurrTime, storedEmployee.NonEpochDate2);
Assert.IsNull(storedEmployee.NullableEpochDate1);
ApproximatelyEqual(expectedCurrTime, storedEmployee.NullableEpochDate2.Value);
ApproximatelyEqual(expectedCurrTimeStoreAsEpoch, storedEmployee.NullableEpochDate2.Value);
ApproximatelyEqual(expectedLongEpochTime, storedEmployee.LongEpochDate1);
ApproximatelyEqual(expectedLongEpochTimeBefore1970, storedEmployee.LongEpochDate2);
ApproximatelyEqual(expectedLongEpochTime, storedEmployee.NullableLongEpochDate1.Value);
Expand Down Expand Up @@ -2327,6 +2329,48 @@ public async Task Test_AutoGeneratedTimestampAttribute_With_Annotations_BatchWri
Assert.IsTrue((DateTime.UtcNow - loaded.LastUpdated.Value).TotalMinutes < 1, "LastUpdated should be recent");
}

/// <summary>
/// Tests that if a custom property converter is used over the global converter on that specific property.
/// </summary>
/// <param name="retrieveDateTimeInUtc"></param>
[TestMethod]
[TestCategory("DynamoDBv2")]
public async Task TestContext_GlobalConverter_And_PropertyConvector()
{
TableCache.Clear();
await CleanupTables();
TableCache.Clear();

// Add a global converter
Context.ConverterCache.Add(typeof(NameType), new NameTypeUpperCaseConverter<NameType>());

var nameType = new NameType
{
FirstName = "John",
MiddleName = "Bob",
LastName = "Smith"
};
var employee = new PropertyConvertorTable
{
Id = 1,
PropertyWithGlobalConvertor = nameType,
PropertyWithPropertyConvertor = nameType,

};
await Context.SaveAsync(employee);

// Load
var storedEmployee = Context.Load<PropertyConvertorTable>(employee.Id);
Assert.IsNotNull(storedEmployee);
Assert.AreEqual(storedEmployee.Id, employee.Id);
Assert.AreEqual(storedEmployee.PropertyWithGlobalConvertor.FirstName, "JOHN");
Assert.AreEqual(storedEmployee.PropertyWithGlobalConvertor.MiddleName, "BOB");
Assert.AreEqual(storedEmployee.PropertyWithGlobalConvertor.LastName, "SMITH");
Assert.AreEqual(storedEmployee.PropertyWithPropertyConvertor.FirstName, "john");
Assert.AreEqual(storedEmployee.PropertyWithPropertyConvertor.MiddleName, "bob");
Assert.AreEqual(storedEmployee.PropertyWithPropertyConvertor.LastName, "smith");
}

private static async Task TestEmptyStringsWithFeatureEnabled()
{
var product = new Product
Expand Down Expand Up @@ -4569,6 +4613,28 @@ public class AnnotatedRangeTable2 : AnnotatedRangeTable
internal int NotAnnotatedAttribute { get; set; }
}

/// <summary>
/// Table with a property converter.
/// </summary>
[DynamoDBTable("HashTable")]
public class PropertyConvertorTable
{
[DynamoDBHashKey]
public int Id { get; set; }

public NameType PropertyWithGlobalConvertor { get; set; }

[DynamoDBProperty(Converter = typeof(NameTypeLowerCaseConverter<NameType>))]
public NameType PropertyWithPropertyConvertor { get; set; }
}

public class NameType
{
public string FirstName { get; set; }
public string MiddleName { get; set; }
public string LastName { get; set; }
}

public class DateTimeUtcConverter : IPropertyConverter
{
public DynamoDBEntry ToEntry(object value) => (DateTime)value;
Expand All @@ -4593,6 +4659,56 @@ public object FromEntry(DynamoDBEntry entry)
}
}

public class NameTypeLowerCaseConverter<T> : IPropertyConverter where T : NameType
{
public DynamoDBEntry ToEntry(object value)
{
var nameType = (NameType)value;
return new Document(new Dictionary<string, DynamoDBEntry>()
{
{"FirstName",nameType.FirstName.ToLower()},
{"MiddleName",nameType.MiddleName.ToLower()},
{"LastName",nameType.LastName.ToLower()}
});
}

public object FromEntry(DynamoDBEntry entry)
{
var document = entry as Document;
return new NameType
{
FirstName = document["FirstName"].AsString().ToLower(),
MiddleName = document["MiddleName"].AsString().ToLower(),
LastName = document["LastName"].AsString().ToLower(),
};
}
}

public class NameTypeUpperCaseConverter<T> : IPropertyConverter where T : NameType
{
public DynamoDBEntry ToEntry(object value)
{
var nameType = (NameType)value;
return new Document(new Dictionary<string, DynamoDBEntry>()
{
{"FirstName",nameType.FirstName.ToUpper()},
{"MiddleName",nameType.MiddleName.ToUpper()},
{"LastName",nameType.LastName.ToUpper()}
});
}

public object FromEntry(DynamoDBEntry entry)
{
var document = entry as Document;
return new NameType
{
FirstName = document["FirstName"].AsString().ToUpper(),
MiddleName = document["MiddleName"].AsString().ToUpper(),
LastName = document["LastName"].AsString().ToUpper(),
};
}
}

#region Flatten

/// <summary>
Expand Down
Loading