Fix Serialization Side‑Effects by Using Cloned SourceDefinition Objects#3112
Fix Serialization Side‑Effects by Using Cloned SourceDefinition Objects#3112Alekhya-Polavarapu wants to merge 3 commits intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates DAB’s DatabaseObject JSON serialization to avoid mutating SourceDefinition (and derived types) when escaping $-prefixed column names, by cloning the definition before serialization.
Changes:
- Updated
DatabaseObjectConverterto escape$-prefixed column names by creating a clonedSourceDefinition/StoredProcedureDefinitionrather than mutating the original instance. - Strengthened serialization tests by turning previously non-asserting
Equals(...)calls into real assertions and by validating specificColumnDefinition/ParameterDefinitionfields.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/Service.Tests/UnitTests/SerializationDeserializationTests.cs | Updates assertions to actually validate equality/field round-trips for serialized database objects/definitions. |
| src/Core/Services/MetadataProviders/Converters/DatabaseObjectConverter.cs | Changes escaping flow to clone SourceDefinition-typed objects before serialization to prevent in-place mutation. |
Comments suppressed due to low confidence (1)
src/Core/Services/MetadataProviders/Converters/DatabaseObjectConverter.cs:85
DatabaseObject.SourceDefinitionis a computed, read-only property that mirrorsTableDefinition/ViewDefinition/StoredProcedureDefinition. InWrite(), escaping/cloning per-property means the same underlying definition will be cloned and serialized twice (e.g., bothTableDefinitionandSourceDefinition), which bloats the JSON and defeatsReferenceHandler.Preserveobject identity. Consider skipping serialization of theSourceDefinitionproperty (or caching the escaped clone per original instance during thisWrite()call) so each definition is serialized once.
// Add other properties of DatabaseObject
foreach (PropertyInfo prop in value.GetType().GetProperties())
{
// Skip the TypeName property, as it has been handled above
if (prop.Name == TYPE_NAME)
{
continue;
}
writer.WritePropertyName(prop.Name);
object? propVal = prop.GetValue(value);
// Only escape columns for properties whose type(derived type) is SourceDefinition.
if (IsSourceDefinitionOrDerivedClassProperty(prop) && propVal is SourceDefinition sourceDef)
{
// Check if we need to escape any column names
propVal = GetSourceDefinitionWithEscapedColumns(sourceDef);
}
JsonSerializer.Serialize(writer, propVal, options);
}
| Assert.AreEqual(expectedColumnDefinition.DbType, deserializedColumnDefinition.DbType); | ||
| Assert.AreEqual(expectedColumnDefinition.SqlDbType, deserializedColumnDefinition.SqlDbType); | ||
| Assert.AreEqual(expectedColumnDefinition.IsAutoGenerated, deserializedColumnDefinition.IsAutoGenerated); | ||
| Assert.AreEqual(expectedColumnDefinition.DefaultValue.ToString(), deserializedColumnDefinition.DefaultValue.ToString()); | ||
| } |
There was a problem hiding this comment.
VerifyColumnDefinitionSerializationDeserialization now compares DefaultValue.ToString(), which will throw if either DefaultValue is null. Since DefaultValue is nullable (object?), this assertion should be null-safe (and ideally validate null vs non-null explicitly) to avoid test crashes when a column has no default.
| private static void VerifyColumnDefinitionSerializationDeserialization(ColumnDefinition expectedColumnDefinition, ColumnDefinition deserializedColumnDefinition) | ||
| { | ||
| // test number of properties/fields defined in Column Definition | ||
| int fields = typeof(ColumnDefinition).GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance).Length; | ||
| Assert.AreEqual(fields, 8); | ||
|
|
||
| // test values | ||
| expectedColumnDefinition.Equals(deserializedColumnDefinition); | ||
| Assert.AreEqual(expectedColumnDefinition.DbType, deserializedColumnDefinition.DbType); | ||
| Assert.AreEqual(expectedColumnDefinition.SqlDbType, deserializedColumnDefinition.SqlDbType); | ||
| Assert.AreEqual(expectedColumnDefinition.IsAutoGenerated, deserializedColumnDefinition.IsAutoGenerated); | ||
| Assert.AreEqual(expectedColumnDefinition.DefaultValue.ToString(), deserializedColumnDefinition.DefaultValue.ToString()); | ||
| } |
There was a problem hiding this comment.
VerifyColumnDefinitionSerializationDeserialization stopped asserting several serialized fields (e.g., SystemType, HasDefault, IsNullable, IsReadOnly). Since this helper is meant to validate round-trip serialization, please add assertions for the remaining fields so regressions in serialization/deserialization are caught.
| Assert.AreEqual(expectedParameterDefinition.DbType, deserializedParameterDefinition.DbType); | ||
| Assert.AreEqual(expectedParameterDefinition.SqlDbType, deserializedParameterDefinition.SqlDbType); | ||
| Assert.AreEqual(expectedParameterDefinition.SystemType, deserializedParameterDefinition.SystemType); | ||
| Assert.AreEqual(expectedParameterDefinition.ConfigDefaultValue.ToString(), deserializedParameterDefinition.ConfigDefaultValue.ToString()); |
There was a problem hiding this comment.
VerifyParameterDefinitionSerializationDeserialization now compares ConfigDefaultValue.ToString(), which will throw if either ConfigDefaultValue is null. Since ConfigDefaultValue is nullable (object?), make this assertion null-safe (and consider asserting HasConfigDefault too) so the helper works for parameters without defaults.
| Assert.AreEqual(expectedParameterDefinition.ConfigDefaultValue.ToString(), deserializedParameterDefinition.ConfigDefaultValue.ToString()); | |
| Assert.AreEqual(expectedParameterDefinition.HasConfigDefault, deserializedParameterDefinition.HasConfigDefault); | |
| if (expectedParameterDefinition.HasConfigDefault) | |
| { | |
| Assert.AreEqual(expectedParameterDefinition.ConfigDefaultValue, deserializedParameterDefinition.ConfigDefaultValue); | |
| } |
| @@ -538,7 +541,10 @@ | |||
| int fields = typeof(ParameterDefinition).GetFields(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance).Length; | |||
| Assert.AreEqual(fields, 9); | |||
| // test values | |||
| expectedParameterDefinition.Equals(deserializedParameterDefinition); | |||
| Assert.AreEqual(expectedParameterDefinition.DbType, deserializedParameterDefinition.DbType); | |||
| Assert.AreEqual(expectedParameterDefinition.SqlDbType, deserializedParameterDefinition.SqlDbType); | |||
| Assert.AreEqual(expectedParameterDefinition.SystemType, deserializedParameterDefinition.SystemType); | |||
| Assert.AreEqual(expectedParameterDefinition.ConfigDefaultValue.ToString(), deserializedParameterDefinition.ConfigDefaultValue.ToString()); | |||
| } | |||
There was a problem hiding this comment.
VerifyParameterDefinitionSerializationDeserialization no longer asserts several fields on ParameterDefinition (e.g., Name, Required, HasConfigDefault, Default, Description). If the goal is to validate serialization round-trips, add assertions for the remaining properties so changes to parameter serialization are detected by tests.
souvikghosh04
left a comment
There was a problem hiding this comment.
It will be good to mention about the cloning approach- shallow copy/deep copy so people are aware.
| { | ||
| EscapeDollaredColumns(sourceDef); | ||
| // Check if we need to escape any column names | ||
| propVal = GetSourceDefinitionWithEscapedColumns(sourceDef); |
There was a problem hiding this comment.
looking at the cloning approach, it seems to be a shallow copy. this should be fine for now but for awareness should be added in notes/comments.
Aniruddh25
left a comment
There was a problem hiding this comment.
LGTM after resolving comments from Copilot
Why make this change?
When serializing Database Object details, the original object is being mutated during the process. While this hasn’t caused issues yet, modifying the source object in-place can introduce subtle bugs later if the same object is reused after serialization. Creating a safe, isolated copy avoids unintended side effects.
What is this change?
This update introduces cloning of the SourceDefinition object and its derived types before serialization.
The flow is as follows:
This approach may introduce some performance overhead, particularly when the sourceDef object is large. An alternative would be to use reflection; however, for derived types, such as ViewDefinition and StoredProcDefinition, the
Columnsproperty do not expose setters . As a result, reflection cannot provide a consistent or uniform cloning solution across all object types.How was this tested?