Skip to content
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

Fix for issue 16701. Brackets around tables names supports non-SQL Server providers #19837

Merged
merged 3 commits into from
Feb 10, 2020

Conversation

lajones
Copy link
Contributor

@lajones lajones commented Feb 8, 2020

Fixes issue #16701 . The tests work for SQL Server but in other providers the table names need square brackets (which are then re-interpreted as the provider's escape character).

@lajones lajones requested a review from roji February 8, 2020 00:33
@lajones lajones self-assigned this Feb 8, 2020
@lajones lajones changed the title Fix for issue 16701. Brackets around tables names supports not SQL Server providers Fix for issue 16701. Brackets around tables names supports non-SQL Server providers Feb 8, 2020
@lajones
Copy link
Contributor Author

lajones commented Feb 10, 2020

Ping @roji .

@roji
Copy link
Member

roji commented Feb 10, 2020

This fails, this time with PostgreSQL complaining about square brackets being present.

These tests use raw SQL (rather than LINQ), but stuff like delimiters vary across databases. For some other raw SQL tests (e.g. FromSqlQuery) we already have RelationalTestStore.NormalizeDelimetersInRawString, where the raw string contains square brackets, but this method replaces them with the provider's delimiters instead - this could be the fix here too.

@lajones
Copy link
Contributor Author

lajones commented Feb 10, 2020

@roji Got it. I've updated the code to use that. Thanks.

@roji
Copy link
Member

roji commented Feb 10, 2020

Looks pretty good! Can you please enclose Id in brackets as well? As a column identifier it has the same rules as the table identifier, after that everything should be green.

@lajones
Copy link
Contributor Author

lajones commented Feb 10, 2020

@roji Done. Also found some other usages of a table TheVoid which I updated for consistency.

Copy link
Member

@roji roji left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All green, thanks!

@lajones lajones merged commit 9477181 into master Feb 10, 2020
@lajones lajones deleted the 200207_Issue16701_01 branch February 10, 2020 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants