[#10173] Guard the empty list case in sql generation for PostgreSQL by pithecuse527 · Pull Request #10261 · apache/gravitino

pithecuse527 · 2026-03-05T16:44:12Z

What changes were proposed in this pull request?

Guard empty roleIds from generating invalid PostgreSQL IN () SQL in UserRoleRel and GroupRoleRel providers.

Why are the changes needed?

(Please clarify why the changes are needed. For instance,

If you propose a new API, clarify the use case for a new API.
If you fix a bug, describe the bug.)

Fix: #10173

Does this PR introduce any user-facing change?

No

How was this patch tested?

UTs

…eSQL

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Copilot · 2026-03-05T19:25:45Z

...e/gravitino/storage/relational/mapper/provider/postgresql/UserRoleRelPostgreSQLProvider.java

        + ") "
+        + "</if>"
+        + "<if test='roleIds == null or roleIds.size() == 0'>"
+        + "AND 1= 0 "


"AND 1= 0 " has inconsistent spacing compared to the other providers (AND 1 = 0). For readability/consistency (and to avoid style drift in generated SQL), align it to AND 1 = 0(also consider removing the extra space after=`).

Suggested change

+ "AND 1= 0 "

+ "AND 1 = 0 "

Copilot · 2026-03-05T19:25:46Z

...avitino/storage/relational/mapper/provider/postgresql/TestUserRoleRelPostgreSQLProvider.java

+
+    Assertions.assertFalse(
+        normalizedSql.matches(".*\\bIN\\s*\\(\\s*\\).*"),
+        "Empty roleIds should not generate invalid SQL IN (...) with no values");


This assertion only verifies the absence of IN (), but it would still pass if the SQL accidentally dropped the role filter entirely (potentially updating all roles for the user/group). Consider strengthening the test by also asserting the presence of the intended guard predicate (e.g., AND 1 = 0) or otherwise verifying the WHERE clause becomes unsatisfiable when roleIds is empty. Same pattern applies to the other three new test classes.

Suggested change

"Empty roleIds should not generate invalid SQL IN (...) with no values");

"Empty roleIds should not generate invalid SQL IN (...) with no values");

Assertions.assertTrue(

normalizedSql.matches(".*\\b1\\s*=\\s*0\\b.*"),

"Empty roleIds should result in an unsatisfiable WHERE clause (e.g., AND 1 = 0)");

Copilot · 2026-03-05T19:25:46Z

...pache/gravitino/storage/relational/mapper/provider/base/TestGroupRoleRelBaseSQLProvider.java

+    SqlSource sqlSource =
+        new XMLLanguageDriver().createSqlSource(new Configuration(), script, Map.class);
+    Map<String, Object> params = new HashMap<>();
+    params.put("groupId", 1L);
+    params.put("roleIds", Collections.emptyList());
+
+    BoundSql boundSql = sqlSource.getBoundSql(params);
+    String normalizedSql = boundSql.getSql().replaceAll("\\s+", " ").trim();
+
+    Assertions.assertFalse(
+        normalizedSql.matches(".*\\bIN\\s*\\(\\s*\\).*"),
+        "Empty roleIds should not generate invalid SQL IN (...) with no values");


The four new test classes appear to be near-identical aside from provider type and parameter key (userId vs groupId). To reduce duplication and ease future changes (e.g., adjusting normalization/assertions), consider extracting a small shared helper (e.g., assertNoEmptyInClause(script, params)) or converting to a parameterized test with inputs (provider/script supplier + id key).

Suggested change

SqlSource sqlSource =

new XMLLanguageDriver().createSqlSource(new Configuration(), script, Map.class);

Map<String, Object> params = new HashMap<>();

params.put("groupId", 1L);

params.put("roleIds", Collections.emptyList());

BoundSql boundSql = sqlSource.getBoundSql(params);

String normalizedSql = boundSql.getSql().replaceAll("\\s+", " ").trim();

Assertions.assertFalse(

normalizedSql.matches(".*\\bIN\\s*\\(\\s*\\).*"),

"Empty roleIds should not generate invalid SQL IN (...) with no values");

Map<String, Object> params = new HashMap<>();

params.put("groupId", 1L);

params.put("roleIds", Collections.emptyList());

assertNoEmptyInClause(

script, params, "Empty roleIds should not generate invalid SQL IN (...) with no values");

}

private void assertNoEmptyInClause(

String script, Map<String, Object> params, String failureMessage) {

SqlSource sqlSource =

new XMLLanguageDriver().createSqlSource(new Configuration(), script, Map.class);

BoundSql boundSql = sqlSource.getBoundSql(params);

String normalizedSql = boundSql.getSql().replaceAll("\\s+", " ").trim();

Assertions.assertFalse(

normalizedSql.matches(".*\\bIN\\s*\\(\\s*\\).*"), failureMessage);

No real need to do this, unless you want to, WET or DAMP tests are usually fine.

yuqi1129 · 2026-03-06T03:57:36Z

@pithecuse527
Could you take the time resolve the comments?

Copilot

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Copilot · 2026-03-06T03:59:22Z

...org/apache/gravitino/storage/relational/mapper/provider/base/UserRoleRelBaseSQLProvider.java

+        + "<if test='roleIds != null and roleIds.size() > 0'>"
+        + "AND role_id IN ("
        + "<foreach collection='roleIds' item='roleId' separator=','>"
        + "#{roleId}"
        + "</foreach>"
-        + " )"
-        + " AND deleted_at = 0"
+        + ") "
+        + "</if>"
+        + "<if test='roleIds == null or roleIds.size() == 0'>"
+        + "AND 1 = 0 "
+        + "</if>"


These two <if> blocks are mutually exclusive, but the intent is clearer and less repetitive using <choose>/<when>/<otherwise>. Refactoring to <choose> would make the “non-empty list” vs “empty/null list” branching more explicit and reduce the chance of future edits accidentally creating overlapping conditions.

...pache/gravitino/storage/relational/mapper/provider/base/TestGroupRoleRelBaseSQLProvider.java

jerryshao

Code Review

Good fix — MyBatis does not support IN () with an empty collection, so guarding it is necessary.

Minor issues

Typo / inconsistent spacing: In UserRoleRelPostgreSQLProvider.softDeleteUserRoleRelByUserAndRoles, the empty-list guard reads "AND 1= 0 " (missing space before 0). All other providers use "AND 1 = 0 ". Please fix for consistency.
Consider early return at service layer: Using AND 1 = 0 is a valid SQL trick, but it is a hidden no-op that may surprise future maintainers reading the mapper. If the caller already knows roleIds is empty and no rows should be affected, consider returning early before calling the mapper (or asserting non-empty at the service layer). This would make the intent explicit. If you prefer to keep the SQL-level guard (e.g., for atomicity), a comment explaining the intent would help.

Positive

Four unit tests covering all four providers is thorough.
The fix is symmetric across base and PostgreSQL providers, which is correct.

Please fix the spacing typo before merging.

[apache#10173] guard the empty list case in sql generation for Postgr…

778cec7

…eSQL

Copilot AI review requested due to automatic review settings March 5, 2026 16:44

Copilot AI reviewed Mar 5, 2026

View reviewed changes

pithecuse527 requested a review from Copilot March 5, 2026 17:21

Copilot AI reviewed Mar 5, 2026

View reviewed changes

pithecuse527 requested a review from Copilot March 5, 2026 19:24

Copilot AI reviewed Mar 5, 2026

View reviewed changes

yuqi1129 requested a review from Copilot March 6, 2026 03:53

Copilot AI reviewed Mar 6, 2026

View reviewed changes

Copilot started reviewing on behalf of yuqi1129 March 6, 2026 04:00 View session

jerryshao reviewed Mar 6, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#10173] Guard the empty list case in sql generation for PostgreSQL#10261

[#10173] Guard the empty list case in sql generation for PostgreSQL#10261
pithecuse527 wants to merge 1 commit intoapache:mainfrom
pithecuse527:fix/invalid-postgresql-with-empty-roleids

pithecuse527 commented Mar 5, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

Copilot AI Mar 5, 2026

Uh oh!

justinmclean Mar 5, 2026

Uh oh!

yuqi1129 commented Mar 6, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 6, 2026

Uh oh!

Uh oh!

jerryshao left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

-        "Empty roleIds should not generate invalid SQL IN (...) with no values");
+        "Empty roleIds should not generate invalid SQL IN (...) with no values");
+    Assertions.assertTrue(
+        normalizedSql.matches(".*\\b1\\s*=\\s*0\\b.*"),
+        "Empty roleIds should result in an unsatisfiable WHERE clause (e.g., AND 1 = 0)");

Conversation

pithecuse527 commented Mar 5, 2026

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

justinmclean Mar 5, 2026

Choose a reason for hiding this comment

Uh oh!

yuqi1129 commented Mar 6, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 6, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jerryshao left a comment

Choose a reason for hiding this comment

Minor issues

Positive

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants