-
Notifications
You must be signed in to change notification settings - Fork 285
Fix the Serialization/Deserialization issue with $ prefix columns #2944
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds support for escaping dollar sign ($) prefixes in column names during JSON serialization/deserialization. This is necessary because JSON property names starting with $ have special meaning in some contexts (like MongoDB's JSON query language) and can cause serialization issues.
- Adds escape/unescape logic to
DatabaseObjectConverterthat converts$to_$during serialization and back during deserialization - Updates
InitializeObjectsmethod to optionally generate column names with dollar sign prefixes - Adds three new test methods to validate serialization/deserialization of database objects with dollar-prefixed column names
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/Core/Services/MetadataProviders/Converters/DatabaseObjectConverter.cs | Implements escaping/unescaping logic for dollar sign prefixed column names in SourceDefinition properties |
| src/Service.Tests/UnitTests/SerializationDeserializationTests.cs | Adds test coverage for dollar sign handling and updates initialization method to support parameterized column name generation |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/Core/Services/MetadataProviders/Converters/DatabaseObjectConverter.cs
Outdated
Show resolved
Hide resolved
src/Core/Services/MetadataProviders/Converters/DatabaseObjectConverter.cs
Show resolved
Hide resolved
src/Core/Services/MetadataProviders/Converters/DatabaseObjectConverter.cs
Outdated
Show resolved
Hide resolved
| { | ||
| private const string TYPE_NAME = "TypeName"; | ||
| private const string DOLLAR_CHAR = "$"; | ||
| private const string ESCAPED_DOLLARCHAR = "_$"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we confidently escape it to "_$", may be there might be columns that are genuinely created with _$.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$ or _$ are allowed in some databases but used in different ways. for e.g., in PostgreSQL, the column should be used in within double-quotes. In MS SQL, its allowed but in box brackets I suppose. MySQL allows too. its better to handle these edge cases and also test it with different databases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should consider if this is an edge case we care about.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Alekhya-Polavarapu - can you add the escaped char based on the database type, and just use such an escaped char that already is used by the respective db itself. So, in case it is \ for MSSQL, use that. and distinguish usage of \ by escaping it with 2 backslashes \\
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prompt what are the valid characters in a database... | Try in Copilot Chat
what are the valid characters in a database column name in Microsoft SQL Server?
Response
In Microsoft SQL Server, the rules for valid column names are:
✅ Allowed Characters
- Letters:
A–Zanda–z - Digits:
0–9(but the name cannot start with a digit) - Underscore:
_ - At sign:
@(only for local variables, not recommended for column names) - Hash:
#(used for temporary tables, not typical for columns)
❌ Not Allowed
- Spaces (unless enclosed in delimiters)
- Special characters like
!,?,%,^,&,*,(,),+,=,/,\,|,~ - Control characters or non-printable characters
Length
- Maximum: 128 characters
Delimiters for Exceptions
If you need spaces or reserved keywords:
- Use square brackets:
[Column Name With Spaces]
- Or double quotes (if
QUOTED_IDENTIFIERis ON):"Column Name With Spaces"
Best Practices
- Start with a letter or underscore
- Avoid reserved keywords (e.g.,
SELECT,TABLE) - Stick to alphanumeric and underscore for portability
Do you want me to also include examples of valid and invalid column names for SQL Server? Or a quick T-SQL script to check if a name is valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For e.g. You can also use any of the not allowed characters as escape chars since that guarantees they wont be present in the column name.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| /// <summary> | ||
| /// Validates serialization and deserilization of Dictionary containing DatabaseView |
Copilot
AI
Nov 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Corrected spelling of 'deserilization' to 'deserialization'.
|
|
||
| Assert.AreEqual(deserializedDatabaseTable.SourceType, _databaseTable.SourceType); | ||
| Assert.AreEqual(deserializedDatabaseTable.FullName, _databaseTable.FullName); | ||
| deserializedDatabaseTable.Equals(_databaseTable); |
Copilot
AI
Nov 3, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The result of Equals() method call is not being asserted. This should be Assert.IsTrue(deserializedDatabaseTable.Equals(_databaseTable)) or similar to validate equality.
| [TestMethod] | ||
| public void TestDatabaseStoredProcedureSerializationDeserialization_WithDollarColumn() | ||
| { | ||
| InitializeObjects(true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: name the argument whose value is true here
| InitializeObjects(true); | |
| InitializeObjects( generateDollaredColumn: true); |
Why make this change?
Serialization and deserialization of metadata currently fail when column names are prefixed with the $ symbol.
Root cause
This issue occurs because we’ve enabled the ReferenceHandler flag in our System.Text.Json serialization settings. When this flag is active, the serializer treats $ as a reserved character used for special metadata (e.g., $id, $ref). As a result, any property name starting with $ is interpreted as metadata and cannot be deserialized properly.
What is this change?
This update introduces custom logic in the converter’s Write and Read methods to handle $-prefixed column names safely.
During serialization, columns beginning with $ are escaped as "_$".
During deserialization, this transformation is reversed to restore the original property names.
How was this tested