Skip to content

Commit dd95d9a

Browse files
CopilotJerryNixon
Copilot
and
JerryNixon
committed
Fix in-place mutation issue - restore original Columns after serialization
Addresses feedback from @JerryNixon: - Changed from in-place modification to temporary swap approach - Store original dictionary, replace with escaped version during serialization - Restore original in finally block to avoid mutating input object - Removed TryGetValue concern by using foreach iteration over dictionary - All existing tests pass, confirming backward compatibility Co-authored-by: JerryNixon <[email protected]>
1 parent 2e12d18 commit dd95d9a

1 file changed

Lines changed: 101 additions & 67 deletions

File tree

‎src/Core/Services/MetadataProviders/Converters/DatabaseObjectConverter.cs‎

Lines changed: 101 additions & 67 deletions
Original file line numberDiff line numberDiff line change
@@ -62,37 +62,60 @@ public override void Write(Utf8JsonWriter writer, DatabaseObject value, JsonSeri
6262
// "TypeName": "Azure.DataApiBuilder.Config.DatabasePrimitives.DatabaseTable, Azure.DataApiBuilder.Config",
6363
writer.WriteString(TYPE_NAME, GetTypeNameFromType(value.GetType()));
6464

65-
// Track SourceDefinition objects we've already escaped to avoid double-escaping
66-
// (SourceDefinition and TableDefinition/ViewDefinition/StoredProcedureDefinition can reference the same object)
67-
HashSet<SourceDefinition> escapedSourceDefs = new HashSet<SourceDefinition>();
65+
// Track SourceDefinition objects and their original Columns dictionaries
66+
// to restore them after serialization (avoiding permanent mutation)
67+
Dictionary<SourceDefinition, Dictionary<string, ColumnDefinition>> originalColumnsDictionaries = new();
6868

69-
// Add other properties of DatabaseObject
70-
foreach (PropertyInfo prop in value.GetType().GetProperties())
69+
try
7170
{
72-
// Skip the TypeName property, as it has been handled above
73-
if (prop.Name == TYPE_NAME)
71+
// Add other properties of DatabaseObject
72+
foreach (PropertyInfo prop in value.GetType().GetProperties())
7473
{
75-
continue;
76-
}
74+
// Skip the TypeName property, as it has been handled above
75+
if (prop.Name == TYPE_NAME)
76+
{
77+
continue;
78+
}
7779

78-
writer.WritePropertyName(prop.Name);
79-
object? propVal = prop.GetValue(value);
80+
writer.WritePropertyName(prop.Name);
81+
object? propVal = prop.GetValue(value);
8082

81-
// Only escape columns for properties whose type(derived type) is SourceDefinition.
82-
// Use HashSet to avoid double-escaping when multiple properties reference the same SourceDefinition object.
83-
if (IsSourceDefinitionOrDerivedClassProperty(prop) && propVal is SourceDefinition sourceDef)
84-
{
85-
if (!escapedSourceDefs.Contains(sourceDef))
83+
// Only escape columns for properties whose type(derived type) is SourceDefinition.
84+
// Temporarily replace dictionary contents with escaped version, then restore after serialization.
85+
if (IsSourceDefinitionOrDerivedClassProperty(prop) && propVal is SourceDefinition sourceDef)
8686
{
87-
EscapeDollaredColumns(sourceDef);
88-
escapedSourceDefs.Add(sourceDef);
87+
if (!originalColumnsDictionaries.ContainsKey(sourceDef))
88+
{
89+
// Store original dictionary contents
90+
originalColumnsDictionaries[sourceDef] = new Dictionary<string, ColumnDefinition>(sourceDef.Columns, sourceDef.Columns.Comparer);
91+
92+
// Replace dictionary contents with escaped version
93+
Dictionary<string, ColumnDefinition> escapedColumns = CreateEscapedColumnsDictionary(sourceDef.Columns);
94+
sourceDef.Columns.Clear();
95+
foreach (var kvp in escapedColumns)
96+
{
97+
sourceDef.Columns[kvp.Key] = kvp.Value;
98+
}
99+
}
89100
}
101+
102+
JsonSerializer.Serialize(writer, propVal, options);
90103
}
91104

92-
JsonSerializer.Serialize(writer, propVal, options);
105+
writer.WriteEndObject();
106+
}
107+
finally
108+
{
109+
// Always restore original Columns dictionary contents to avoid mutating the input object
110+
foreach (var kvp in originalColumnsDictionaries)
111+
{
112+
kvp.Key.Columns.Clear();
113+
foreach (var colKvp in kvp.Value)
114+
{
115+
kvp.Key.Columns[colKvp.Key] = colKvp.Value;
116+
}
117+
}
93118
}
94-
95-
writer.WriteEndObject();
96119
}
97120

