feat: prefer query API sql param while preserving legacy params#3640
feat: prefer query API sql param while preserving legacy params#3640msmithstubbs wants to merge 2 commits into
sql param while preserving legacy params#3640Conversation
|
|
||
| backend_id = backend.id | ||
|
|
||
| expect(ClickHouseAdaptor, :execute_query, fn %{id: ^backend_id}, |
There was a problem hiding this comment.
running against a clickhouse instance is preferred as the stub makes the test more brittle to refactors of execute_query
2ce4cd5 to
931122b
Compare
| ), | ||
| {:ok, declared_params} <- Sql.parameters(expanded_query) do | ||
| {:ok, declared_params} <- | ||
| Sql.parameters(expanded_query, dialect: Sql.to_dialect(language)) do |
There was a problem hiding this comment.
Running tests against Clickhouse instance revealed the language was not being passed to Sql.parameters/1
![depthfirst-app[bot]](https://avatars.githubusercontent.com/in/1021002?s=60&v=4){kind=small}
| @spec extract_query(map()) :: {:ok, atom(), String.t()} | {:error, String.t()} | ||
| @spec extract_query(map()) :: | ||
| {:ok, :infer | :bq_sql | :ch_sql | :pg_sql, String.t()} | {:error, String.t()} | ||
| defp extract_query(%{"sql" => sql}), do: {:ok, :infer, sql} |
There was a problem hiding this comment.
🟡 Severity: MEDIUM
When bq_sql/ch_sql/pg_sql deprecated params are combined with a mismatched backend_id, the query is validated using the wrong SQL dialect but executed on the target backend. For example, bq_sql with a ClickHouse backend_id uses BigQuery SQL validation but runs on ClickHouse, potentially bypassing ClickHouse-specific SQL restrictions (e.g., system.* table access).
Helpful? Add 👍 / 👎
💡 Fix Suggestion
Suggestion: The root cause is in resolve_language/2 at lines 149–152, not in extract_query/1. The second clause defp resolve_language(language, _backend), do: language blindly returns the deprecated param's dialect (:bq_sql, :ch_sql, :pg_sql) even when an explicit backend has been resolved. Fix this by changing the first clause to match on a concrete %Backend{} struct (i.e. any non-nil backend) instead of only on :infer, so that whenever an explicit backend is present the dialect is always derived from that backend's actual type:
# Before:
defp resolve_language(:infer, backend),
do: EndpointQuery.map_backend_to_language(backend, SingleTenant.supabase_mode?())
defp resolve_language(language, _backend), do: language
# After:
defp resolve_language(_language, %Backend{} = backend),
do: EndpointQuery.map_backend_to_language(backend, SingleTenant.supabase_mode?())
defp resolve_language(language, _backend), do: languageWith this change, if a caller sends bq_sql=...&backend_id=<clickhouse_id>, the resolved ClickHouse backend struct matches the first clause and the dialect is correctly set to :ch_sql — the deprecated param's hint is ignored. When no backend_id is provided (fetch_backend returns nil), the second clause still applies and the legacy param's dialect (:ch_sql, :pg_sql, etc.) is preserved for backward compatibility with the 'first backend of that type' routing.
There was a problem hiding this comment.
Correct, but that is the legacy behaviour.
931122b to
e2add22
Compare
3 participants
/api/querypreferssqlparam over language specificch_sql|bq_sql|pg_sqlparamssqlandbackend_idSQL languages is determined by the backend.ch_sqlandpg_sqlwithout abackend_idwill query first backend of that typesqlwithout abackend_idis mapped tobq_sqland the default system backend (BigQuery or other).Follow on from #3093 comment