Copilot commented on code in PR #1209:
URL:
https://github.com/apache/skywalking-banyandb/pull/1209#discussion_r3534882424
##########
ui/src/components/BydbQL/Index.vue:
##########
@@ -584,6 +658,26 @@ SELECT * FROM STREAM log in sw_recordsLog TIME > '-30m'`);
@ready="onCodeMirrorReady"
/>
</div>
+ <div class="query-params-container">
+ <div class="params-header">
+ <span class="params-title">Parameters (bound to ? placeholders in
order)</span>
+ <el-button size="small" @click="addParam" :disabled="loading"> Add
Parameter </el-button>
+ </div>
+ <div v-for="(param, index) in queryParams" :key="index"
class="param-row">
+ <span class="param-index">?{{ index + 1 }}</span>
+ <el-select v-model="param.type" size="small" class="param-type">
Review Comment:
Using the array index as the v-for key can cause row state to “shift” when
removing parameters (inputs/selects may appear to change the wrong row). Use a
stable per-parameter id (e.g., store an `id` on each param when added) and key
by that id instead of `index`.
##########
docs/interacting/bydbql.md:
##########
@@ -154,6 +157,76 @@ TIME < '-1d'
The `parseTime` function automatically determines whether a timestamp is
absolute (RFC3339) or relative (duration string) and converts it appropriately.
+## 2.6. Parameter Binding
+
+BydbQL supports positional parameter binding with the `?` placeholder, so
untrusted values never need to be concatenated into the query string. This
eliminates QL injection: a bound value is always treated as a plain value and
can never alter the query structure.
+
+```sql
+SELECT trace_id, message
+FROM STREAM logs IN group1
+TIME > ?
+WHERE message MATCH(?)
+```
+
+Parameters are supplied alongside the query and are bound to the placeholders
in their order of appearance. Each parameter is a `model.v1.TagValue`. Over
HTTP:
+
+```json
+POST /api/v1/bydbql/query
+{
+ "query": "SELECT trace_id, message FROM STREAM logs IN group1 TIME > ? WHERE
message MATCH(?)",
+ "params": [
+ {"str": {"value": "-30m"}},
+ {"str": {"value": "error"}}
+ ]
+}
+```
+
+### 2.6.1. Supported Positions
+
+Placeholders are accepted in *value* positions only. The complete list, with
the parameter types each position accepts:
+
+| Position | Example | Accepted parameter types |
+|---|---|---|
+| `TIME` comparison value | `TIME > ?` | `str` (RFC3339, relative like `-30m`,
or `now`), `timestamp` |
Review Comment:
The supported-positions table says TIME placeholders accept only
`str`/`timestamp`, but later in this same section you note that `int`
parameters bind in TIME positions (and binder.go also accepts `TagValue_Int`).
This is internally inconsistent and may mislead users; either include `int` in
the table (with the existing caveat) or change binding rules to reject it.
##########
mcp/src/query/validation.ts:
##########
@@ -119,12 +133,77 @@ export function normalizeQueryHints(args: unknown):
QueryHints {
return {
description: typeof rawArgs.description === 'string' ?
rawArgs.description.trim() : undefined,
BydbQL: typeof rawArgs.BydbQL === 'string' ? rawArgs.BydbQL.trim() :
undefined,
+ params: Array.isArray(rawArgs.params) ? (rawArgs.params as BydbQLParam[])
: undefined,
resource_type: typeof rawArgs.resource_type === 'string' ?
rawArgs.resource_type.trim() : undefined,
resource_name: typeof rawArgs.resource_name === 'string' ?
rawArgs.resource_name.trim() : undefined,
group: typeof rawArgs.group === 'string' ? rawArgs.group.trim() :
undefined,
};
}
+function validateParamString(value: unknown, position: number): string {
+ if (typeof value !== 'string') {
+ throw new Error(`params[${position}]: str value must be a string`);
+ }
+ if (value.length > maxParamValueLength) {
+ throw new Error(`params[${position}]: value exceeds ${maxParamValueLength}
characters`);
+ }
+ return value;
+}
+
+function validateParamInteger(value: unknown, position: number): string {
+ if (typeof value === 'number' && Number.isSafeInteger(value)) {
+ return String(value);
+ }
+ if (typeof value === 'string' && integerPattern.test(value)) {
+ return value;
+ }
+ throw new Error(`params[${position}]: int value must be an integer`);
+}
Review Comment:
validateParamInteger accepts arbitrarily long integer strings (it only
regex-checks), even though maxParamValueLength exists. This can allow very
large param payloads (e.g., megabytes of digits) to get passed through to the
HTTP API, which is unnecessary and can become a resource/DoS vector. Apply the
same length limit used for string params after converting the int to its string
representation.
##########
docs/api-reference.md:
##########
@@ -2117,6 +2117,7 @@ QueryRequest is the main request message for BydbQL
queries
| Field | Type | Label | Description |
| ----- | ---- | ----- | ----------- |
| query | [string](#string) | | query is the BydbQL query string |
+| params | [banyandb.model.v1.TagValue](#banyandb-model-v1-TagValue) |
repeated | params are bound to the `?` placeholders in the query string by
their order of appearance. The number of params must match the number of
placeholders. Accepted variants depend on the placeholder's position:
str/timestamp for TIME values, str/int/null for scalar comparisons,
str/int/null/str_array/int_array for IN/MATCH/HAVING value lists (arrays expand
in place), and int for LIMIT/OFFSET/TOP N counts. binary_data is rejected
everywhere. |
Review Comment:
This API reference text says TIME placeholders accept only str/timestamp,
but the binder accepts int parameters in TIME positions as well. Keep this
description consistent with the actual binding rules (or update the binding
rules to match the docs).
##########
api/proto/banyandb/bydbql/v1/query.proto:
##########
@@ -33,6 +34,13 @@ option java_package =
"org.apache.skywalking.banyandb.bydbql.v1";
message QueryRequest {
// query is the BydbQL query string
string query = 1 [(validate.rules).string.min_len = 1];
+ // params are bound to the `?` placeholders in the query string by their
order of appearance.
+ // The number of params must match the number of placeholders.
+ // Accepted variants depend on the placeholder's position: str/timestamp for
TIME values,
+ // str/int/null for scalar comparisons, str/int/null/str_array/int_array for
IN/MATCH/HAVING
+ // value lists (arrays expand in place), and int for LIMIT/OFFSET/TOP N
counts.
Review Comment:
The QueryRequest `params` comment says TIME placeholders accept only
str/timestamp, but the binder accepts int parameters in TIME positions
(BindParams.bindTimeValue handles TagValue_Int) and the docs mention that int
binds but later fails transformation. Please align this proto comment with
actual accepted variants (or adjust binder semantics accordingly).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
