Modified capitalization in metadata queries (#1896) #1921

gamewizzzz · 2023-12-10T07:47:18Z

Why make this change?

Resolves [Bug]: Builder Not Working For MSSQL Databases With Case Sensitive Collation (Azure.DataApiBuilder.Core.Resolvers.QueryExecutor) #1896

What is this change?

These changes impact API builder startup. The query output has not been modified, but certain field references are capitalized.
The proper case is used to avoid "field not found" and "key field not found" errors when the database collation is case-sensitive.

How was this tested?

Integration Tests - Ran locally; about 5 tests failed, but it was the same ones that failed before modifications
Unit Tests

Sample Request(s)

N/A

Modified metadata queries to use proper case to avoid "field not found" errors whether database collation is sensitive or not

gamewizzzz · 2023-12-10T07:58:16Z

@microsoft-github-policy-service agree

severussundar · 2023-12-12T09:28:51Z

/azp run

azure-pipelines · 2023-12-12T09:29:12Z

Azure Pipelines successfully started running 5 pipeline(s).

seantleonard

thank you for making these changes to enable compat with case sensitive db collations! few things to update then should be good to go.

src/Core/Resolvers/DWSqlQueryBuilder.cs

src/Core/Resolvers/MsSqlQueryBuilder.cs

src/Core/Resolvers/DWSqlQueryBuilder.cs

Co-authored-by: Sean Leonard <sean.leonard@microsoft.com>

gamewizzzz · 2023-12-15T04:31:27Z

Code change suggestions reviewed and accepted. Thanks for the heads up on the tab characters.

abhishekkumams · 2023-12-15T09:27:58Z

/azp run

azure-pipelines · 2023-12-15T09:28:22Z

Azure Pipelines successfully started running 5 pipeline(s).

seantleonard

thanks for making these changes!

seantleonard · 2023-12-15T15:58:02Z

/azp run

azure-pipelines · 2023-12-15T15:58:23Z

Azure Pipelines successfully started running 5 pipeline(s).

Aniruddh25 · 2024-03-01T04:07:56Z

/azp run

azure-pipelines · 2024-03-01T04:08:09Z

Azure Pipelines successfully started running 2 pipeline(s).

Aniruddh25 · 2024-03-01T04:40:44Z

@gamewizzzz, the MySQL/PgSQL tests are failing since their query builders need to be updated as well.

Could you please fix it in this function:

data-api-builder/src/Core/Resolvers/MySqlQueryBuilder.cs

Line 357 in fa419cd

    
           public string BuildQueryToGetReadOnlyColumns(string schemaParamName, string tableParamName)

EHekkert · 2024-06-18T22:55:15Z

@gamewizzzz, are you still working on this?

gamewizzzz · 2024-06-19T03:02:55Z

No. Shortly after getting the test build to run successfully, one of the other branches changed something to the point I could not upgrade the tool properly on the test environment. Haven't had time to try again since.

…

On Tue, Jun 18, 2024 at 5:55 PM Erwin Hekkert ***@***.***> wrote: @gamewizzzz <https://github.com/gamewizzzz>, are you still working on this? — Reply to this email directly, view it on GitHub <#1921 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADRTLSCZNQQBRNHO3WE7YLDZIC3GVAVCNFSM6AAAAABAOMCOPSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNZXGIYTQNJWGU> . You are receiving this because you were mentioned.Message ID: ***@***.***>

sander1095 · 2024-06-19T05:43:08Z

Hi @Aniruddh25 . Any help from the DAB team to finish this PR?

Aniruddh25 · 2024-07-15T15:59:47Z

/azp run

Aniruddh25 · 2024-07-15T16:00:12Z

Hi @Aniruddh25 . Any help from the DAB team to finish this PR?

@sander1095 - yes, we will take it forward and merge this.

aaronburtle · 2024-07-19T22:38:46Z

/azp run

RubenCerna2079 · 2024-07-19T22:43:08Z

/azp run

RubenCerna2079 · 2024-07-23T22:56:03Z

/azp run

src/Core/Resolvers/PostgresQueryBuilder.cs

gamewizzzz · 2024-07-24T03:01:50Z

The example you reference in "image.png" shows double quotes, I'm not sure how that helps your argument. Looking at line 231 where the quotes are used, I would say the quotes are needed. In line 231, dynamic SQL syntax is being assigned to a string variable so it can be passed as an argument to another command that executes the SQL. That can't be done without at least one set of quotes since you have spaces in the statement. The most you could do is combine lines 231 and 232, but then again the code is broken out that way to help with readability. I agree with your point about the styling inconsistency of single quotes (') vs double quotes ("). The inconsistency costs extra time in explaining and transfering to other maintainers. However, there are legitimate reasons to use one over the other in MS SQL queries: - Variable expansion. Double quotes allow you to pre-expand variables before assigning the result to the string, whereas single quotes you cannot. You may be able to get away with wrapping line 231 in single quotes, but line 232 you cannot because schemaParamName is being expanded - Flexibility of SQL syntax. Double quotes still allow you to embed single quotes in the SQL query without much headache. In line 232, a string literal is being referenced in the filter of the dynamic SQL, and is wrapped in single quotes. If you tried to use single quotes to wrap the whole statement, you will have to escape the single quotes of the literal. Once you start having to do escaped characters, it gets a little more difficult to work with them when debugging errors

…

On Tue, Jul 23, 2024 at 6:21 PM aaronburtle ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In src/Core/Resolvers/PostgresQueryBuilder.cs <#1921 (comment)> : > @@ -228,7 +228,7 @@ public string BuildStoredProcedureResultDetailsQuery(string databaseObjectName) /// <inheritdoc/> public string BuildQueryToGetReadOnlyColumns(string schemaParamName, string tableParamName) { - string query = "SELECT attname AS column_name FROM pg_attribute " + + string query = "SELECT attname AS \"COLUMN_NAME\" FROM pg_attribute " + Are quotes needed here? The other statements don't seem to make use of double quotes, for example image.png (view on web) <https://github.com/user-attachments/assets/8e7364e6-9194-4e65-94c0-75f269a141cf> For style consistency would be better if we can remove these, or if quotes are needed see if single quote will work since we already use some single quotes in queries in this class. — Reply to this email directly, view it on GitHub <#1921 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADRTLSHGE6RBBDBV6KSLKTLZN3QOPAVCNFSM6AAAAABAOMCOPSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDCOJVGI2DQMJSGE> . You are receiving this because you were mentioned.Message ID: ***@***.***>

aaronburtle · 2024-07-24T22:29:19Z

The example you reference in "image.png" shows double quotes

I probably wasn't clear enough, but I am referencing the hard coded and escaped double quotes around COLUMN_NAME, typically if we are quoting parts of the sql statements we use a helper function, or leave the quotes out, or in some cases I think we have single quotes. Just want to align this with the rest of the code.

I should have mentioned the helper function but it slipped my mind and I was confused why we had escaped double quotes there.

…r function

RubenCerna2079 · 2024-07-24T23:12:36Z

/azp run

aaronburtle · 2024-07-24T23:27:43Z

/azp run

RubenCerna2079 · 2024-07-25T17:43:35Z

/azp run

aaronburtle

LGTM!

## Why make this change? - Resolves #1896 ## What is this change? - These changes impact API builder startup. The query output has not been modified, but certain field references are capitalized. - The proper case is used to avoid "field not found" and "key field not found" errors when the database collation is case-sensitive. ## How was this tested? - [x] Integration Tests - Ran locally; about 5 tests failed, but it was the same ones that failed before modifications - [ ] Unit Tests ## Sample Request(s) - N/A --------- Co-authored-by: Sean Leonard <sean.leonard@microsoft.com> Co-authored-by: Aniruddh Munde <anmunde@microsoft.com> Co-authored-by: RubenCerna2079 <32799214+RubenCerna2079@users.noreply.github.com> Co-authored-by: Ruben Cerna <rcernaserna@microsoft.com>

## Port metadata - Porting from `main` to `release/1.2` - Issue: #1896 - PR: #1921 ## Why make this change? - Resolves #1896 ## What is this change? - These changes impact API builder startup. The query output has not been modified, but certain field references are capitalized. - The proper case is used to avoid "field not found" and "key field not found" errors when the database collation is case-sensitive. ## How was this tested? - [x] Integration Tests - Ran locally; about 5 tests failed, but it was the same ones that failed before modifications - [ ] Unit Tests ## Sample Request(s) - N/A Co-authored-by: gamewizzzz <gamewizzzz@gmail.com> Co-authored-by: Aniruddh Munde <anmunde@microsoft.com> Co-authored-by: RubenCerna2079 <32799214+RubenCerna2079@users.noreply.github.com> Co-authored-by: Ruben Cerna <rcernaserna@microsoft.com>

Modified capitalization in metadata queries (Azure#1896)

2e988b9

Modified metadata queries to use proper case to avoid "field not found" errors whether database collation is sensitive or not

gamewizzzz requested review from mbhaskar, Aniruddh25, JelteF, seantleonard, Mathos1432, gledis69, aaronburtle, milismsft, tarazou9, ayush3797, abhishekkumams, aaronpowell, severussundar and ravishetye as code owners December 10, 2023 07:47

seantleonard reviewed Dec 12, 2023

View reviewed changes

gamewizzzz and others added 4 commits December 14, 2023 22:27

Update src/Core/Resolvers/DWSqlQueryBuilder.cs

fbc4d9c

Co-authored-by: Sean Leonard <sean.leonard@microsoft.com>

Update src/Core/Resolvers/MsSqlQueryBuilder.cs

8d030be

Co-authored-by: Sean Leonard <sean.leonard@microsoft.com>

Update src/Core/Resolvers/MsSqlQueryBuilder.cs

d13e7e0

Co-authored-by: Sean Leonard <sean.leonard@microsoft.com>

Update src/Core/Resolvers/DWSqlQueryBuilder.cs

6a99c21

Co-authored-by: Sean Leonard <sean.leonard@microsoft.com>

Merge branch 'main' into caseSensitiveCollationBug

679048b

seantleonard approved these changes Dec 15, 2023

View reviewed changes

gamewizzzz requested review from rohkhann, neeraj-sharma2592 and sourabh1007 as code owners March 1, 2024 04:07

Aniruddh25 approved these changes Mar 1, 2024

View reviewed changes

Merge branch 'main' into caseSensitiveCollationBug

29c2cb5

Merge branch 'main' into caseSensitiveCollationBug

e03865f

Updated the BuildQueryToGetReadOnlyColumn function for both programs

fd3eec1

aaronburtle reviewed Jul 23, 2024

View reviewed changes

src/Core/Resolvers/PostgresQueryBuilder.cs Outdated Show resolved Hide resolved

Changed function in PostgresQueryBuilder to use QuoteIdentifier helpe…

9430e08

…r function

aaronburtle approved these changes Jul 25, 2024

View reviewed changes

RubenCerna2079 merged commit 6ebabab into Azure:main Jul 25, 2024

seantleonard mentioned this pull request Aug 5, 2024

[Cherrypick] [Port] Modified capitalization in metadata queries #2315

Merged

2 tasks

Modified capitalization in metadata queries (#1896) #1921

Modified capitalization in metadata queries (#1896) #1921

Uh oh!

Conversation

gamewizzzz commented Dec 10, 2023

Why make this change?

What is this change?

How was this tested?

Sample Request(s)

Uh oh!

gamewizzzz commented Dec 10, 2023

Uh oh!

severussundar commented Dec 12, 2023

Uh oh!

azure-pipelines bot commented Dec 12, 2023

Uh oh!

seantleonard left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gamewizzzz commented Dec 15, 2023

Uh oh!

abhishekkumams commented Dec 15, 2023

Uh oh!

azure-pipelines bot commented Dec 15, 2023

Uh oh!

seantleonard left a comment

Choose a reason for hiding this comment

Uh oh!

seantleonard commented Dec 15, 2023

Uh oh!

azure-pipelines bot commented Dec 15, 2023

Uh oh!

Aniruddh25 commented Mar 1, 2024

Uh oh!

azure-pipelines bot commented Mar 1, 2024

Uh oh!

Aniruddh25 commented Mar 1, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

EHekkert commented Jun 18, 2024

Uh oh!

gamewizzzz commented Jun 19, 2024 via email

Uh oh!

sander1095 commented Jun 19, 2024

Uh oh!

Aniruddh25 commented Jul 15, 2024

Uh oh!

Aniruddh25 commented Jul 15, 2024

Uh oh!

aaronburtle commented Jul 19, 2024

Uh oh!

RubenCerna2079 commented Jul 19, 2024

Uh oh!

RubenCerna2079 commented Jul 23, 2024

Uh oh!

Uh oh!

gamewizzzz commented Jul 24, 2024 via email

Uh oh!

aaronburtle commented Jul 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

RubenCerna2079 commented Jul 24, 2024

Uh oh!

aaronburtle commented Jul 24, 2024

Uh oh!

RubenCerna2079 commented Jul 25, 2024

Uh oh!

aaronburtle left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Aniruddh25 commented Mar 1, 2024 •

edited

Loading

aaronburtle commented Jul 24, 2024 •

edited

Loading