You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by "ClaireLytt (via GitHub)" <gi...@apache.org> on 2026/08/02 05:38:51 UTC
[I] Design contract for SQL Server OPENQUERY encrypt rewrite [shardingsphere]
ClaireLytt opened a new issue, #39313:
URL: https://github.com/apache/shardingsphere/issues/39313
## Feature Request
**For English only**, other languages will not be accepted.
Please pay attention on issues you submitted, because we maybe need more details.
If no response anymore and we cannot make decision by current information, we will **close it**.
Please answer these questions before submitting your issue. Thanks!
### Is your feature request related to a problem?
Yes. PR #39156 explored encrypt rewrite for SQL Server `UPDATE OPENQUERY (...) SET ...`, but review showed that the current approach is blocked by missing design contracts rather than by more local patches.
OPENQUERY pass-through SQL is executed by the linked-server provider, not by the local SQL Server engine. The current implementation:
1. looks up Encrypt metadata mainly by the terminal table name from the inner SQL;
2. owns a SQL Server-style inner SQL scanner/rewriter inside the Encrypt module;
3. always emits SQL Server bracket-quoted physical column names;
4. does not authoritatively detect linked-server provider identity.
As a result, it may rewrite the wrong remote table, emit SQL for the wrong dialect, or silently skip Encrypt rewrite. Review retained PR #39156 as Needs Discussion and recommended opening a focused design issue before infrastructure or Encrypt integration work.
This issue is a design follow-up to https://github.com/apache/shardingsphere/issues/30227 and PR [#39156](https://github.com/apache/shardingsphere/pull/39156). Implementation should not start until the contract below is accepted.
### Describe the feature you would like.
--
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: notifications-unsubscribe@shardingsphere.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Discussion]Design contract for SQL Server OPENQUERY encrypt rewrite [shardingsphere]
Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
terrymanu closed issue #39313: [Discussion]Design contract for SQL Server OPENQUERY encrypt rewrite
URL: https://github.com/apache/shardingsphere/issues/39313
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Discussion]Design contract for SQL Server OPENQUERY encrypt rewrite [shardingsphere]
Posted by "terrymanu (via GitHub)" <gi...@apache.org>.
terrymanu commented on issue #39313:
URL: https://github.com/apache/shardingsphere/issues/39313#issuecomment-5327503463
Hi @ClaireLytt, thanks for the well-structured write-up. The overall direction is right from my point of view: fail-closed semantics, retiring the hand-written scanner, and reusing the shared parser are all consistent with how we want this built. I'm fine with accepting the contract with the adjustments below, and I'd treat this as an enhancement (`db: SQLServer`, `in: Kernel`).
On the individual points:
1. **Provider boundary** — Agreed. Explicit `databaseType` per linked server is the only workable source at rewrite time; we cannot query `sys.servers` from the rewrite path. Unknown providers must fail closed.
2. **Compatibility nuance to add** — There is a small tension between "fail-closed on unconfigured linked server" and "users without the section keep today's behavior". Please make it explicit: when no linked-server section is configured at all, Encrypt ignores OPENQUERY entirely (current behavior); once the section exists, any linked server absent from it fails closed with `UnsupportedEncryptOperationException`. That keeps the additive-compatibility promise precise.
3. **Metadata ownership** — I can live with option (b), for one reason: the mapping target is the logical-table namespace, which is already shared across features, so binding remote identities inside the Encrypt rule would force sharding/mask to duplicate the same mapping later. Keep the standalone section to identity mapping only (`linked server -> databaseType + catalog.schema.table -> logical table`); column-level semantics stay in each feature rule. If others prefer starting under the Encrypt rule for minimal surface and promoting later, that is also defensible — please weigh in.
4. **Identity mapping** — Agreed, and the no-fallback rule is the important part. `EncryptRule.findEncryptTable(tableName)` matches by bare name today, which is exactly the collision the identity model removes.
5. **Parser reuse (Option 1)** — Agreed, and the two open questions have concrete answers. `SQLParserRule` already parses additional SQL text at runtime (see `ProxySQLComQueryParser`), and once the pass-through literal is unescaped, the inner statement is a regular SQL Server statement. `SQLToken` offsets are absolute within the rewritten string, so the natural composition is: rewrite the inner statement as its own SQL, then splice back using the outer literal position. The real risk is the T-SQL escaping layer — doubled apostrophes inside the literal shift offsets and must be re-escaped in the rewritten output; this was a P1 in the #39156 review, so the spike must cover string literals containing `''`.
6. **Verification** — Rewrite-level coverage does not need a live linked server: the config-driven `SQLRewriterIT` pattern plus Encrypt unit tests cover identity resolution and the fail-closed branches. A real linked-server E2E needs two SQL Server containers plus `sp_addlinkedserver`; I'd defer that decision until the feature is stable rather than block the contract on it.
Next step: please run the feasibility spike (your steps 1 and 2, including `''`-escaped literals and bracket/dollar/at/hash identifiers) and report the results in this issue. If the spike passes, we can consider the contract accepted and split the work into an infrastructure PR followed by the Encrypt integration PR, migrating the boundary scenarios from #39156 and deleting `EncryptOpenQueryPassThroughSQL` rather than extending it.
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] [Discussion]Design contract for SQL Server OPENQUERY encrypt rewrite [shardingsphere]
Posted by "ClaireLytt (via GitHub)" <gi...@apache.org>.
ClaireLytt commented on issue #39313:
URL: https://github.com/apache/shardingsphere/issues/39313#issuecomment-5604870664
@terrymanu
## Feasibility Spike Results
Per your request, I ran the feasibility spike to verify whether the existing SQL Server ANTLR parser can parse OPENQUERY inner SQL and whether token offsets compose correctly with the outer statement. The spike covers all the areas you specified: `''`-escaped literals, bracket-delimited identifiers, and `$`/`#` identifiers.
**All 7 tests pass.** Option 1 (reuse existing parser) is fully viable.
### Test Summary
| # | Test | Result |
| --- | ---------------------------------------------------------------------------------------------------------------------------------------------------- | ------ |
| 1 | Basic `SELECT ... FROM ... WHERE` parse — correct AST, column/table segment offsets accurate | Pass |
| 2 | Bracket-delimited identifiers — `[HumanResources].[Department]` resolves to owner + table | Pass |
| 3 | Special character identifiers — `Depart$ment`, `#Department` parse correctly | Pass |
| 4 | Comments — `/* block */` and `-- line` comments handled | Pass |
| 5 | `''`-escaped string literal — decodes and parses correctly | Pass |
| 6 | End-to-end offset composition — extract inner SQL from `FunctionSegment`, parse independently, rewrite column, splice back at correct outer position | Pass |
| 7 | End-to-end with `''` escaping — full decode → parse → rewrite → re-encode → splice cycle produces correct output | Pass |
### Key Technical Findings
**Parser reuse works standalone.** The existing API `SQLParserRule.getSQLParserEngine(sqlServerType).parse(innerSQL, false)` handles OPENQUERY inner SQL with no modifications. This is the same pattern already used by `EncryptShowCreateTableMergedResult` and `ProxySQLComQueryParser`.
**OPENQUERY AST structure is already sufficient.** The outer parser produces a `FunctionSegment` with the inner SQL accessible as `LiteralExpressionSegment` (2nd parameter of the OPENQUERY function). No grammar changes are needed.
**`''` escaping requires explicit handling.** `StringLiteralValue` strips the outer quotes but does not decode `''` → `'`. The correct flow is:
1. Extract from `getLiterals()` (still contains `''`)
2. Decode `''` → `'`
3. Parse the decoded string with the SQL Server parser
4. Rewrite using AST offsets
5. Re-encode `'` → `''`
6. Splice back into the outer SQL at `[literalSeg.startIndex, literalSeg.stopIndex]`
**Offset composition is straightforward.** The inner statement is rewritten as its own SQL (offsets are relative to the inner string), then spliced back using the outer literal position. In practice, this is a single `Substitutable` token covering the outer literal's range, whose `toString()` returns `'` + re-encoded inner SQL + `'`.
### Conclusion
No new parser infrastructure or extension contract (Option 2) is needed. The hand-written scanner `EncryptOpenQueryPassThroughSQL` can be fully retired and replaced with the shared parser.
### Proposed Next Steps
1. Consider the contract accepted based on these results.
2. Split the implementation into:
- **Infrastructure PR** — linked-server configuration, identity mapping, parser reuse infrastructure
- **Encrypt Integration PR** — OPENQUERY UPDATE rewrite on the agreed contract, migrate boundary scenarios from #39156, delete `EncryptOpenQueryPassThroughSQL`
Could you confirm the contract is accepted so I can proceed with the infrastructure PR?
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Design contract for SQL Server OPENQUERY encrypt rewrite [shardingsphere]
Posted by "ClaireLytt (via GitHub)" <gi...@apache.org>.
ClaireLytt commented on issue #39313:
URL: https://github.com/apache/shardingsphere/issues/39313#issuecomment-5179149959
This proposal outlines the design specifications for SQL Server’s OPENQUERY encryption rewriting feature. Please review and confirm the feasibility of this approach to facilitate further development efforts. Thank you for your time! @terrymanu
--
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: notifications-unsubscribe@shardingsphere.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org