search

Azure / data-api-builder

Data API builder provides modern REST and GraphQL endpoints to your Azure Databases and on-prem stores.
https://aka.ms/dab/docs
MIT License
899 stars 180 forks source link

[Bug]: Optional stored procedure parameters generate the error "Missing required procedure parameters" #1748

Closed yorek closed 1 week ago

yorek commented 1 year ago

What happened?

I have a stored procedure like the following:

create procedure dbo.stp_Dummy
@mandatoryParam int,
@optionalParam int = 10
as
select 
    @mandatoryParam as mandatoryParam,
    @optionalParam as optionalParam
go

for which the @optionalParam is, 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):

"Dummy": {
    "source": {
      "type": "stored-procedure",
      "object": "dbo.stp_Dummy",
      "parameters": {
        "mandatoryParam": 10
      }
    },
    "graphql": {
      "operation": "query"
    },
    "permissions": [
      {
        "role": "anonymous",
        "actions": [
          "execute"
        ]
      }
    ]
  }

I get the following error during the startup:

 Azure.DataApiBuilder.Service.Exceptions.DataApiBuilderException: Invalid request. Missing required procedure parameters: optionalParam for entity: Dummy

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

Information: Microsoft.DataApiBuilder 0.8.52+c69925060e1942d28515b9c4b89ea24832da0c7c
Information: Config not provided. Trying to get default config based on DAB_ENVIRONMENT...
Information: Environment variable DAB_ENVIRONMENT is (null)
Loading config file from dab-config.json.
Information: Loaded config file: dab-config.json
Information: Setting default minimum LogLevel: Debug for Development mode.
Starting the runtime engine...
Loading config file from dab-config.json.
info: Microsoft.AspNetCore.DataProtection.KeyManagement.XmlKeyManager[63]
      User profile is available. Using 'C:\Users\damauri\AppData\Local\ASP.NET\DataProtection-Keys' as key repository and Windows DPAPI to encrypt keys at rest.
info: Azure.DataApiBuilder.Core.Services.ISqlMetadataProvider[0]
      Dummy path: /api/Dummy
dbug: Azure.DataApiBuilder.Core.Resolvers.IQueryExecutor[0]
      Executing query:
SELECT name as result_field_name, TYPE_NAME(system_type_id) as result_type, is_nullable FROM sys.dm_exec_describe_first_result_set_for_object (OBJECT_ID('dbo.stp_Dummy'), 0) WHERE is_hidden is not NULL AND is_hidden = 0
dbug: Azure.DataApiBuilder.Core.Services.ISqlMetadataProvider[0]
      Logging primary key information for entity: Dummy.
info: Azure.DataApiBuilder.Core.Configurations.RuntimeConfigValidator[0]
      Validating Relationship Section in Config...
fail: Azure.DataApiBuilder.Service.Startup[0]
      Unable to complete runtime initialization. Refer to exception for error details.
      Azure.DataApiBuilder.Service.Exceptions.DataApiBuilderException: Invalid request. Missing required procedure parameters: optionalParam for entity: Dummy
         at Azure.DataApiBuilder.Core.Configurations.RuntimeConfigValidator.ValidateStoredProceduresInConfig(RuntimeConfig runtimeConfig, ISqlMetadataProvider sqlMetadataProvider)
         at Azure.DataApiBuilder.Service.Startup.PerformOnConfigChangeAsync(IApplicationBuilder app)
fail: Azure.DataApiBuilder.Service.Startup[0]
      Exiting the runtime engine...