98121
private static bool IsSourceDefinitionOrDerivedClassProperty(PropertyInfo prop)
@@ -102,52 +125,56 @@ private static bool IsSourceDefinitionOrDerivedClassProperty(PropertyInfo prop)
102125
}
103126

104127
/// <summary>
105-
/// Escapes column keys that start with '$' or 'DAB_ESCAPE$' for serialization.
128+
/// Creates a new dictionary with escaped column keys for serialization.
106129
/// Uses a double-encoding approach to handle edge cases:
107130
/// 1. First escapes columns starting with 'DAB_ESCAPE$' to 'DAB_ESCAPE$DAB_ESCAPE$...'
108131
/// 2. Then escapes columns starting with '$' to 'DAB_ESCAPE$...'
109132
/// This ensures that even if a column is named 'DAB_ESCAPE$xyz', it will be properly handled.
133+
/// Returns a new dictionary without modifying the original.
110134
/// </summary>
111-
private static void EscapeDollaredColumns(SourceDefinition sourceDef)
135+
private static Dictionary<string, ColumnDefinition> CreateEscapedColumnsDictionary(Dictionary<string, ColumnDefinition> originalColumns)
112136
{
113-
if (sourceDef.Columns is null || sourceDef.Columns.Count == 0)
137+
if (originalColumns is null || originalColumns.Count == 0)
114138
{
115-
return;
139+
return new Dictionary<string, ColumnDefinition>(StringComparer.InvariantCultureIgnoreCase);
116140
}
117141

142+
// Create a new dictionary with the same comparer as the original
143+
Dictionary<string, ColumnDefinition> escapedColumns = new(originalColumns.Comparer);
144+
118145
// Step 1: Escape columns that start with the escape sequence itself
119146
// This prevents collision when a column name already contains 'DAB_ESCAPE$'
120-
List<string> keysStartingWithEscapeSequence = sourceDef.Columns.Keys
121-
.Where(k => k.StartsWith(ESCAPED_DOLLARCHAR, StringComparison.Ordinal))
122-
.ToList();
123-
124-
foreach (string key in keysStartingWithEscapeSequence)
147+
foreach (var kvp in originalColumns)
125148
{
126-
ColumnDefinition col = sourceDef.Columns[key];
127-
sourceDef.Columns.Remove(key);
128-
string newKey = ESCAPED_DOLLARCHAR + key;
129-
sourceDef.Columns[newKey] = col;
130-
}
149+
string key = kvp.Key;
150+
ColumnDefinition value = kvp.Value;
131151

132-
// Step 2: Escape columns that start with '$'
133-
List<string> keysToEscape = sourceDef.Columns.Keys
134-
.Where(k => k.StartsWith(DOLLAR_CHAR, StringComparison.Ordinal))
135-
.ToList();
136-
137-
foreach (string key in keysToEscape)
138-
{
139-
ColumnDefinition col = sourceDef.Columns[key];
140-
sourceDef.Columns.Remove(key);
141-
string newKey = ESCAPED_DOLLARCHAR + key[1..];
142-
sourceDef.Columns[newKey] = col;
152+
if (key.StartsWith(ESCAPED_DOLLARCHAR, StringComparison.Ordinal))
153+
{
154+
// Double-escape: DAB_ESCAPE$FirstName -> DAB_ESCAPE$DAB_ESCAPE$FirstName
155+
escapedColumns[ESCAPED_DOLLARCHAR + key] = value;
156+
}
157+
else if (key.StartsWith(DOLLAR_CHAR, StringComparison.Ordinal))
158+
{
159+
// Escape dollar: $FirstName -> DAB_ESCAPE$FirstName
160+
escapedColumns[ESCAPED_DOLLARCHAR + key.Substring(1)] = value;
161+
}
162+
else
163+
{
164+
// No escaping needed
165+
escapedColumns[key] = value;
166+
}
143167
}
168+
169+
return escapedColumns;
144170
}
145171

