Revert "fix: unknown lucene field falls through in search" - #2447
Conversation
This reverts commit 8aad6d6.
|
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🔵 Tier 2 — Low RiskSmall, isolated change with no API route or data model modifications. Why this tier:
Review process: AI review + quick human skim (target: 5–15 min). Reviewer validates AI assessment and checks for domain-specific concerns. Stats
|
E2E Test Results✅ All tests passed • 199 passed • 3 skipped • 1326s
Tests ran across 4 shards in parallel. |
Greptile SummaryThis PR reverts #2422 ("fix: unknown lucene field falls through in search"), removing the SELECT-alias resolution feature (
Confidence Score: 3/5The revert reintroduces two broken behaviors: a null-dereference crash in the exact-match path and raw-SQL injection of unrecognized Lucene field names into ClickHouse queries. Removing the ?? new Map() guard means any metadata implementation returning null from getMaterializedColumnsLookupTable will crash with a TypeError at materializedColumns.entries(), outside the try/catch. The fallback returning { found: true } for unknown fields means ClickHouse will receive raw, unvalidated identifiers and reject the query at runtime. Both queryParser.ts (null guard removal and unknown-field fallback) and renderChartConfig.ts (alias wiring removal) warrant a close look before merging. Important Files Changed
Reviews (3): Last reviewed commit: "Merge branch 'main' into revert-2422-luc..." | Re-trigger Greptile |
| materializedColumns = | ||
| (await this.metadata.getMaterializedColumnsLookupTable({ | ||
| await this.metadata.getMaterializedColumnsLookupTable({ | ||
| databaseName: this.databaseName, | ||
| tableName: this.tableName, | ||
| connectionId: this.connectionId, | ||
| })) ?? new Map(); | ||
| }); |
There was a problem hiding this comment.
The
?? new Map() null-coalescing guard was intentionally added in #2422 to defend against getMaterializedColumnsLookupTable resolving to null or undefined. Without it, if the call resolves to null (e.g. some Metadata implementations or test mocks use mockResolvedValue(null)), materializedColumns.entries() will throw a TypeError outside the try/catch block — the catch only covers the await, not the iteration below it. The base mockMetadata in renderChartConfig.test.ts (line 40) still mocks this to return null, so any test that reaches this exact-match path with the default mock will now fail.
| materializedColumns = | |
| (await this.metadata.getMaterializedColumnsLookupTable({ | |
| await this.metadata.getMaterializedColumnsLookupTable({ | |
| databaseName: this.databaseName, | |
| tableName: this.tableName, | |
| connectionId: this.connectionId, | |
| })) ?? new Map(); | |
| }); | |
| materializedColumns = | |
| (await this.metadata.getMaterializedColumnsLookupTable({ | |
| databaseName: this.databaseName, | |
| tableName: this.tableName, | |
| connectionId: this.connectionId, | |
| })) ?? new Map(); |
| // It might be an alias, let's just try the column | ||
| // TODO: Verify aliases | ||
| return { | ||
| found: false, | ||
| found: true, | ||
| columnExpression: field, | ||
| columnType: 'Unknown', | ||
| }; | ||
| // throw new Error(`Column not found: ${field}`); |
There was a problem hiding this comment.
Unknown fields now always pass through as bare identifiers
The fallback now returns { found: true, columnExpression: field } for any field that is not a known column or prefix match, and the original throw is left commented out. This means an unrecognised Lucene field name (e.g. a typo) is injected as a raw SQL identifier, which ClickHouse will reject with an "Unknown identifier" error at query time instead of returning a clean no-match result. The // TODO: Verify aliases comment signals this is a known temporary state — worth tracking the replacement work so users don't get opaque ClickHouse errors on invalid field references.
…gh-as-a-raw-sql-identifier
…gh-as-a-raw-sql-identifier
Reverts #2422