-
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?
Changes from all commits
dd71cb5
8c73c88
74e4320
eec58f6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -276,8 +276,96 @@ public void TestDictionaryDatabaseObjectSerializationDeserialization() | |||||
| VerifySourceDefinitionSerializationDeserialization(deserializedDatabaseTable.TableDefinition, _databaseTable.TableDefinition, "FirstName"); | ||||||
| } | ||||||
|
|
||||||
| private void InitializeObjects() | ||||||
| /// <summary> | ||||||
| /// Validates serialization and deserilization of Dictionary containing DatabaseTable | ||||||
Alekhya-Polavarapu marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| /// The table will have dollar sign prefix ($) in the column name | ||||||
| /// this is how we serialize and deserialize metadataprovider.EntityToDatabaseObject dict. | ||||||
| /// </summary> | ||||||
| [TestMethod] | ||||||
| public void TestDictionaryDatabaseObjectSerializationDeserialization_WithDollarColumn() | ||||||
| { | ||||||
| InitializeObjects(true); | ||||||
|
|
||||||
| _options = new() | ||||||
| { | ||||||
| Converters = { | ||||||
| new DatabaseObjectConverter(), | ||||||
| new TypeConverter() | ||||||
| }, | ||||||
| ReferenceHandler = ReferenceHandler.Preserve, | ||||||
| }; | ||||||
|
|
||||||
| Dictionary<string, DatabaseObject> dict = new() { { "person", _databaseTable } }; | ||||||
|
|
||||||
| string serializedDict = JsonSerializer.Serialize(dict, _options); | ||||||
| Dictionary<string, DatabaseObject> deserializedDict = JsonSerializer.Deserialize<Dictionary<string, DatabaseObject>>(serializedDict, _options)!; | ||||||
|
|
||||||
| DatabaseTable deserializedDatabaseTable = (DatabaseTable)deserializedDict["person"]; | ||||||
|
|
||||||
| Assert.AreEqual(deserializedDatabaseTable.SourceType, _databaseTable.SourceType); | ||||||
| Assert.AreEqual(deserializedDatabaseTable.FullName, _databaseTable.FullName); | ||||||
| deserializedDatabaseTable.Equals(_databaseTable); | ||||||
Alekhya-Polavarapu marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| VerifySourceDefinitionSerializationDeserialization(deserializedDatabaseTable.SourceDefinition, _databaseTable.SourceDefinition, "$FirstName"); | ||||||
| VerifySourceDefinitionSerializationDeserialization(deserializedDatabaseTable.TableDefinition, _databaseTable.TableDefinition, "$FirstName"); | ||||||
| } | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Validates serialization and deserilization of Dictionary containing DatabaseView | ||||||
Alekhya-Polavarapu marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| /// The table will have dollar sign prefix ($) in the column name | ||||||
| /// this is how we serialize and deserialize metadataprovider.EntityToDatabaseObject dict. | ||||||
| /// </summary> | ||||||
| [TestMethod] | ||||||
| public void TestDatabaseViewSerializationDeserialization_WithDollarColumn() | ||||||
| { | ||||||
| InitializeObjects(true); | ||||||
|
|
||||||
| TestTypeNameChanges(_databaseView, "DatabaseView"); | ||||||
|
|
||||||
| // Test to catch if there is change in number of properties/fields | ||||||
| // Note: On Addition of property make sure it is added in following object creation _databaseView and include in serialization | ||||||
| // and deserialization test. | ||||||
| int fields = typeof(DatabaseView).GetProperties(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance).Length; | ||||||
| Assert.AreEqual(fields, 6); | ||||||
|
|
||||||
| string serializedDatabaseView = JsonSerializer.Serialize(_databaseView, _options); | ||||||
| DatabaseView deserializedDatabaseView = JsonSerializer.Deserialize<DatabaseView>(serializedDatabaseView, _options)!; | ||||||
|
|
||||||
| Assert.AreEqual(deserializedDatabaseView.SourceType, _databaseView.SourceType); | ||||||
| deserializedDatabaseView.Equals(_databaseView); | ||||||
Alekhya-Polavarapu marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| VerifySourceDefinitionSerializationDeserialization(deserializedDatabaseView.SourceDefinition, _databaseView.SourceDefinition, "$FirstName"); | ||||||
| VerifySourceDefinitionSerializationDeserialization(deserializedDatabaseView.ViewDefinition, _databaseView.ViewDefinition, "$FirstName"); | ||||||
| } | ||||||
|
|
||||||
| /// <summary> | ||||||
| /// Validates serialization and deserilization of Dictionary containing DatabaseStoredProcedure | ||||||
Alekhya-Polavarapu marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| /// The table will have dollar sign prefix ($) in the column name | ||||||
| /// this is how we serialize and deserialize metadataprovider.EntityToDatabaseObject dict. | ||||||
| /// </summary> | ||||||
| [TestMethod] | ||||||
| public void TestDatabaseStoredProcedureSerializationDeserialization_WithDollarColumn() | ||||||
| { | ||||||
| InitializeObjects(true); | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: name the argument whose value is
Suggested change
|
||||||
|
|
||||||
| TestTypeNameChanges(_databaseStoredProcedure, "DatabaseStoredProcedure"); | ||||||
|
|
||||||
| // Test to catch if there is change in number of properties/fields | ||||||
| // Note: On Addition of property make sure it is added in following object creation _databaseStoredProcedure and include in serialization | ||||||
| // and deserialization test. | ||||||
| int fields = typeof(DatabaseStoredProcedure).GetProperties(BindingFlags.Public | BindingFlags.NonPublic | BindingFlags.Instance).Length; | ||||||
| Assert.AreEqual(fields, 6); | ||||||
|
|
||||||
| string serializedDatabaseSP = JsonSerializer.Serialize(_databaseStoredProcedure, _options); | ||||||
| DatabaseStoredProcedure deserializedDatabaseSP = JsonSerializer.Deserialize<DatabaseStoredProcedure>(serializedDatabaseSP, _options)!; | ||||||
|
|
||||||
| Assert.AreEqual(deserializedDatabaseSP.SourceType, _databaseStoredProcedure.SourceType); | ||||||
| deserializedDatabaseSP.Equals(_databaseStoredProcedure); | ||||||
Alekhya-Polavarapu marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| VerifySourceDefinitionSerializationDeserialization(deserializedDatabaseSP.SourceDefinition, _databaseStoredProcedure.SourceDefinition, "$FirstName", true); | ||||||
| VerifySourceDefinitionSerializationDeserialization(deserializedDatabaseSP.StoredProcedureDefinition, _databaseStoredProcedure.StoredProcedureDefinition, "$FirstName", true); | ||||||
| } | ||||||
|
|
||||||
| private void InitializeObjects(bool generateDollaredColumn = false) | ||||||
| { | ||||||
| string columnName = generateDollaredColumn ? "$FirstName" : "FirstName"; | ||||||
| _options = new() | ||||||
| { | ||||||
| // ObjectConverter behavior different in .NET8 most likely due to | ||||||
|
|
@@ -289,10 +377,11 @@ private void InitializeObjects() | |||||
| new DatabaseObjectConverter(), | ||||||
| new TypeConverter() | ||||||
| } | ||||||
|
|
||||||
| }; | ||||||
|
|
||||||
| _columnDefinition = GetColumnDefinition(typeof(string), DbType.String, true, false, false, new string("John"), false); | ||||||
| _sourceDefinition = GetSourceDefinition(false, false, new List<string>() { "FirstName" }, _columnDefinition); | ||||||
| _sourceDefinition = GetSourceDefinition(false, false, new List<string>() { columnName }, _columnDefinition); | ||||||
|
|
||||||
| _databaseTable = new DatabaseTable() | ||||||
| { | ||||||
|
|
@@ -311,10 +400,10 @@ private void InitializeObjects() | |||||
| { | ||||||
| IsInsertDMLTriggerEnabled = false, | ||||||
| IsUpdateDMLTriggerEnabled = false, | ||||||
| PrimaryKey = new List<string>() { "FirstName" }, | ||||||
| PrimaryKey = new List<string>() { columnName }, | ||||||
| }, | ||||||
| }; | ||||||
| _databaseView.ViewDefinition.Columns.Add("FirstName", _columnDefinition); | ||||||
| _databaseView.ViewDefinition.Columns.Add(columnName, _columnDefinition); | ||||||
|
|
||||||
| _parameterDefinition = new() | ||||||
| { | ||||||
|
|
@@ -331,10 +420,10 @@ private void InitializeObjects() | |||||
| SourceType = EntitySourceType.StoredProcedure, | ||||||
| StoredProcedureDefinition = new() | ||||||
| { | ||||||
| PrimaryKey = new List<string>() { "FirstName" }, | ||||||
| PrimaryKey = new List<string>() { columnName }, | ||||||
| } | ||||||
| }; | ||||||
| _databaseStoredProcedure.StoredProcedureDefinition.Columns.Add("FirstName", _columnDefinition); | ||||||
| _databaseStoredProcedure.StoredProcedureDefinition.Columns.Add(columnName, _columnDefinition); | ||||||
| _databaseStoredProcedure.StoredProcedureDefinition.Parameters.Add("Id", _parameterDefinition); | ||||||
| } | ||||||
|
|
||||||
|
|
||||||
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
A–Zanda–z0–9(but the name cannot start with a digit)_@(only for local variables, not recommended for column names)#(used for temporary tables, not typical for columns)❌ Not Allowed
!,?,%,^,&,*,(,),+,=,/,\,|,~Length
Delimiters for Exceptions
If you need spaces or reserved keywords:
QUOTED_IDENTIFIERis ON):"Column Name With Spaces"Best Practices
SELECT,TABLE)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.