146172
/// <summary>
147173
/// Unescapes column keys for deserialization using reverse double-encoding:
148174
/// 1. First unescapes columns starting with 'DAB_ESCAPE$DAB_ESCAPE$' to 'DAB_ESCAPE$...'
149175
/// 2. Then unescapes columns starting with 'DAB_ESCAPE$' to '$...'
150176
/// This ensures proper reconstruction of original column names even in edge cases.
177+
/// Modifies the dictionary in-place during deserialization.
151178
/// </summary>
152179
private static void UnescapeDollaredColumns(SourceDefinition sourceDef)
153180
{
@@ -156,32 +183,39 @@ private static void UnescapeDollaredColumns(SourceDefinition sourceDef)
156183
return;
157184
}
158185

159-
// Step 1: Unescape columns that were double-escaped (originally started with 'DAB_ESCAPE$')
160-
string doubleEscapeSequence = ESCAPED_DOLLARCHAR + ESCAPED_DOLLARCHAR;
161-
List<string> doubleEscapedKeys = sourceDef.Columns.Keys
162-
.Where(k => k.StartsWith(doubleEscapeSequence, StringComparison.Ordinal))
163-
.ToList();
186+
// Create a new dictionary with unescaped keys
187+
Dictionary<string, ColumnDefinition> unescapedColumns = new(sourceDef.Columns.Comparer);
164188

165-
foreach (string key in doubleEscapedKeys)
189+
foreach (var kvp in sourceDef.Columns)
166190
{
167-
ColumnDefinition col = sourceDef.Columns[key];
168-
sourceDef.Columns.Remove(key);
169-
// Remove the first 'DAB_ESCAPE$' prefix
170-
string newKey = key.Substring(ESCAPED_DOLLARCHAR.Length);
171-
sourceDef.Columns[newKey] = col;
172-
}
191+
string key = kvp.Key;
192+
ColumnDefinition value = kvp.Value;
173193

174-
// Step 2: Unescape columns that start with 'DAB_ESCAPE$' (originally started with '$')
175-
List<string> keysToUnescape = sourceDef.Columns.Keys
176-
.Where(k => k.StartsWith(ESCAPED_DOLLARCHAR, StringComparison.Ordinal))
177-
.ToList();
194+
// Step 1: Unescape columns that were double-escaped (originally started with 'DAB_ESCAPE$')
195+
string doubleEscapeSequence = ESCAPED_DOLLARCHAR + ESCAPED_DOLLARCHAR;
196+
if (key.StartsWith(doubleEscapeSequence, StringComparison.Ordinal))
197+
{
198+
// Remove the first 'DAB_ESCAPE$' prefix: DAB_ESCAPE$DAB_ESCAPE$FirstName -> DAB_ESCAPE$FirstName
199+
unescapedColumns[key.Substring(ESCAPED_DOLLARCHAR.Length)] = value;
200+
}
201+
// Step 2: Unescape columns that start with 'DAB_ESCAPE$' (originally started with '$')
202+
else if (key.StartsWith(ESCAPED_DOLLARCHAR, StringComparison.Ordinal))
203+
{
204+
// Add back the '$' prefix: DAB_ESCAPE$FirstName -> $FirstName
205+
unescapedColumns[DOLLAR_CHAR + key.Substring(ESCAPED_DOLLARCHAR.Length)] = value;
206+
}
207+
else
208+
{
209+
// No unescaping needed
210+
unescapedColumns[key] = value;
211+
}
212+
}
178213

179-
foreach (string key in keysToUnescape)
214+
// Replace the dictionary contents with the unescaped version
215+
sourceDef.Columns.Clear();
216+
foreach (var kvp in unescapedColumns)
180217
{
181-
ColumnDefinition col = sourceDef.Columns[key];
182-
sourceDef.Columns.Remove(key);
183-
string newKey = DOLLAR_CHAR + key[ESCAPED_DOLLARCHAR.Length..];
184-
sourceDef.Columns[newKey] = col;
218+
sourceDef.Columns[kvp.Key] = kvp.Value;
185219
}
186220
}
187221

0 commit comments

Comments
 (0)