Skip to content

fix(studio): persist cleared optional instance and registry fields - #4466

Merged
lizhimins merged 1 commit into
apache:rocketmq-studiofrom
123123213weqw:fix/studio-cleared-optional-fields
Sep 21, 2026
Merged

lizhimins merged 1 commit into
apache:rocketmq-studiofrom
123123213weqw:fix/studio-cleared-optional-fields

Conversation

@123123213weqw

Copy link
Copy Markdown
Contributor

Which Issue(s) This PR Fixes

Brief Description

Clearing an optional field had no effect. The update replaces every editable field of the entry, but MyBatis-Plus updateById omits null entity fields, so an omitted column silently kept its previous value even though the request submitted no value for it:

This is one functional domain — "an omitted optional field means cleared, not unchanged" — so it covers both entry points in one change rather than one patch each.

It builds on #4276 and #4277, whose pull requests were closed when rocketmq-studio was folded into master and the base branch was deleted; #4276 is folded in here rather than resubmitted on its own. The changes are in save() and the registry update, not in the search paths, so this does not overlap the open LIKE-escaping change (#4458).

topic remark cannot be cleared (#4272) is the same class of defect but was already fixed by #4275, which is in master; it is deliberately not touched here.

How Did You Test This Change?

cd server && mvn -B -ntp test
[INFO] Tests run: 2431, Failures: 0, Errors: 0, Skipped: 0

The 17 errors reported alongside are the @SpringBootTest classes that need a reachable MySQL 8 (StudioApplicationTest, HealthProbeIntegrationTest, AuthService*IntegrationTest, RmqAlertStateMapperIntegrationTest); they fail identically on the untouched baseline, and the bundled deploy/docker-compose.yml MySQL is the intended way to run them.

Test coverage, each with its red/green pair from the original branch:

  • MybatisPlusInstanceRepositoryTest — an update that clears the admin credential reference writes NULL; against the unfixed code the stored reference survives and the assertion fails.
  • NameserverRegistryServiceTest — an update that omits the k8s namespace, k8s id and description writes NULL for each, and an update that supplies them still stores the supplied values.

Checklist

  • One coherent change; unrelated modifications are not bundled in
  • Commit subject follows Conventional Commits (fix:)
  • Tests added or updated for non-trivial changes, test methods named ...Test
  • New UI text has both Chinese and English entries under web/src/i18n/ — not applicable, no frontend change
  • Architecture constraints stay green (mvn test runs the ArchUnit checks)
  • New source files carry the ASF license header — no new source files
  • Documentation touched where behaviour changed — none of these behaviours is documented

@RockteMQ-AI RockteMQ-AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Fixes a persistence bug where clearing optional fields (like admin_credential_ref, k8s_namespace, description) had no effect because MyBatis-Plus updateById omits null entity fields. Now uses UpdateWrapper to explicitly set null values.

Review

  • Correctness ✅ — clearOmittedOptionalColumns() explicitly sets null for cleared fields using UpdateWrapper.set(column, null); the stored entry now matches the request
  • Consistency ✅ — Same pattern applied to both NameserverRegistryService and MybatisPlusInstanceRepository; instance and registry updates behave identically
  • Tests ✅ — New tests verify that clearing optional fields actually persists the null values
  • Compatibility ✅ — No API changes; persistence behavior only

Code Quality

Clear documentation explaining the MyBatis-Plus behavior and why the workaround is needed. The anyCleared flag avoids unnecessary UPDATE queries when no fields are being cleared.

LGTM — important fix for configuration management.


Automated review by github-manager-bot

@lizhimins
lizhimins changed the base branch from master to rocketmq-studio September 21, 2026 04:05
Clearing an optional field had no effect: the update replaces every editable
field of the entry, but MyBatis-Plus updateById omits null entity fields, so an
omitted column silently kept its previous value even though the request
submitted no value for it.

- A cleared instance admin credential reference survived the update; assign the
  column explicitly when the request omits it, next to the existing "update was
  not applied" guard.
- The same for the NameServer registry entry: an omitted k8s namespace, k8s id
  or description kept its previous value. The registry update now assigns the
  omitted optional columns explicitly.

Both paths keep the red/green pair from their original branches: an assertion
that a cleared reference is written as NULL, and one for each cleared registry
column.

Fixes apache#4273
Fixes apache#4274
@lizhimins
lizhimins force-pushed the fix/studio-cleared-optional-fields branch from d10ce21 to 61c76e5 Compare September 21, 2026 04:18
@lizhimins
lizhimins merged commit f759510 into apache:rocketmq-studio Sep 21, 2026
unbridled-41 added a commit to unbridled-41/rocketmq-dashboard that referenced this pull request Sep 21, 2026
…te omits it

The rule update replaces every editable field of the rule, but
MybatisPlusAlertRepository.replaceRule built the entity and relied on
updateById, whose NOT_NULL strategy omits null fields from the SET
clause. An update body that omits an optional value therefore passed
validation, the service echoed the submitted VO (field cleared), and
the stored rule silently kept its previous value - the scope filters,
severity, duration, channels, template and description could not be
cleared through the API. clearOmittedOptionalColumns now assigns those
columns explicitly, the same approach NameserverRegistryService uses
for its cleared registry columns (merged apache#4466). lastTriggered is not
an editable field (owned by markRuleTriggered) and keeps the
skip-on-null behaviour.
lizhimins pushed a commit that referenced this pull request Sep 24, 2026
… them (#4784)

test(alert): pin that a fully-loaded rule update skips the clear pass

The clear pass only runs when a column would otherwise be silently
skipped. Lock the other half of that contract: an update carrying every
optional value issues updateById and nothing else, so the
fully-loaded callers (toggle, bulk toggle, import) are provably
unaffected.

fix(alert): persist a cleared optional column when an alert-rule update omits it

The rule update replaces every editable field of the rule, but
MybatisPlusAlertRepository.replaceRule built the entity and relied on
updateById, whose NOT_NULL strategy omits null fields from the SET
clause. An update body that omits an optional value therefore passed
validation, the service echoed the submitted VO (field cleared), and
the stored rule silently kept its previous value - the scope filters,
severity, duration, channels, template and description could not be
cleared through the API. clearOmittedOptionalColumns now assigns those
columns explicitly, the same approach NameserverRegistryService uses
for its cleared registry columns (merged #4466). lastTriggered is not
an editable field (owned by markRuleTriggered) and keeps the
skip-on-null behaviour.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

3 participants