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

Fetch correct parameter type and default value from stored procedure definition #989

Closed
abhishekkumams opened this issue Nov 21, 2022 · 2 comments
Assignees
Labels
config changes related to config engine issues that require change in engine code graphql mssql an issue thats specific to mssql
Milestone

Comments

@abhishekkumams
Copy link
Contributor

abhishekkumams commented Nov 21, 2022

    Why can't we look up the parameter type from stored procedure definition? By trying to parse into an int first, we are restricting the parameter to an int, even if it were actually a string. 

What if the parameter type is actually a string but the default value given is a number? - The runtime value may not necessarily be a number but a different string, - the graphql schema that we generate here will not allow such a use case...

E.g.

config:

"parameters": {
   "stringparam" : "23"
}

runtime:

teststoredproc ("stringparam": "this_wont_be_allowed") {
}

Why do we mandate default value for parameters to be specified in the config?
Why can't I only specify the name of the stored procedure in the config and have graphql use the default value specified in the stored proc definition to be used? Can't we infer default value of parameter from stored procedure definition?

Also test what happens if the stored procedure has parameters on the database but the config doesn't..

Originally posted by @Aniruddh25 in #979 (comment)

@seantleonard
Copy link
Contributor

This was at least partially addressed in #1383. Instead of resolving the parameter value type of the runtime configuration supplied parameter value, the engine now checks the stored procedure's database metadata to ensure that the runtime config supplied value can be parsed into the value type defined in the database.

What is not addressed in 1383 is the automatic resolving of a default value from database metadata instead of requiring a default value be defined in runtime config.

@seantleonard seantleonard added mssql an issue thats specific to mssql graphql engine issues that require change in engine code config changes related to config labels Apr 17, 2023
@seantleonard seantleonard added this to the July2023 milestone Apr 17, 2023
@Aniruddh25
Copy link
Contributor

With #1847 default value is no longer REQUIRED in runtime config, but we let the db provide the default when needed. We are unable to find the exact default value from stored proc definition, due to limitations described in #1847.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
config changes related to config engine issues that require change in engine code graphql mssql an issue thats specific to mssql
Projects
None yet
Development

No branches or pull requests

4 participants