Skip to content

fix(settings): preserve unmanaged settings fields on partial saves - #2863

Open
yyqdbngt wants to merge 1 commit into
apache:rocketmq-studiofrom
yyqdbngt:codex/yy-settings-partial-saves
Open

fix(settings): preserve unmanaged settings fields on partial saves#2863
yyqdbngt wants to merge 1 commit into
apache:rocketmq-studiofrom
yyqdbngt:codex/yy-settings-partial-saves

Conversation

@yyqdbngt

Copy link
Copy Markdown

Summary

  • In SettingsService.saveGeneralSettings, inherit the stored value for fields that a
    partial writer does not manage: dingtalkWebhook, smsWebhook, emailRecipients,
    llmEngine, maxTokens, temperature.
  • An absent (null) value now inherits the stored one; an explicit empty string still
    clears a text field, so the existing clear-by-empty-string behaviour is unchanged.
  • Added regression tests for both partial-writer directions and the explicit-clear case.

Why

The persisted settings blob has two writers that each manage only part of the fields:

  • the general-settings save (/api/settings/general/save, GeneralSettingsUpdateDTO)
    has no maxTokens/temperature properties, so every general save nulled the LLM
    tuning and the next LLM config load silently fell back to the defaults;
  • the LLM config save (LlmConfigService.saveConfig) builds the VO without
    dingtalkWebhook/smsWebhook/emailRecipients/llmEngine, so saving the LLM
    config wiped the notification channels and engine override.

Both were data-loss-on-save bugs in the opposite direction of each other.

Testing

  • cd server && mvn -Dtest=SettingsServiceTest test — Tests run: 46, Failures: 0, Errors: 0
  • cd server && mvn -Dtest=LlmConfigServiceTest test — Tests run: 28, Failures: 0, Errors: 0

@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

Clean fix for partial-writer data loss in saveGeneralSettings. When the general-settings UI and LLM-config UI each save a subset of fields, the null fields now correctly inherit from the stored value instead of silently overwriting it.

Findings

  • [Info] SettingsService.java:153 — Consider a helper method to reduce null-check repetition as the field list grows (non-blocking)

Suggestions

  • The existing awsRegion check uses !StringUtils.hasText() (inherits on empty string too), while the new checks use == null (only inherits on null). This distinction is intentional per the PR description and the tests confirm it — just flagging for awareness that the two patterns coexist.
  • Tests cover both directions (general-save preserves LLM, LLM-save preserves notifications) and the explicit-clear edge case. Good coverage.

Verdict

Straightforward bug fix with solid test coverage. LGTM.


Automated review by github-manager

// The settings blob has two partial writers: the general-settings save does not
// manage the LLM tuning values, and the LLM-config save does not manage the
// notification fields. An absent (null) value therefore inherits the stored one,
// while an explicit empty string still clears a text field.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Good documentation of the two-writer invariant. One consideration: if more fields are added later, the null-check boilerplate will grow linearly. A small helper method could keep this maintainable. Not blocking — just a thought for future iterations.

@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.

LGTM — defensive fix improving input validation and error handling.


Automated review by "github-manager-bot"

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants