You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@shardingsphere.apache.org by zh...@apache.org on 2026/03/16 19:50:34 UTC
(shardingsphere) branch master updated: Add more test cases on DatabaseIdentifierContextFactoryTest (#38470)
This is an automated email from the ASF dual-hosted git repository.
zhangliang pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/shardingsphere.git
The following commit(s) were added to refs/heads/master by this push:
new fa98b38b717 Add more test cases on DatabaseIdentifierContextFactoryTest (#38470)
fa98b38b717 is described below
commit fa98b38b717bb6ce4d92bde3e291587f183b8e84
Author: Liang Zhang <zh...@apache.org>
AuthorDate: Tue Mar 17 03:50:27 2026 +0800
Add more test cases on DatabaseIdentifierContextFactoryTest (#38470)
* Add more test cases on DatabaseIdentifierContextFactoryTest
* Add more test cases on DatabaseIdentifierContextFactoryTest
* Add more test cases on DatabaseIdentifierContextFactoryTest
---
.codex/skills/review-pr/SKILL.md | 21 ++++
.../DatabaseIdentifierContextFactoryTest.java | 117 +++++++++++++++++----
2 files changed, 117 insertions(+), 21 deletions(-)
diff --git a/.codex/skills/review-pr/SKILL.md b/.codex/skills/review-pr/SKILL.md
index 8c9897918ba..f32d5f6fd1a 100644
--- a/.codex/skills/review-pr/SKILL.md
+++ b/.codex/skills/review-pr/SKILL.md
@@ -43,6 +43,19 @@ description: >-
for example: same-name object, shadowing, disabled-flag path, or adjacent valid input.
If no such validation exists, bias to `Not Mergeable`.
11. Closing previous-round blockers and passing happy-path tests is not sufficient for `Mergeable`; you must still run a fresh-pass risk scan on the latest head.
+12. If a PR touches any shared execution path, reusable SPI, metadata assembly path, or other code that can affect multiple dialects or features,
+ treat it as a blast-radius review.
+ Trigger signals (examples only, not a whitelist): `infra/common`, `infra/binder/core`, shared kernel entrypoints, common SPI, reusable metadata loaders,
+ shared name-resolution logic, or shared default-schema / fallback logic.
+ You must explicitly enumerate the non-target dialects or features that also execute that path, and review at least one concrete counterexample outside the PR's stated target.
+ Do not interpret the trigger signals above as a complete list. If this blast-radius scan is missing, bias to `Not Mergeable`.
+13. If local verification is used to support mergeability and the command is module-scoped, include `-am` by default unless you can prove all dependent modules were built from the same latest PR head.
+ Do not treat a scoped run without dependent modules as conclusive evidence for `Mergeable`.
+14. Before outputting `Mergeable`, perform one explicit adversarial pass that assumes the PR is unsafe and actively searches for:
+ - one cross-dialect or adjacent-feature regression path,
+ - one config-disabled or feature-flag-off path,
+ - one original symptom path that is only partially covered by tests.
+ If any of these remain unresolved, set `Merge Verdict: Not Mergeable`.
## Execution Boundary
@@ -107,12 +120,15 @@ When information gaps block mergeability, request at least:
- Compatibility: behavior/config/API-SPI/SQL dialect versions
- Regression: similar statements, adjacent features, exception branches
- For parser, binder, routing, and default-schema changes, explicitly compare the new behavior against official dialect semantics and check whether precedence or shadowing rules changed
+ - If shared code is touched, build a blast-radius list of affected dialects/features and review at least one non-target example against its documented semantics
+ - If config flags or temporary properties exist on the touched path, review both enabled and disabled states
5. Test adequacy:
- Is there a failing case first or reproducible steps?
- Are major branches, boundaries, and counterexamples covered?
- Are tests mapped one-to-one with fix points?
- Distinguish fixture-assisted validation from production-path validation; if tests bypass the real assembly chain,
metadata loader, SPI discovery path, or routing path, state that gap explicitly and downgrade confidence
+ - If the PR adds multiple static metadata definitions, verify that regression tests cover the originally reported objects one-to-one; do not infer coverage from a single representative object unless the code path is truly identical and that equivalence is stated
6. Supply-chain and license gates (triggered by changes):
- If dependency manifests or lockfiles changed, check vulnerability severity and license constraints
- Mark whether extra security review is required
@@ -132,6 +148,8 @@ When information gaps block mergeability, request at least:
- Are adjacent paths sharing the same root cause also validated?
- Does the fix preserve the original precedence or lookup semantics for adjacent valid cases?
- Would a same-name or shadowing case take a different path after this patch?
+- If the fix sits in shared code, would another dialect or feature now take a different path from its documented semantics?
+- Does the config-disabled or feature-flag-off path still behave correctly?
If the root-cause chain cannot be fully proven fixed, set `Merge Verdict: Not Mergeable`.
@@ -147,6 +165,7 @@ If the root-cause chain cannot be fully proven fixed, set `Merge Verdict: Not Me
- Name-resolution or fallback-precedence compatibility
- Feature-flag or disabled-path compatibility
- Functional degradation risk: old-scenario regression, boundary input behavior changes, error-code/exception semantic drift.
+- Cross-dialect/shared-path risk: a target-specific fix changing generic behavior for other databases or features.
- Operational risk: config migration complexity, gray-release and rollback complexity.
- Supply-chain risk: vulnerabilities, licenses, transitive dependency changes from new deps.
@@ -229,3 +248,5 @@ Use committer tone, gentle wording, no emojis; structure:
- Do not ignore unrelated changes.
- Do not reuse old conclusions after new commits are added without re-review.
- Do not include emojis in change request text.
+- Do not output `Mergeable` for a shared-code change unless you have checked at least one non-target dialect or feature that also uses the changed path.
+- Do not output `Mergeable` when local verification omitted `-am` on a module-scoped Maven run and dependency freshness matters.
diff --git a/infra/common/src/test/java/org/apache/shardingsphere/infra/metadata/identifier/DatabaseIdentifierContextFactoryTest.java b/infra/common/src/test/java/org/apache/shardingsphere/infra/metadata/identifier/DatabaseIdentifierContextFactoryTest.java
index fc9522267e7..5a97429bc81 100644
--- a/infra/common/src/test/java/org/apache/shardingsphere/infra/metadata/identifier/DatabaseIdentifierContextFactoryTest.java
+++ b/infra/common/src/test/java/org/apache/shardingsphere/infra/metadata/identifier/DatabaseIdentifierContextFactoryTest.java
@@ -26,21 +26,29 @@ import org.apache.shardingsphere.infra.config.props.ConfigurationProperties;
import org.apache.shardingsphere.infra.config.props.ConfigurationPropertyKey;
import org.apache.shardingsphere.infra.config.props.MetadataIdentifierCaseSensitivity;
import org.apache.shardingsphere.infra.metadata.database.resource.ResourceMetaData;
+import org.apache.shardingsphere.infra.metadata.database.resource.unit.StorageUnit;
import org.apache.shardingsphere.infra.spi.type.typed.TypedSPILoader;
import org.apache.shardingsphere.infra.util.props.PropertiesBuilder;
import org.apache.shardingsphere.infra.util.props.PropertiesBuilder.Property;
import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+import javax.sql.DataSource;
import java.util.Collections;
-import java.util.Properties;
+import java.util.stream.Stream;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
-import static org.junit.jupiter.api.Assertions.assertFalse;
import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
class DatabaseIdentifierContextFactoryTest {
+ private static final DatabaseType DATABASE_TYPE = TypedSPILoader.getService(DatabaseType.class, "FIXTURE");
+
@Test
void assertCreateDefault() {
DatabaseIdentifierContext actual = DatabaseIdentifierContextFactory.createDefault();
@@ -49,32 +57,99 @@ class DatabaseIdentifierContextFactoryTest {
assertTrue(actualRule.matches("Foo", "foo", QuoteCharacter.NONE));
}
- @Test
- void assertCreateWithProtocolTypeAndProps() {
- DatabaseType databaseType = TypedSPILoader.getService(DatabaseType.class, "Oracle");
- DatabaseIdentifierContext actual = DatabaseIdentifierContextFactory.create(databaseType, new ConfigurationProperties(new Properties()));
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("createWithProtocolTypeAndPropsArguments")
+ void assertCreateWithProtocolTypeAndProps(final String name, final DatabaseType protocolType, final ConfigurationProperties props, final LookupMode expectedLookupMode,
+ final String actualIdentifier, final String logicIdentifier, final boolean expectedMatched) {
+ DatabaseIdentifierContext actual = DatabaseIdentifierContextFactory.create(protocolType, props);
IdentifierCaseRule actualRule = actual.getRule(IdentifierScope.TABLE);
- assertTrue(actualRule.matches("FOO", "foo", QuoteCharacter.NONE));
- assertFalse(actualRule.matches("Foo", "foo", QuoteCharacter.NONE));
+ assertThat(actualRule.getLookupMode(QuoteCharacter.NONE), is(expectedLookupMode));
+ assertThat(actualRule.matches(actualIdentifier, logicIdentifier, QuoteCharacter.NONE), is(expectedMatched));
}
- @Test
- void assertCreateWithResourceMetaDataAndProps() {
- DatabaseType databaseType = TypedSPILoader.getService(DatabaseType.class, "PostgreSQL");
- ConfigurationProperties props = new ConfigurationProperties(PropertiesBuilder.build(new Property(ConfigurationPropertyKey.METADATA_IDENTIFIER_CASE_SENSITIVITY.getKey(), "sensitive")));
- DatabaseIdentifierContext actual = DatabaseIdentifierContextFactory.create(databaseType, new ResourceMetaData(Collections.emptyMap(), Collections.emptyMap()), props);
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("createWithResourceMetaDataAndPropsArguments")
+ void assertCreateWithResourceMetaDataAndProps(final String name, final DatabaseType protocolType, final ResourceMetaData resourceMetaData,
+ final ConfigurationProperties props, final LookupMode expectedLookupMode,
+ final String actualIdentifier, final String logicIdentifier, final boolean expectedMatched) {
+ DatabaseIdentifierContext actual = DatabaseIdentifierContextFactory.create(protocolType, resourceMetaData, props);
IdentifierCaseRule actualRule = actual.getRule(IdentifierScope.TABLE);
- assertThat(actualRule.getLookupMode(QuoteCharacter.NONE), is(LookupMode.EXACT));
+ assertThat(actualRule.getLookupMode(QuoteCharacter.NONE), is(expectedLookupMode));
+ assertThat(actualRule.matches(actualIdentifier, logicIdentifier, QuoteCharacter.NONE), is(expectedMatched));
}
- @Test
- void assertRefreshWithProtocolTypeAndProps() {
- DatabaseType databaseType = TypedSPILoader.getService(DatabaseType.class, "PostgreSQL");
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("refreshWithProtocolTypeAndPropsArguments")
+ void assertRefreshWithProtocolTypeAndProps(final String name, final DatabaseType protocolType, final ConfigurationProperties props, final LookupMode expectedLookupMode,
+ final String actualIdentifier, final String logicIdentifier, final boolean expectedMatched) {
+ DatabaseIdentifierContext actual = DatabaseIdentifierContextFactory.createDefault();
+ DatabaseIdentifierContextFactory.refresh(actual, protocolType, props);
+ IdentifierCaseRule actualRule = actual.getRule(IdentifierScope.TABLE);
+ assertThat(actualRule.getLookupMode(QuoteCharacter.NONE), is(expectedLookupMode));
+ assertThat(actualRule.matches(actualIdentifier, logicIdentifier, QuoteCharacter.NONE), is(expectedMatched));
+ }
+
+ @ParameterizedTest(name = "{0}")
+ @MethodSource("refreshWithResourceMetaDataAndPropsArguments")
+ void assertRefreshWithResourceMetaDataAndProps(final String name, final DatabaseType protocolType, final ResourceMetaData resourceMetaData,
+ final ConfigurationProperties props, final LookupMode expectedLookupMode,
+ final String actualIdentifier, final String logicIdentifier, final boolean expectedMatched) {
DatabaseIdentifierContext actual = DatabaseIdentifierContextFactory.createDefault();
- ConfigurationProperties props = new ConfigurationProperties(PropertiesBuilder.build(
- new Property(ConfigurationPropertyKey.METADATA_IDENTIFIER_CASE_SENSITIVITY.getKey(), MetadataIdentifierCaseSensitivity.SENSITIVE.name())));
- DatabaseIdentifierContextFactory.refresh(actual, databaseType, props);
+ DatabaseIdentifierContextFactory.refresh(actual, protocolType, resourceMetaData, props);
IdentifierCaseRule actualRule = actual.getRule(IdentifierScope.TABLE);
- assertThat(actualRule.getLookupMode(QuoteCharacter.NONE), is(LookupMode.EXACT));
+ assertThat(actualRule.getLookupMode(QuoteCharacter.NONE), is(expectedLookupMode));
+ assertThat(actualRule.matches(actualIdentifier, logicIdentifier, QuoteCharacter.NONE), is(expectedMatched));
+ }
+
+ private static Stream<Arguments> createWithProtocolTypeAndPropsArguments() {
+ return Stream.of(
+ Arguments.of("null protocol type and null props use insensitive rules", null, null, LookupMode.NORMALIZED, "Foo", "foo", true),
+ Arguments.of("sensitive props override protocol rules", DATABASE_TYPE,
+ createConfigurationProperties(MetadataIdentifierCaseSensitivity.SENSITIVE), LookupMode.EXACT, "Foo", "foo", false),
+ Arguments.of("insensitive props normalize identifiers", DATABASE_TYPE,
+ createConfigurationProperties(MetadataIdentifierCaseSensitivity.INSENSITIVE), LookupMode.NORMALIZED, "Foo", "foo", true));
+ }
+
+ private static Stream<Arguments> createWithResourceMetaDataAndPropsArguments() {
+ return Stream.of(
+ Arguments.of("null resource metadata and null props use insensitive rules", null, null, null, LookupMode.NORMALIZED, "Foo", "foo", true),
+ Arguments.of("null storage units keep explicit sensitive rules", DATABASE_TYPE, new ResourceMetaData(Collections.emptyMap(), null),
+ createConfigurationProperties(MetadataIdentifierCaseSensitivity.SENSITIVE), LookupMode.EXACT, "Foo", "foo", false),
+ Arguments.of("empty storage units keep explicit sensitive rules", DATABASE_TYPE, new ResourceMetaData(Collections.emptyMap(), Collections.emptyMap()),
+ createConfigurationProperties(MetadataIdentifierCaseSensitivity.SENSITIVE), LookupMode.EXACT, "Foo", "foo", false),
+ Arguments.of("first data source path keeps explicit sensitive rules", DATABASE_TYPE, createResourceMetaDataWithFirstDataSource(),
+ createConfigurationProperties(MetadataIdentifierCaseSensitivity.SENSITIVE), LookupMode.EXACT, "Foo", "foo", false));
+ }
+
+ private static Stream<Arguments> refreshWithProtocolTypeAndPropsArguments() {
+ return Stream.of(
+ Arguments.of("null protocol type and null props refresh to insensitive rules", null, null, LookupMode.NORMALIZED, "Foo", "foo", true),
+ Arguments.of("sensitive props refresh to exact lookup", DATABASE_TYPE,
+ createConfigurationProperties(MetadataIdentifierCaseSensitivity.SENSITIVE), LookupMode.EXACT, "Foo", "foo", false),
+ Arguments.of("insensitive props refresh to normalized lookup", DATABASE_TYPE,
+ createConfigurationProperties(MetadataIdentifierCaseSensitivity.INSENSITIVE), LookupMode.NORMALIZED, "Foo", "foo", true));
+ }
+
+ private static Stream<Arguments> refreshWithResourceMetaDataAndPropsArguments() {
+ return Stream.of(
+ Arguments.of("null resource metadata and null props refresh to insensitive rules", null, null, null, LookupMode.NORMALIZED, "Foo", "foo", true),
+ Arguments.of("null storage units refresh to exact lookup", DATABASE_TYPE, new ResourceMetaData(Collections.emptyMap(), null),
+ createConfigurationProperties(MetadataIdentifierCaseSensitivity.SENSITIVE), LookupMode.EXACT, "Foo", "foo", false),
+ Arguments.of("empty storage units refresh to exact lookup", DATABASE_TYPE, new ResourceMetaData(Collections.emptyMap(), Collections.emptyMap()),
+ createConfigurationProperties(MetadataIdentifierCaseSensitivity.SENSITIVE), LookupMode.EXACT, "Foo", "foo", false),
+ Arguments.of("first data source path refreshes to exact lookup", DATABASE_TYPE, createResourceMetaDataWithFirstDataSource(),
+ createConfigurationProperties(MetadataIdentifierCaseSensitivity.SENSITIVE), LookupMode.EXACT, "Foo", "foo", false));
+ }
+
+ private static ConfigurationProperties createConfigurationProperties(final MetadataIdentifierCaseSensitivity caseSensitivity) {
+ return new ConfigurationProperties(PropertiesBuilder.build(new Property(ConfigurationPropertyKey.METADATA_IDENTIFIER_CASE_SENSITIVITY.getKey(), caseSensitivity.name())));
+ }
+
+ private static ResourceMetaData createResourceMetaDataWithFirstDataSource() {
+ ResourceMetaData result = mock(ResourceMetaData.class);
+ StorageUnit storageUnit = mock(StorageUnit.class);
+ when(storageUnit.getDataSource()).thenReturn(mock(DataSource.class));
+ when(result.getStorageUnits()).thenReturn(Collections.singletonMap("foo_ds", storageUnit));
+ return result;
}
}