[Bug]: Optional stored procedure parameters generate the error "Missing required procedure parameters" #1748
Comments
|
this is related to this: #989. There is a validation error that is done on the runtimeconfig where we check that number of params on stored proc should match number in runtimeconfig, which is incorrect. We had reported this during our sync ups. I have the fix for this and will check it in soon. |
|
@rohkhann and @abhishekkumams can you check if #1847 does indeed resolve this issue too? if so, please link the pr item and close this. |
|
@rohkhann to confirm whether this is resolved. |
|
We're hitting this issue |
|
@simonsabin, which version of DAB are you using? |
|
Whatever you get with the service. We changed a proc and added optional parameters and the service died.
Simon Sabin
…________________________________
From: Sean Leonard ***@***.***>
Sent: Tuesday, March 26, 2024 4:50:57 PM
To: Azure/data-api-builder ***@***.***>
Cc: Simon Sabin ***@***.***>; Mention ***@***.***>
Subject: Re: [Azure/data-api-builder] [Bug]: Optional stored procedure parameters generate the error "Missing required procedure parameters" (Issue #1748)
@simonsabin<https://github.com/simonsabin>, which version of DAB are you using?
—
Reply to this email directly, view it on GitHub<#1748 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAJHM22LHX67QFWWZOQ6LLDY2GRPDAVCNFSM6AAAAAA5IE2UZWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRQHE3TCNBTGQ>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Now, if you have parameters in your stored proc, you dont need to duplicate those parameters on the config file. Thats what this fix handles: #1847. However, we still dont have the capability to detect an optional parameter if it is not specified in the call to the stored proc. |
|
Thats a massive problem as there is no way to deploy the change without breaking the system. Which ever is deployed first, DAB or DB, the result is DAB fails to run until both are deployed |
|
I have hit this issue today. What is a good work-around for optional parameters? |
|
Hitting a similar issue as well, with a "Missing required procedure parameters" during a call to a stored proc with an optional parameter (not declared in the dab config file) without passing this parameter for this call. |
|
I believe this should be removed Sure there is value at design time if the configuration hasn't been setup correctly to provide a nicer error, but you will still get an error if you don't specify all the required parameters. |
|
On my end, this is the Exception I'm hitting when trying to call a stored procedure without providing a value for a parameter for which the stored procedure has a default value: data-api-builder/src/Core/Services/RequestValidator.cs Lines 173 to 177 in 689c3e7 |
|
That as well then. I'd do a PR but think this needs a discussion on whether its by default or configurable. |
|
@seantleonard, @rohkhann, @abhishekkumams > may I ask you your thoughts on this one? |
|
@Benjiiim, agreed with your and @simonsabin's finding. Working on a fix that incorporate both of your experiences encountering errors. There isn't a straightforward method to determine whether a stored procedure has a default valued parameter and what that value is without parsing the object definition which is error prone. Instead, the change I'm making essentially defers error handling to the database. In this case, error 201:
SELECT message_id AS Error,
severity AS Severity,
[Event Logged] = CASE is_event_logged
WHEN 0 THEN 'No' ELSE 'Yes'
END,
[text] AS [Description]
FROM sys.messages
WHERE language_id = 1033
and message_id = 201Consequently, DAB will not fail startup or terminate a request when DAB doesn't detect an optional parameter in the request body when DAB runtime config doesn't define a default value for that optional parameter. What do you think? |
|
Sounds great.
I think there is a case that needs to be dealt with and that’s a parameter that’s defined in the config that’s not supplied and no default.
Ps would personally love to know how to code this change and the associated tests. Would you be interested in working on it with together ?
Simon Sabin
…________________________________
From: Sean Leonard ***@***.***>
Sent: Thursday, July 25, 2024 8:01:59 PM
To: Azure/data-api-builder ***@***.***>
Cc: Simon Sabin ***@***.***>; Mention ***@***.***>
Subject: Re: [Azure/data-api-builder] [Bug]: Optional stored procedure parameters generate the error "Missing required procedure parameters" (Issue #1748)
@Benjiiim<https://github.com/Benjiiim>, agreed with your and @simonsabin<https://github.com/simonsabin>'s finding. Working on a fix that incorporate both of your experiences encountering errors.
There isn't a straightforward method to determine whether a stored procedure has a default valued parameter and what that value is without parsing the object definition which is error prone.
* https://stackoverflow.com/questions/63581531/sql-statement-to-retrieve-procedure-optional-parameters-list
Instead, the change I'm making essentially defers error handling to the database. In this case, error 201:
"201", // Procedure or function '%.*ls' expects parameter '%.*ls', which was not supplied.
SELECT message_id AS Error,
severity AS Severity,
[Event Logged] = CASE is_event_logged
WHEN 0 THEN 'No' ELSE 'Yes'
END,
[text] AS [Description]
FROM sys.messages
WHERE language_id = 1033
and message_id = 201
Consequently, DAB will not fail startup or terminate a request when DAB doesn't detect an optional parameter in the request body when DAB runtime config doesn't define a default value for that optional parameter.
What do you think?
—
Reply to this email directly, view it on GitHub<#1748 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/AAJHM24ILU5CUEKRCEF57S3ZOFDSPAVCNFSM6AAAAAA5IE2UZWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJRGIYDONJSG4>.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
|
Hi Simon, I have a working branch here: https://github.com/Azure/data-api-builder/tree/dev/sean/sp_opt_1748 I'd be happy to help you work on your suggestion:
To clarify, when you say parameter defined in the config file, do you mean this? https://learn.microsoft.com/azure/data-api-builder/reference-configuration#parameters {
"entities": {
"<entity-name>": {
...
"type": "stored-procedure",
"parameters": {
"<parameter-name-1>" : "<default-value>",
"<parameter-name-2>" : "<default-value>",
"<parameter-name-3>" : "<default-value>"
}
}
}
}a value is required when defining params in the config. Perhaps you encountered a bug where an invalid, empty, or null value is provided for the parameter and DAB isn't handling properly? |
|
It’s just about what gets validated where and what’s controllable and how. |
|
I see. Do you have a few test cases in mind? That way I can better guide you were those can be added. |
… procedure params are excluded from config (#2346) ## Why make this change? - Closes #1748 - DAB should not crash during startup for the following scenario: 1. Database Stored procedure defined with 2 parameters (at least 1 mandatory param). DAB config defines default values for 1 of 2 of those parameters. Consequently, DAB fails config validation because the number of default parameters defined does not match the number of parameters in the database. However, this is not an issue when 0 config param defaults are defined. This leads to the next scenario: 2. DAB should not fail a request when config doesn't define default values for parameters and database does. Should let database handle the error because DAB has no way of resolving mandatory vs. optional stored procedure parameters. ## What is this change? 1. Adds new sub-status code `DatabaseInputError` that is used when MSSQL returns error code 201 with message: > Procedure or function '%.*ls' expects parameter '%.*ls', which was not supplied. 1. With this change, DAB **now returns** the following error to the client when DAB config doesn't define default values for mandatory stored procedure parameters: > Invalid request. Missing required request parameters. 1. The db returned error contains database object metadata which we have typically avoided returning to the client from DAB engine because the dab config file typically overrides any database object names. DAB will return 1. `DabGraphQLResultSerializer` class added which ensures that responses indicating 'missing procedure parameters' result in HTTP 400 for the GraphQL endpoint. REST endpoint returns 400 without needing the result serializer. ### Example requests Use the sample db schema and dab config from the linked issue. 1. GraphQL Endpoint ```graphql query ExecuteDummy { executeDummy { mandatoryParam optionalParam } } ``` Response HTTP 400 ```json { "errors": [ { "message": "Invalid request. Missing required request parameters.", "extensions": { "message": "Invalid request. Missing required request parameters.", "stackTrace": "<stack trace omitted for brevity>", "code": "DatabaseInputError" } } ] } ``` 3. REST endpoint ```https GET localhost:5000/api/Dummy?optionalParam=2345 ``` Response HTTP 400 ```json { "error": { "code": "DatabaseInputError", "message": "Invalid request. Missing required request parameters.", "status": 400 } } ``` ## How was this tested? updates stored procedure tests which check for missing required request parameters. - [x] Integration Tests - [x] Unit Tests
What happened?
I have a stored procedure like the following:
for which the
@optionalParamis, as the name implies, optional.If I configure DAB to use that stored procedure using the following configuration, omitting the
optionalParam(since it is optional):I get the following error during the startup:
Version
Microsoft.DataApiBuilder 0.8.52+c69925060e1942d28515b9c4b89ea24832da0c7c
What database are you using?
Azure SQL
What hosting model are you using?
Local (including CLI)
Which API approach are you accessing DAB through?
REST, GraphQL
Relevant log output
Code of Conduct
The text was updated successfully, but these errors were encountered: