Add support for role query parameters #328
Conversation
|
|
| console.log(await rows1.json()) | ||
|
|
||
| await client.close() | ||
| })() |
There was a problem hiding this comment.
We aim to have runnable examples; I suggest using the roles bootstrap from tests here.
It is also a good idea to:
- demonstrate expected failures with CH error codes (printing it in the console will suffice so that we can see that 497 ACCESS_DENIED, just like in some other examples, like
abort_request.ts,cancel_query.ts, and probably some others) - print the output rows like in all other examples (the result set should be consumed, as it holds an unreleased keep-alive socket, waiting for streaming to start; again, a misuse in an example - ResultSets should not be "dangling" in the code)
There was a problem hiding this comment.
Updated the examples so that they run successfully, consume output rows, and print any error codes.
Example output:
| // with a role defined in the client configuration, all queries will use the specified role | ||
| await client.command({ | ||
| query: `SELECT * FROM SECURED_TABLE`, | ||
| }) |
There was a problem hiding this comment.
This should be a query call; now it is a misuse in an example, we want to avoid that
There was a problem hiding this comment.
Updated all SELECT queries to use .query instead of .command
examples/README.md
Outdated
| @@ -67,6 +67,7 @@ If something is missing, or you found a mistake in one of these examples, please | |||
| - [default_format_setting.ts](default_format_setting.ts) - sending queries using `exec` method without a `FORMAT` clause; the default format will be set from the client settings. | |||
| - [session_id_and_temporary_tables.ts](session_id_and_temporary_tables.ts) - creating a temporary table, which requires a session_id to be passed to the server. | |||
| - [session_level_commands.ts](session_level_commands.ts) - using SET commands, memorized for the specific session_id. | |||
| - [role.ts](role.ts) - using one more more roles, without explicit `USE` commands or session IDs | |||
There was a problem hiding this comment.
| - [role.ts](role.ts) - using one more more roles, without explicit `USE` commands or session IDs | |
| - [role.ts](role.ts) - using one or more roles without explicit `USE` commands or session IDs |
| @@ -0,0 +1,23 @@ | |||
| import { createClient } from '@clickhouse/client' // or '@clickhouse/client-web' | |||
|
|
|||
| void (async () => { | |||
There was a problem hiding this comment.
Let's add a "See also" link to https://clickhouse.com/docs/en/interfaces/http#setting-role-with-query-parameters
| @@ -78,6 +78,7 @@ describe('toSearchParams', () => { | |||
| qaz: 'qux', | |||
| }, | |||
| session_id: 'my-session-id', | |||
| role: ['my-role-1', 'my-role-2'], | |||
There was a problem hiding this comment.
Let's add a test with a single role (as a string).
| if (role && typeof role === 'string') { | ||
| params.set('role', role) | ||
| } else if (role && Array.isArray(role)) { | ||
| for (const r of role) { | ||
| params.append('role', r) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Nit: maybe just a single undefined check will suffice?
| if (role && typeof role === 'string') { | |
| params.set('role', role) | |
| } else if (role && Array.isArray(role)) { | |
| for (const r of role) { | |
| params.append('role', r) | |
| } | |
| } | |
| if (role) { | |
| if (typeof role === 'string') { | |
| params.set('role', role) | |
| } else if (Array.isArray(role)) { | |
| for (const r of role) { | |
| params.append('role', r) | |
| } | |
| } | |
| } | |
|
The CLA also needs to be signed before we merge it. |
Will do - I am working with my company to make sure I am not violating any of their open source policies by signing, but will sign as soon as I have approval. |
Summary
This PR adds support for specifying roles via request query parameters. Closes #269.
Checklist
Delete items not relevant to your PR: