data-api-builder icon indicating copy to clipboard operation
data-api-builder copied to clipboard

Make parameter length based on underlying column length except for complex ops

Open simonsabin opened this issue 10 months ago • 26 comments

Why make this change?

Fixes issue #2626

What is this change?

Where a parameter is linked to an underlying column in the data model the length is stored When the query is executed the length is used to ensure parameters have consistent lengths

For certain operations the value is padded with escape characters or %s, these need to be allowed for. In those cases the length is set to -1 (max) in order that the extra characters are allowed for AND consistent lengths are maintained

How was this tested?

  • [x] Integration Tests

Checking the resultant query is using the right length was done using trace. Not sure with the architecture whether it would be possible to mock at that level.

Sample Request(s)

See new tests TestFilterParamForStringFilterWorkWithComplexOp and TestFilterParamForStringFilterWorkWithNotContains

simonsabin avatar Mar 20 '25 19:03 simonsabin

It’s to cover the edge case of matching with the LiKE that you get with contains and starts with, and also the escaping of values that occurs in the predicates. If you don’t you can end up with false positives and false negatives

Simon Sabin


From: aaronburtle @.> Sent: Thursday, March 20, 2025 9:19:26 PM To: Azure/data-api-builder @.> Cc: Simon Sabin @.>; Author @.> Subject: Re: [Azure/data-api-builder] Make parameter length based on underlying column length except for complex ops (PR #2627)

@aaronburtle commented on this pull request.


In src/Service.Tests/DatabaseSchema-MsSql.sqlhttps://github.com/Azure/data-api-builder/pull/2627#discussion_r2006469243:

@@ -514,7 +514,7 @@ SET IDENTITY_INSERT books ON INSERT INTO books(id, title, publisher_id) VALUES (1, 'Awesome book', 1234), (2, 'Also Awesome book', 1234), -(3, 'Great wall of china explained', 2345), +(3, 'Great wall of china explained]', 2345),

I am curious, why the extra "]"?

— Reply to this email directly, view it on GitHubhttps://github.com/Azure/data-api-builder/pull/2627#pullrequestreview-2704137237, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAJHM2Z7OZVBM5HTNT3F6SD2VMWF5AVCNFSM6AAAAABZOI6YHOVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDOMBUGEZTOMRTG4. You are receiving this because you authored the thread.Message ID: @.***>

simonsabin avatar Mar 20 '25 21:03 simonsabin

applied suggested changes

simonsabin avatar Mar 20 '25 21:03 simonsabin

/azp run

aaronburtle avatar Mar 20 '25 23:03 aaronburtle

Azure Pipelines successfully started running 6 pipeline(s).

azure-pipelines[bot] avatar Mar 20 '25 23:03 azure-pipelines[bot]

Pipeline failures are just whitespace format.

aaronburtle avatar Mar 21 '25 05:03 aaronburtle

/azp run

Aniruddh25 avatar Mar 24 '25 03:03 Aniruddh25

Azure Pipelines successfully started running 6 pipeline(s).

azure-pipelines[bot] avatar Mar 24 '25 03:03 azure-pipelines[bot]

/azp run

Aniruddh25 avatar Mar 24 '25 03:03 Aniruddh25

Azure Pipelines successfully started running 6 pipeline(s).

azure-pipelines[bot] avatar Mar 24 '25 03:03 azure-pipelines[bot]

/azp run

Aniruddh25 avatar Mar 25 '25 00:03 Aniruddh25

Azure Pipelines successfully started running 6 pipeline(s).

azure-pipelines[bot] avatar Mar 25 '25 00:03 azure-pipelines[bot]

/azp run

Aniruddh25 avatar Mar 28 '25 23:03 Aniruddh25

Azure Pipelines successfully started running 6 pipeline(s).

azure-pipelines[bot] avatar Mar 28 '25 23:03 azure-pipelines[bot]

/azp run

Aniruddh25 avatar Mar 28 '25 23:03 Aniruddh25

Azure Pipelines successfully started running 6 pipeline(s).

azure-pipelines[bot] avatar Mar 28 '25 23:03 azure-pipelines[bot]

/azp run

aaronburtle avatar Mar 31 '25 14:03 aaronburtle

Azure Pipelines successfully started running 6 pipeline(s).

azure-pipelines[bot] avatar Mar 31 '25 14:03 azure-pipelines[bot]

/azp run

Aniruddh25 avatar Apr 01 '25 19:04 Aniruddh25

Azure Pipelines successfully started running 6 pipeline(s).

azure-pipelines[bot] avatar Apr 01 '25 19:04 azure-pipelines[bot]

whats the state of this?

simonsabin avatar Apr 10 '25 15:04 simonsabin

@Aniruddh25 @aaronburtle whats needed to get this merged in?

simonsabin avatar Apr 18 '25 14:04 simonsabin

Running the tests to ensure they pass. Will merge this soon.

Aniruddh25 avatar Apr 23 '25 19:04 Aniruddh25

/azp run

Aniruddh25 avatar Apr 24 '25 00:04 Aniruddh25

Azure Pipelines successfully started running 6 pipeline(s).

azure-pipelines[bot] avatar Apr 24 '25 00:04 azure-pipelines[bot]

/azp run

Aniruddh25 avatar May 07 '25 01:05 Aniruddh25

Azure Pipelines successfully started running 6 pipeline(s).

azure-pipelines[bot] avatar May 07 '25 01:05 azure-pipelines[bot]