Add kernel linked-server module for OPENQUERY encrypt rewrite infrastructure - #39786
ClaireLytt wants to merge 17 commits into
Conversation
terrymanu
left a comment
There was a problem hiding this comment.
Result
Review Result: Not Mergeable
Feedback Mode: Change Request
Blocking Issues: 2
The linked-server module violates the accepted ownership and remote-identity contracts.
Blocking Issues
-
The mapping-only rule incorrectly classifies mapped tables as enhanced tables
- Evidence:
LinkedServerTableMapperRuleAttribute#getEnhancedTableNamesreturns every mapped logical table. Shared consumers consequently treat those tables as enhanced:SelectStatementBaseContextchanges derived-projection handling, whileSingleTableLoadUtilsclassifies them as feature-required single tables. The accepted design states that linked-server configuration owns identity mapping only and feature rules retain column-level semantics (discussion). - Impact: A linked-server mapping can alter projection/result-metadata behavior and single-table loading even when no consuming feature owns enhancement of that table. This is especially incorrect for the intended future sharding or mask consumers.
- Required Change: Do not expose linked-server mappings through
getEnhancedTableNames(). Keep enhancement ownership in Encrypt or the consuming feature, and add a focused regression test showing that linked-server mapping alone does not activate enhanced-table behavior.
- Evidence:
-
The API preserves the bare-table fallback that the identity contract was intended to remove
- Evidence:
LinkedServerConfigurationrepresents remote identities as uncheckedMap<String, String>keys, andLinkedServerTable#findLogicalTableaccepts any string. The tests explicitly configure and successfully resolve bare names such asDepartment(test). No configuration checker or identity type rejects this form. The agreed contract requireslinkedServer + catalog + schema + tableand forbids fallback to a terminal table name (discussion). - Impact: Same-named tables in different remote catalogs or schemas can still be configured or resolved through an ambiguous terminal name, recreating the wrong-table encryption risk this infrastructure is supposed to eliminate.
- Required Change: Represent or validate a canonical full remote-table identity at the linked-server configuration boundary, reject bare terminal names, and test distinct catalog/schema identities sharing the same table name.
- Evidence:
Coverage
- Candidate: Public PR #39786, head
ff120fc2b09151fa0c5226c68e4b7f26fe988559. - Baseline:
d613652c999968544ed7278e8e21897353e88d5c; verified merge base is identical. GitHub and compare scopes both contain the same 21 files. - Requirements: PR description, issue #39313 and its complete public discussion, repository contracts, and Microsoft OPENQUERY documentation.
- Files accounted for:
RELEASE-NOTES.md;kernel/pom.xml; all four files underkernel/linked-server/**/pom.xml; the two API Java files; twelve core Java files; two SPI service descriptors; and all three new test classes. - Behavior clusters: module/distribution wiring; public configuration and remote identity; runtime lookup and rule attributes; YAML/SPI persistence; OPENQUERY AST/escaping helpers; tests and release note.
- Discovery: Root-cause/behavior, blast-radius/contracts, tests/runtime/operations, and final convergence passes completed. No unresolved evidence gaps.
- Style verification:
./mvnw -pl kernel,kernel/linked-server,kernel/linked-server/api,kernel/linked-server/core spotless:check checkstyle:check -Pcheck -T1Cexited0. Spotless covered all four changed POMs and all 14 changed Java files; Checkstyle covered all 14 Java files.RELEASE-NOTES.mdand the two service descriptors are not governed by the configured Spotless formats; POMs and those three non-Java files are not governed by source Checkstyle. - Tests: The initial core-only test invocation exited
1solely because the new API artifact was unavailable locally. The corrected reactor command,./mvnw -pl kernel/linked-server/core -am -DskipITs -Dspotless.skip=true test, exited0; all 25 linked-server tests passed. - Dialect scope: SQL Server. No grammar or visitor was changed, and SQL Server has no repository branch dialect requiring propagation.
- CI scope: Code-correctness result only. GitHub Actions and checks were not reviewed; local style verification above is separate from CI.
…fied table keys - Return empty set from getEnhancedTableNames() since linked-server is identity-mapping only, not column-level enhancement - Validate table keys contain dot separator in LinkedServerConfiguration constructor to reject bare terminal names per apache#39313 contract - Update all test fixtures to use catalog.schema.table format - Add tests for bare-name rejection and cross-catalog disambiguation
|
@terrymanu
Issue2: Three tests cover this:
|
terrymanu
left a comment
There was a problem hiding this comment.
Result
Review Result: Not Mergeable
Feedback Mode: Change Request
Blocking Issues: 1
The current validation still accepts remote-table identities that omit a required component.
Blocking Issues
-
An omitted schema passes the “fully qualified” identity check
- Evidence:
LinkedServerConfigurationrequires at least two dots, soHumanResources..Departmentpasses; the compiled constructor accepted it locally.LinkedServerTablepreserves and resolves the key without further validation. SQL Server permits this syntax with the schema omitted (Microsoft example). The latest tests reject bare and two-part names, but do not cover omitted components. - Impact: The rule can accept a mapping without the explicit
catalog.schema.tableidentity required by issue #39313. That defeats the identity boundary this infrastructure must provide to Encrypt integration. - Required Change: Validate three present identity components at the configuration boundary, reject omitted or surplus components, and cover those cases in focused tests.
- Evidence:
Coverage
- Candidate: Public PR #39786, head
6c37281a0f298f4275ae98f8784dd45a538637f2; based613652c999968544ed7278e8e21897353e88d5c. GitHub’s changed-file list and comparison both contain the same 21 files. No local candidate delta was reviewed. - Requirements and scope: PR description, the complete public discussion of issue #39313, rule and YAML consumers, OPENQUERY helpers, tests, module wiring, and release note. All 21 files were accounted for across configuration/identity, runtime lookup and attributes, YAML/SPI, OPENQUERY helpers, tests, and packaging. Root-cause, contract, test/runtime, risk, and latest-head convergence passes found no other confirmed blocker or unresolved decisive gap. The two findings from the earlier public review are fixed at this head.
- Style verification: On an isolated snapshot of the effective head,
./mvnw -pl kernel/linked-server/api,kernel/linked-server/core -Pcheck -T1C spotless:check checkstyle:checkexited0. Spotless selected all 14 changed Java files and both module POMs; Checkstyle reports selected all 14 Java files with zero violations../mvnw -pl kernel,kernel/linked-server -Pcheck -T1C spotless:checkexited0and selectedkernel/pom.xmlandkernel/linked-server/pom.xml. The Java inventory comprises the two API files and twelve core production/test files in the authoritative PR list. Checkstyle does not govern POMs; the configured Spotless formats do not governRELEASE-NOTES.mdor the two SPI descriptors. - Tests:
./mvnw -pl kernel/linked-server/core -am -DskipITs -Dspotless.skip=true testexited0; all 29 linked-server tests passed. - CI scope: Code-correctness review only. GitHub Actions and checks were not reviewed; the local style checks above are separate from CI.
terrymanu
left a comment
There was a problem hiding this comment.
Result
Review Result: Not Mergeable
Feedback Mode: Change Request
Blocking Issues: 1
The linked-server identity lookup can conflate distinct SQL Server identifiers and return the wrong logical-table mapping instead of failing closed.
Blocking Issues
-
Case-insensitive identity maps silently overwrite distinct remote mappings
- Evidence:
LinkedServerRulestores server configurations in aCaseInsensitiveMap;LinkedServerTabledoes the same for qualified remote identities. The configuration validation checks only the three-part shape and does not detect equivalent-key conflicts (LinkedServerConfiguration). The test explicitly requires case-insensitive lookup but does not cover collisions (LinkedServerRuleTest). - Evidence: A candidate-local reproduction using the configured
java-util4.103.0 showed that two table keys differing only by case resolve to the later logical table; two server names differing only by case make the earlier server mapping unreachable. SQL Server permits object identifiers differing only by case under case-sensitive collation (Microsoft Database Identifiers). - Impact: A configured or unconfigured case-distinct remote table can be resolved to another logical table. The intended Encrypt consumer could consequently apply the wrong encryption metadata, violating the accepted full-identity and fail-closed contract in issue #39313.
- Required Change: Preserve remote identity distinctions unless authoritative provider/collation semantics prove them equivalent, and reject conflicting server or table mappings under the lookup equality rules. Add focused coverage for case-distinct SQL Server identities and conflicting case-equivalent server/table entries.
- Evidence:
Coverage
- Candidate: Public PR #39786, head
edba40690e08204e524dc38c8225cf6049b981ec. - Baseline:
d613652c999968544ed7278e8e21897353e88d5c; verified merge base is identical. GitHub and local triple-dot inventories match across all 21 changed files. - Files accounted for:
RELEASE-NOTES.md; four Maven POMs; two API Java files; nine core production Java files; two SPI descriptors; and three test classes. - Behavior clusters: module/distribution wiring; public configuration and remote identity; runtime lookup and rule attributes; YAML/SPI persistence; OPENQUERY AST/escaping utilities; tests and release note.
- Discovery: Root-cause/behavior, blast-radius/contracts, tests/runtime/operations, and latest-head convergence passes completed. The latest GitHub refresh confirmed the same head and file scope. No unresolved evidence gaps.
- Style verification:
./mvnw -pl kernel,kernel/linked-server,kernel/linked-server/api,kernel/linked-server/core -Pcheck -T1C spotless:check checkstyle:checkexited0. Spotless covered all 14 changed Java files and four changed POMs; Checkstyle covered all 14 Java files with zero violations.RELEASE-NOTES.mdand the two SPI descriptors are not governed by the configured Spotless formats or source Checkstyle; POMs are not governed by source Checkstyle. - Tests:
./mvnw -pl kernel/linked-server/core -am -DskipITs -Dspotless.skip=true testexited0; all 31 linked-server tests passed. - Parser scope: SQL Server only. No grammar or visitor changed. The OPENQUERY boundary was checked against Microsoft’s OPENQUERY specification; no dialect-family propagation is required.
- CI scope: Code-correctness review only. GitHub Actions and checks were not reviewed; the completed local style verification is separate from CI.
terrymanu
left a comment
There was a problem hiding this comment.
Result
Review Result: Not Mergeable
Feedback Mode: Change Request
Blocking Issues: 2
The latest head still has an unsafe remote-identity lookup and violates two applicable repository style rules.
Blocking Issues
-
Case-sensitive remote tables can resolve to the wrong logical table
- Evidence:
LinkedServerTablecopies mappings into aCaseInsensitiveMap. With onlyHumanResources.dbo.Departmentconfigured, a local reproduction showed that a lookup forHumanResources.dbo.departmentreturns its mapping. SQL Server permits those to be distinct tables under a case-sensitive database collation, as documented by Microsoft. The new duplicate-key checks prevent overwrites but do not prevent this lookup. - Impact: The rule can supply another remote table’s logical identity to the planned Encrypt rewrite instead of failing closed, contrary to the accepted identity contract.
- Required Change: Preserve distinct remote identities unless their equivalence is established by authoritative provider and collation information. Cover a configured table and an unconfigured table whose names differ only by case.
- Evidence:
-
Added Java code violates the repository’s manual style rules
- Evidence: The wrapped
checkArgumentcalls inLinkedServerRule.javaandLinkedServerConfiguration.javareconstruct to lines of 170, 182, and 191 characters; the line-wrapping rule requires one line at 200 characters or fewer. Asserted operation values are also namedresultinOpenQueryUtilsTest.javaandYamlLinkedServerRuleConfigurationSwapperTest.java, contrary to the test naming rule. - Impact: These added declarations do not meet the repository’s written standards, which Spotless and Checkstyle do not fully enforce.
- Required Change: Apply the one-line form where required and name asserted operation values with the
actualprefix throughout the affected tests.
- Evidence: The wrapped
Coverage
- Candidate: Public PR #39786, head
a19c57169ab820a8ce6c520985b6557d47129aef; based613652c999968544ed7278e8e21897353e88d5c. GitHub’s PR and comparison endpoints agree on all 21 changed files. No local candidate changes were reviewed. - Requirements and behavior: PR description, issue #39313 and its public discussion. The accepted infrastructure maps a configured linked server and full remote-table identity to a logical table; uncertain identity resolution must fail closed.
- Files accounted for: Four POMs (
kernel/pom.xmland the three underkernel/linked-server/); two API Java files (LinkedServerRuleConfiguration.java,LinkedServerConfiguration.java); nine core production Java files (LinkedServerOrder.java,OpenQueryUtils.java,LinkedServerRule.java,LinkedServerTableMapperRuleAttribute.java,LinkedServerRuleBuilder.java,LinkedServerTable.java,YamlLinkedServerRuleConfiguration.java,YamlLinkedServerConfiguration.java,YamlLinkedServerRuleConfigurationSwapper.java); three core tests (OpenQueryUtilsTest.java,LinkedServerRuleTest.java,YamlLinkedServerRuleConfigurationSwapperTest.java); two SPI descriptors; andRELEASE-NOTES.md. - Review passes: Module wiring, configuration and identity, runtime lookup, YAML and SPI, OPENQUERY helpers, tests, and release note were assessed through behavior, contract, runtime, and final convergence passes. No unresolved evidence gap remains.
- Local style verification: On a temporary archive of the exact head,
./mvnw -pl kernel,kernel/linked-server,kernel/linked-server/api,kernel/linked-server/core -Pcheck -T1C spotless:check checkstyle:checkexited0. Spotless selected all 14 listed Java files and four POMs; Checkstyle’s report lists all 14 Java files with zero violations. Checkstyle does not govern POMs. Neither configured check governsRELEASE-NOTES.mdor the two SPI descriptors. - Tests:
./mvnw -pl kernel/linked-server/core -am -DskipITs -Dspotless.skip=true testexited0; all 41 linked-server tests passed. - CI scope: Code-correctness review only. GitHub Actions and checks were not reviewed; the local style checks above are separate from CI.
|
Hi, thanks for the review! I've pushed fixes for both blocking issues from round 4. Issue 1: Case-sensitive remote tables can resolve to the wrong logical table
New test: The existing Issue 2: Style violations
|
terrymanu
left a comment
There was a problem hiding this comment.
Result
Review Result: Not Mergeable
Feedback Mode: Change Request
Blocking Issues: 2
The latest head passes local style checks and tests, but its configuration rejects a supported SQL Server table identity, and one test still violates a written repository naming rule.
Blocking Issues
-
Case-distinct remote tables cannot both be configured
- Evidence:
LinkedServerConfiguration.java:59rejects keys after lowercasing them; the test confirms the rejection. SQL Server permits tables whose names differ only by case under a case sensitive database collation (Microsoft documentation). The latest lookup change preserves case, but configuration still discards that distinction. - Impact: Users cannot map both remote tables to their respective logical tables under the accepted full-identity contract.
- Required Change: Remove the unconditional case-folded rejection or make it aware of the remote collation. Test two case-distinct identities mapped to different logical tables.
- Evidence:
-
A test result retains a noncompliant name
- Evidence:
OpenQueryUtilsTest.java:157-161stores the asserted result inreEncoded. The repository test rule requires anactualprefix or a direct assertion. This was present and missed in the previous review. - Impact: The added test does not meet the written standard; Checkstyle and Spotless do not enforce this rule.
- Required Change: Rename the asserted value with an
actualprefix or assert the expression directly.
- Evidence:
Coverage
- Candidate and requirements: Public PR #39786 at
62146c30c5f50442171e0ab60a04896026f4a184, based613652c999968544ed7278e8e21897353e88d5c. GitHub’s PR and comparison endpoints agree on all 21 changed files. The issue discussion accepts the linked-server identity mapping as staged infrastructure. - Files and review passes: Accounted for four POMs, two API Java files, nine core production Java files, three core tests, two SPI descriptors, and
RELEASE-NOTES.md. Reviewed configuration and lookup, rule ownership, YAML and SPI wiring, OPENQUERY helpers, tests, packaging, and the latest delta through root-cause, contract, runtime, and convergence passes. No unresolved evidence gap remains. - Local style verification: On an archive of the exact head,
./mvnw -pl kernel,kernel/linked-server,kernel/linked-server/api,kernel/linked-server/core -Pcheck -T1C spotless:check checkstyle:checkexited0. Spotless selected all four changed POMs and all 14 changed Java files; Checkstyle’s reports list all 14 Java files with zero violations. The Java files covered were APILinkedServerRuleConfiguration.javaandLinkedServerConfiguration.java; coreLinkedServerOrder.java,OpenQueryUtils.java,LinkedServerRule.java,LinkedServerTableMapperRuleAttribute.java,LinkedServerRuleBuilder.java,LinkedServerTable.java,YamlLinkedServerRuleConfiguration.java,YamlLinkedServerConfiguration.java,YamlLinkedServerRuleConfigurationSwapper.java,OpenQueryUtilsTest.java,LinkedServerRuleTest.java, andYamlLinkedServerRuleConfigurationSwapperTest.java. Checkstyle does not govern POMs; neither configured check governs the release note or SPI descriptors. - Tests:
./mvnw -pl kernel/linked-server/core -am -DskipITs -Dspotless.skip=true testexited0; all 42 linked-server tests passed. - Focus: Code correctness only. GitHub Actions and checks were not reviewed; the local style verification above is separate from CI.
- Jev: The typesafe-ai skill was not used because this required full-path code review rather than a bounded judgment. Jev MCP was not called because quota was unconfirmed: 0 calls and no judgments delegated. It did not change routing, conclusions, or execution; efficiency cannot be measured accurately, and no Jev-related problems were found.
for #39313.
Changes proposed in this pull request:
Before committing this PR, I'm sure that I have checked the following options:
./mvnw clean install -B -T1C -Dmaven.javadoc.skip -Dmaven.jacoco.skip -e.Claude Code was used to assist with code generation.