crit: Microsoft.AspNetCore.Hosting.Diagnostics[6]
      Application startup exception
      System.ApplicationException: Could not initialize the engine with the runtime config file: dab-config.json
         at Azure.DataApiBuilder.Service.Startup.Configure(IApplicationBuilder app, IWebHostEnvironment env, RuntimeConfigProvider runtimeConfigProvider)
         at System.RuntimeMethodHandle.InvokeMethod(Object target, Span`1& arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
         at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
         at Microsoft.AspNetCore.Hosting.ConfigureBuilder.Invoke(Object instance, IApplicationBuilder builder)
         at Microsoft.AspNetCore.Hosting.ConfigureBuilder.<>c__DisplayClass4_0.<Build>b__0(IApplicationBuilder builder)
         at Microsoft.AspNetCore.Hosting.GenericWebHostBuilder.<>c__DisplayClass15_0.<UseStartup>b__1(IApplicationBuilder app)  
         at Microsoft.AspNetCore.Mvc.Filters.MiddlewareFilterBuilderStartupFilter.<>c__DisplayClass0_0.<Configure>g__MiddlewareFilterBuilder|0(IApplicationBuilder builder)
         at Microsoft.AspNetCore.HostFilteringStartupFilter.<>c__DisplayClass0_0.<Configure>b__0(IApplicationBuilder app)       
         at Microsoft.AspNetCore.Hosting.GenericWebHostService.StartAsync(CancellationToken cancellationToken)
Unable to launch the runtime due to: System.ApplicationException: Could not initialize the engine with the runtime config file: dab-config.json
   at Azure.DataApiBuilder.Service.Startup.Configure(IApplicationBuilder app, IWebHostEnvironment env, RuntimeConfigProvider runtimeConfigProvider)
   at System.RuntimeMethodHandle.InvokeMethod(Object target, Span`1& arguments, Signature sig, Boolean constructor, Boolean wrapExceptions)
   at System.Reflection.RuntimeMethodInfo.Invoke(Object obj, BindingFlags invokeAttr, Binder binder, Object[] parameters, CultureInfo culture)
   at Microsoft.AspNetCore.Hosting.ConfigureBuilder.Invoke(Object instance, IApplicationBuilder builder)
   at Microsoft.AspNetCore.Hosting.ConfigureBuilder.<>c__DisplayClass4_0.<Build>b__0(IApplicationBuilder builder)
   at Microsoft.AspNetCore.Hosting.GenericWebHostBuilder.<>c__DisplayClass15_0.<UseStartup>b__1(IApplicationBuilder app)        
   at Microsoft.AspNetCore.Mvc.Filters.MiddlewareFilterBuilderStartupFilter.<>c__DisplayClass0_0.<Configure>g__MiddlewareFilterBuilder|0(IApplicationBuilder builder)
   at Microsoft.AspNetCore.HostFilteringStartupFilter.<>c__DisplayClass0_0.<Configure>b__0(IApplicationBuilder app)
   at Microsoft.AspNetCore.Hosting.GenericWebHostService.StartAsync(CancellationToken cancellationToken)
   at Microsoft.Extensions.Hosting.Internal.Host.StartAsync(CancellationToken cancellationToken)
   at Microsoft.Extensions.Hosting.HostingAbstractionsHostExtensions.RunAsync(IHost host, CancellationToken token)
   at Microsoft.Extensions.Hosting.HostingAbstractionsHostExtensions.RunAsync(IHost host, CancellationToken token)
   at Microsoft.Extensions.Hosting.HostingAbstractionsHostExtensions.Run(IHost host)
   at Azure.DataApiBuilder.Service.Program.StartEngine(String[] args)
Error: Failed to start the engine.

Code of Conduct

rohkhann commented 1 year ago

this is related to this: https://github.com/Azure/data-api-builder/issues/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.

seantleonard commented 8 months ago

@rohkhann and @abhishekkumams can you check if #1847 does indeed resolve this issue too? if so, please link the pr item and close this.

seantleonard commented 7 months ago

@rohkhann to confirm whether this is resolved.

simonsabin commented 6 months ago

We're hitting this issue

seantleonard commented 6 months ago

@simonsabin, which version of DAB are you using?

simonsabin commented 6 months ago

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)

@simonsabinhttps://github.com/simonsabin, which version of DAB are you using?

— Reply to this email directly, view it on GitHubhttps://github.com/Azure/data-api-builder/issues/1748#issuecomment-2020971434, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAJHM22LHX67QFWWZOQ6LLDY2GRPDAVCNFSM6AAAAAA5IE2UZWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDAMRQHE3TCNBTGQ. You are receiving this because you were mentioned.Message ID: @.***>

rohkhann commented 6 months ago

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: https://github.com/Azure/data-api-builder/pull/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.

simonsabin commented 6 months ago

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

BenMenegazzo commented 3 months ago

I have hit this issue today. What is a good work-around for optional parameters?

Benjiiim commented 3 months ago

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. Please see #2245

simonsabin commented 2 months ago

I believe this should be removed https://github.com/Azure/data-api-builder/blob/689c3e7affe5b94d3ea5d7452c49235068e42652/src/Core/Resolvers/Sql%20Query%20Structures/SqlExecuteQueryStructure.cs#L71 or some configuration to remove it. 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.

Benjiiim commented 2 months ago

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:

https://github.com/Azure/data-api-builder/blob/689c3e7affe5b94d3ea5d7452c49235068e42652/src/Core/Services/RequestValidator.cs#L173-L177

simonsabin commented 2 months ago

That as well then. I'd do a PR but think this needs a discussion on whether its by default or configurable.

Benjiiim commented 2 months ago

@seantleonard, @rohkhann, @abhishekkumams > may I ask you your thoughts on this one?

seantleonard commented 2 months ago

@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:

"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?

simonsabin commented 2 months ago

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)

@Benjiiimhttps://github.com/Benjiiim, agreed with your and @simonsabinhttps://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.

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 GitHubhttps://github.com/Azure/data-api-builder/issues/1748#issuecomment-2251207527, or unsubscribehttps://github.com/notifications/unsubscribe-auth/AAJHM24ILU5CUEKRCEF57S3ZOFDSPAVCNFSM6AAAAAA5IE2UZWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDENJRGIYDONJSG4. You are receiving this because you were mentioned.Message ID: @.***>

seantleonard commented 2 months ago

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:

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.

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?

simonsabin commented 2 months ago

It’s just about what gets validated where and what’s controllable and how. Thinking about it, what is validating data types, default values. I as was pointed out there were a number of places in the code base this touched and on my investigation some didn’t have test cases thus the desire to understand how we put tests in place for this stuffs

seantleonard commented 2 months ago

I see. Do you have a few test cases in mind? That way I can better guide you were those can be added.