Visitar URL original
[#10270] test(core): Guard @Param("roleId") binding on RoleMetaMapper.softDeleteRoleMetaByRoleId by dennismdejong · Pull Request #13666 · apache/gravitino · GitHub
Skip to content

[#10270] test(core): Guard @Param("roleId") binding on RoleMetaMapper.softDeleteRoleMetaByRoleId - #13666

Merged
yuqi1129 merged 2 commits into
apache:mainfrom
dennismdejong:dennismdejong/improvement-fix-missing-mybatis-param-for-role-s-2
Oct 8, 2026
Merged

[#10270] test(core): Guard @Param("roleId") binding on RoleMetaMapper.softDeleteRoleMetaByRoleId#13666
yuqi1129 merged 2 commits into
apache:mainfrom
dennismdejong:dennismdejong/improvement-fix-missing-mybatis-param-for-role-s-2

Conversation

@dennismdejong

@dennismdejong dennismdejong commented Oct 1, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Adds a reflection-based regression test (TestRoleMetaMapper#testSoftDeleteRoleMetaByRoleIdHasNamedParam) asserting that RoleMetaMapper.softDeleteRoleMetaByRoleId carries @Param("roleId") on its first argument. The @Param fix itself is already present in the current codebase.

Why are the changes needed?

Issue #10270 reports that a missing @Param("roleId") on RoleMetaMapper.softDeleteRoleMetaByRoleId could cause MyBatis named-parameter binding failures at runtime during role deletion via the REST delete-role path. The annotation is already in place on main (the method now also takes @Param("currentVersion") for OCC), but nothing guarded it from regressing. This test locks in the named binding.

Fix: #10270

Does this PR introduce any user-facing change?

No.

How was this patch tested?

  • ./gradlew :core:spotlessApply
  • ./gradlew :core:test --tests org.apache.gravitino.storage.relational.mapper.TestRoleMetaMapper -PskipITs -PskipDockerTests=true (1 test, 0 failures)

@yuqi1129

yuqi1129 commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

@dennismdejong
Did we only need to add test codes to fix the problem?

@github-actions

github-actions Bot commented Oct 6, 2026 •
edited
Loading

Copy link
Copy Markdown

Code Coverage Report

Overall Project 71.05% 🟢
Files changed No Java source files changed -

Module Coverage
aliyun 19.74% 🔴
api 53.88% 🟢
authorization-common 85.96% 🟢
authorization-ranger 14.44% 🔴
aws 53.54% 🟢
aws-bundle 0.0% 🔴
azure 32.1% 🔴
azure-bundle 0.0% 🔴
catalog-common 29.49% 🔴
catalog-fileset 82.06% 🟢
catalog-glue 73.24% 🟢
catalog-hive 83.63% 🟢
catalog-jdbc-common 46.31% 🟢
catalog-jdbc-doris 87.56% 🟢
catalog-jdbc-mysql 81.8% 🟢
catalog-jdbc-postgresql 83.89% 🟢
catalog-jdbc-starrocks 79.16% 🟢
catalog-kafka 75.21% 🟢
catalog-lakehouse-generic 78.48% 🟢
catalog-lakehouse-hudi 79.1% 🟢
catalog-lakehouse-iceberg 86.0% 🟢
catalog-lakehouse-paimon 84.36% 🟢
catalog-model 77.99% 🟢
cli 44.62% 🟢
client-java 78.31% 🟢
client-java-runtime 0.0% 🔴
common 60.13% 🟢
core 86.76% 🟢
filesystem-hadoop3 76.51% 🟢
flink 55.25% 🟢
flink-common 32.08% 🔴
flink-runtime 0.0% 🔴
gcp 32.2% 🔴
hadoop-auth 68.0% 🟢
hadoop-common 17.84% 🔴
hive-metastore-common 62.3% 🟢
hive-metastore3-libs 58.8% 🟢
iceberg-aliyun-bundle 0.0% 🔴
iceberg-common 66.53% 🟢
iceberg-rest-server 76.97% 🟢
idp-basic 86.77% 🟢
integration-test-common 0.0% 🔴
jobs 70.88% 🟢
lance-common 36.14% 🔴
lance-rest-server 69.51% 🟢
lineage 59.39% 🟢
optimizer 83.86% 🟢
optimizer-api 42.56% 🟢
server 91.11% 🟢
server-common 82.7% 🟢
spark 61.79% 🟢
tencent 81.78% 🟢
trino-connector 62.12% 🟢

@dennismdejong

dennismdejong commented Oct 6, 2026

Copy link
Copy Markdown
Contributor Author

@yuqi1129 The production change isn't needed here — the fix is already on main. @Param("roleId") was added to RoleMetaMapper.softDeleteRoleMetaByRoleId in #10664 (commit a7914c9ae), and #12746 later kept it while adding @Param("currentVersion") for OCC. So #10270's code fix is already in place; this PR only adds the regression guard the issue itself suggested, so the binding can't silently regress.

There is also existing runtime coverage: TestAuthMappers#testRoleDeleteUsesCurrentVersion calls the mapper through MyBatis, so a missing @Param would already fail there. The reflection test just makes the contract explicit. Happy to drop it if you'd prefer to rely on that existing coverage.

@yuqi1129

yuqi1129 commented Oct 7, 2026

Copy link
Copy Markdown
Contributor

@yuqi1129 The production change isn't needed here — the fix is already on main. @Param("roleId") was added to RoleMetaMapper.softDeleteRoleMetaByRoleId in #10664 (commit a7914c9ae), and #12746 later kept it while adding @Param("currentVersion") for OCC. So #10270's code fix is already in place; this PR only adds the regression guard the issue itself suggested, so the binding can't silently regress.

There is also existing runtime coverage: TestAuthMappers#testRoleDeleteUsesCurrentVersion calls the mapper through MyBatis, so a missing @Param would already fail there. The reflection test just makes the contract explicit. Happy to drop it if you'd prefer to rely on that existing coverage.

I see.

@yuqi1129
yuqi1129 merged commit 2c18230 into apache:main Oct 8, 2026
38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yuqi1129 yuqi1129 yuqi1129 approved these changes

Assignees

@dennismdejong dennismdejong

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[Improvement] Fix missing MyBatis @Param for role soft-delete mapper binding

2 participants

@dennismdejong @yuqi1129