Skip to content

Config option to allow custom parameter syntax #78

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

Merged
merged 16 commits into from
Apr 30, 2024

Conversation

not-night-but
Copy link
Contributor

This follows the same syntax that is allowed by sql-formatter, which we use in beekeeper-studio

interface ParamTypes {
  positional?: boolean, // ansi standard positional '?' parameters
  numbered?: Array<"?" | ":" | "$">, // psql like numeric parametrs '$1'
  named?: Array<":" | "@" | "$">, // mssql like named parameters ':name'
  quoted?: Array<":" | "@" | "$">, // quoted parameters, like bigquery ':`name here`'
  custom?: Array<string> // regex describing a custom parameter, eg. '/\{[a-zA-Z0-9_]+\}/' allows '{name}' as a param
}

@MasterOdin
Copy link
Member

Would this negate the need for #77 or does beekeeper want both?

I'm assuming the need here is that beekeeper wants to allow people to specify variables in any format or something regardless of what the database itself usually uses for parameters?

@not-night-but
Copy link
Contributor Author

not-night-but commented Apr 18, 2024

@MasterOdin I believe it would yes. I actually didn't see that PR till I filed this one (oops), but the idea here was to have the same config schema as sql-formatter, so that objects could be passed to both (as sql-formatter can replace the params). This also provides more options.

And yes the hope is to have parameters be agnostic of db type. Seems that in general is confused many users, and we already replace parameters before the query gets to the driver anyways

@not-night-but
Copy link
Contributor Author

@MasterOdin @rathboma

I've implemented the requested changes, minus how we classify params that fail to be scanned in scanParameter as I'm not sure how we want to handle it. I just left it as how we handled it before, but I'm open to changing that as well. Though I'm not sure how much that branch is even hit, as isParameter should prevent that from happening.

A couple things:

  1. I'm not sure defaultParamTypesFor should be where it is, feels like a utils fn, I just didn't want to add a file just for the function
  2. In that function, I kept the default behaviour for each dialect, and added bigquery, but sql-formatter supports a couple more by default for some dialects (eg sqlite has 4 different syntaxes for params). I'm just hesitant to change default behaviour and possibly break applications using identifier. Thoughts?

@rathboma
Copy link
Contributor

@not-night-but I think we should adhere to specs - so if SQLite has 4 different supported param types, that is what we should show. We can always increment the version if we feel like it's a bigger break

@MasterOdin
Copy link
Member

Yeah, making sqlite adhere more closely to the spec, and just returning more correctly for variable identification seems like a minor release, especially since supporting positional variables shouldn't change, would only affect people using a mix of variable types, but feels like we're incorrect in that instance.

@not-night-but
Copy link
Contributor Author

@MasterOdin @rathboma Implemented the changes:

  1. scanParameter now returns an unknown token if it can't determine what type the param is
  2. Sqlite now supports positional, ?1 numbered, :named, and @named parameters. As a result, one test was removed because it's no longer a possible case

@not-night-but not-night-but mentioned this pull request Apr 26, 2024
positional?: boolean;
numbered?: ('?' | ':' | '$')[];
named?: (':' | '@' | '$')[];
quoted?: (':' | '@' | '$')[];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Confirming that there is not any error checking if someone passes a value that's not in this array of values? I'm fine merging this as is, and then making my own PR doing this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. There is no error checking besides typescript type checking

@MasterOdin MasterOdin merged commit e15e335 into coresql:main Apr 30, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants