Feature/licensing infrastructure#93
Merged
Merged
Conversation
Three changes that together close issue #92 — UPDATE with a string-literal RHS silently overwrote the target column with null. 1. Update.sql (sql module) Render literals via `value.sql` instead of `value.value`. For StringValue the two differ: `.value` returns the bare scala string `USA`, while `.sql` returns the quoted form `'USA'`. For numerics and booleans the two coincide, so this change is identity for non-string literals. The bug surfaced because GatewayApi.run(update: Update) previously did `api.updateByQuery(update.table, update.sql)` — re-emitting the AST as a string and re-parsing it. The unquoted re-emission turned the bare token `USA` into an Identifier on the second parse, which routed `Update.customPipeline` through `ScriptProcessor.fromScript` and produced `def param1 = ctx.USA; ctx.country = param1`. With `ignore_failure = true` the field-miss was swallowed and the column was set to null. A new regression test in ParserSpec exercises the full parse → Update.sql → re-parse cycle and asserts the resulting customPipeline produces a SetProcessor (not a ScriptProcessor). 2. IndicesApi.updateByQuery (core module) Add an overload that accepts an already-parsed Update AST and skips the SQL parser entirely. The string-based overload is retained for callers submitting raw JSON. Both overloads share a private `runUpdateByQuery` helper for the post-parse path (pipeline extraction → WHERE → JSON → execute), so behaviour is identical regardless of entry point. ElasticClientDelegator and MetricsElasticClient forward the new overload. 3. GatewayApi.run (core module) Dispatch SQL UPDATE statements to the AST overload so the parse runs exactly once. This is the structural fix that closes the entire "AST → re-emit → re-parse" failure mode — even a future literal-rendering regression would no longer silently corrupt data through this path. Side effect of pulling the JSON content out of `parseQueryForUpdate`'s Right branch inside the shared helper: a latent bug where SELECT queries pre-converted to JSON were being discarded in favour of the raw input string is also fixed. Closes #92.
…Query Continuation of the issue #92 fix. The same audit applied to DELETE and INSERT — neither has the silent-data-corruption bug UPDATE had (Where.sql and Insert.sql round-trip safely through the parser), but both paid the same double-parse cost: GatewayApi.run was emitting `delete.sql` / `insert.sql` and IndicesApi was re-parsing the result. Mirror the updateByQuery refactor: - `IndicesApi` - Add `deleteByQuery(index, delete: Delete, refresh)` overload that skips parseSqlQueryForDeletion. Shared `runDeleteByQuery` helper holds the post-parse path (exists-check → openIfNeeded → execute → restore). Small `validateDeleteIndex` and `finalizeDeleteByQuery` helpers keep both entry points readable. - Add `insertByQuery(index, insert: Insert, refresh)` overload that skips parseInsertQuery. Shared `runInsertByQuery` helper holds the post-parse path (load index metadata → validate ON CONFLICT → validate AS SELECT columns → derive bulk options → build source → bulk insert). `validateInsertIndex` and `insertResultFinalize` factor out the index check and the recover-to-ElasticFailure plumbing. - `GatewayApi.run` - `case delete: Delete` and `case insert: Insert` now call the AST overloads directly. Each SQL DML statement is parsed exactly once on its way through the dispatcher. The other `insertByQuery` call site (INSERT … AS SELECT constructed from scratch for index population) keeps the string overload — it isn't a round-trip. - `ElasticClientDelegator` and `MetricsElasticClient` - Forward both new overloads (Delete / Insert imports added). `BulkResult` is now imported explicitly in IndicesApi so the `runInsertByQuery` helper can annotate its `Future[BulkResult]` return type without leaking it through the existing public API. No behaviour change for callers — the string overloads still exist and do the exact same work — the AST overloads just skip one parse. core compiles clean; ParserSpec UPDATE tests (including the round-trip regression from the previous commit) all pass.
…cation Narrow `LicenseRefreshStrategyFactory.resolveMode` from `private` to `private[licensing]` so the licensing module can verify the resolution contract directly, without going through SPI resolution. Add `LicenseRefreshStrategyFactoryDriverModeSpec` covering both branches: `refresh.enabled = false` -> Driver, `refresh.enabled = true` -> LongRunning. Companion to softclient4es-jdbc Story 10.3 (Review patch D2) - JDBC's own integration test cannot verify mode resolution end-to-end because its priority-1 TestLicenseManagerSpi ignores the mode parameter. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.