Table name collision bug fix #1990
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
aaronburtle
requested review from
mbhaskar,
Aniruddh25,
JelteF,
seantleonard,
Mathos1432,
gledis69,
milismsft,
tarazou9,
ayush3797,
abhishekkumams,
aaronpowell,
severussundar and
ravishetye
as code owners
Aniruddh25
requested changes
Jan 26, 2024
src/Service.Tests/SqlTests/RestApiTests/Find/FindApiTestBase.cs
Outdated
Show resolved
Hide resolved
Aniruddh25
requested changes
Jan 31, 2024
…provider for each data source
There was a problem hiding this comment.
Few things to consider:
SourceDefinitionclass has a method calledFullNamewhich returnsschemaName.tableName. A new method can be introduced that returnsDBname.SchemaName.objectName.
Advantages with introducing this method
a) This would cover all database object types.
b) Logic of determining the full name would not get repeated at different places.
sourceDefinition.FullName() and sourceDefinition.FullNameIncludingDbName() (just example method name) can be used instead of determining the complete name multiple times.
3. This issue was reported for REST Get APIs. But, would be good to test this for other types like POST, PUT, etc.
4. This can be an opportunity to validate similar scenarios with GraphQL (This can be a separate issue/PR)
Aniruddh25
requested changes
Feb 8, 2024
severussundar
approved these changes
Feb 16, 2024
src/Service.Tests/SqlTests/RestApiTests/Find/FindApiTestBase.cs
Outdated
Show resolved
Hide resolved
seantleonard
approved these changes
Feb 16, 2024
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
Aniruddh25
requested changes
Feb 22, 2024
aaronburtle
requested review from
rohkhann,
neeraj-sharma2592 and
sourabh1007
as code owners
|
Looks good to merge once unit tests succeed in pipeline and merged with main. |
Aniruddh25
approved these changes
Feb 28, 2024
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 2 pipeline(s). |
seantleonard
pushed a commit
that referenced
this pull request
Mar 1, 2024
## Why make this change? Closes #1908 ## What is this change? We were previously filling the schema information for the entity data set but only using the table name. We add in the schema name to deconflict between two tables with the same name in different schemas. This means that we retrieve information from the entity data set by using schema and tablename when schema is available, and just the table name otherwise. The reason that we do not need to worry about collisions at the schema and table name level (eg. where we have the same schema and table names within different databases) is because this entity data set is a part of the SqlMetadataProvider, and we have a SqlMetadataProvider for each database (not database type but actual DBs). ## How was this tested? A regression test with a collision between table names in different schemas was added. We also update the unit test for SqlMetadatProvider. ## Sample Request(s) see #1908
seantleonard
added a commit
that referenced
this pull request
Mar 4, 2024
Cherry picks to `release/0.11` from `main` ## Why make this change? Closes #1908 ## What is this change? We were previously filling the schema information for the entity data set but only using the table name. We add in the schema name to deconflict between two tables with the same name in different schemas. This means that we retrieve information from the entity data set by using schema and tablename when schema is available, and just the table name otherwise. The reason that we do not need to worry about collisions at the schema and table name level (eg. where we have the same schema and table names within different databases) is because this entity data set is a part of the SqlMetadataProvider, and we have a SqlMetadataProvider for each database (not database type but actual DBs). ## How was this tested? A regression test with a collision between table names in different schemas was added. We also update the unit test for SqlMetadatProvider. Co-authored-by: aaronburtle <93220300+aaronburtle@users.noreply.github.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Why make this change?
Closes #1908
What is this change?
We were previously filling the schema information for the entity data set but only using the table name. We add in the schema name to deconflict between two tables with the same name in different schemas. This means that we retrieve information from the entity data set by using schema and tablename when schema is available, and just the table name otherwise.
The reason that we do not need to worry about collisions at the schema and table name level (eg. where we have the same schema and table names within different databases) is because this entity data set is a part of the SqlMetadataProvider, and we have a SqlMetadataProvider for each database (not database type but actual DBs).
How was this tested?
A regression test with a collision between table names in different schemas was added. We also update the unit test for SqlMetadatProvider.
Sample Request(s)
see #1908