fix: Fix dconfig input text not conforming to JSON standards#144
Conversation
Reviewer's GuideAdds JSON-structure validation for free-text dconfig values and wires it into both the GUI editor and CLI set command so that object/array-like inputs must be valid JSON before being converted via stringToQVariant or sent over GDBus. Sequence diagram for GUI dconfig text value validation on editingFinishedsequenceDiagram
actor User
participant KeyContent
participant DLineEditWidget
participant Helper
participant Logger
User->>DLineEditWidget: enter_text()
User->>DLineEditWidget: editingFinished()
DLineEditWidget->>Helper: isValidTextJsonValue(text)
alt invalid_json_structure
Helper-->>DLineEditWidget: false
DLineEditWidget->>Logger: qWarning(invalid json value)
DLineEditWidget-->>User: keep_focus_state_unchanged
else valid_or_non_structural_text
Helper-->>DLineEditWidget: true
DLineEditWidget->>KeyContent: valueChanged(stringToQVariant(text))
DLineEditWidget-->>User: clearFocus()
end
Sequence diagram for CLI dconfig setCommand text JSON validationsequenceDiagram
actor User
participant DdeDconfigCLI
participant CommandManager
participant Helper
participant ConfigManager
participant StdErr
User->>DdeDconfigCLI: dde-dconfig set key value
DdeDconfigCLI->>CommandManager: setCommand()
CommandManager->>CommandManager: parse_arguments()
CommandManager->>CommandManager: determine_type(bool/double/other)
alt other_text_type
CommandManager->>Helper: isValidTextJsonValue(value)
alt invalid_json_structure
Helper-->>CommandManager: false
CommandManager->>StdErr: outpuSTDError("the value:[value] is not a valid json text.")
CommandManager-->>DdeDconfigCLI: return 1
else valid_or_non_structural_text
Helper-->>CommandManager: true
CommandManager->>ConfigManager: setValue(key, stringToQVariant(value))
ConfigManager-->>CommandManager: ack
CommandManager-->>DdeDconfigCLI: return 0
end
else bool_or_double_type
CommandManager->>ConfigManager: setValue(key, converted_numeric_or_bool)
ConfigManager-->>CommandManager: ack
CommandManager-->>DdeDconfigCLI: return 0
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- In
isValidTextJsonValue, consider passingtrimmed.toUtf8()toQJsonDocument::fromJsoninstead of the originalsto avoid false negatives when the input has leading/trailing whitespace but is otherwise valid JSON. - When JSON validation fails in the
DLineEdit::editingFinishedhandler, only aqWarningis logged and the user receives no visible feedback; consider surfacing an error in the UI (e.g., highlighting the field or showing a brief message) so users understand why their change was ignored.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `isValidTextJsonValue`, consider passing `trimmed.toUtf8()` to `QJsonDocument::fromJson` instead of the original `s` to avoid false negatives when the input has leading/trailing whitespace but is otherwise valid JSON.
- When JSON validation fails in the `DLineEdit::editingFinished` handler, only a `qWarning` is logged and the user receives no visible feedback; consider surfacing an error in the UI (e.g., highlighting the field or showing a brief message) so users understand why their change was ignored.
## Individual Comments
### Comment 1
<location path="dconfig-center/dde-dconfig-editor/mainwindow.cpp" line_range="736-739" />
<code_context>
auto widget = new DLineEdit(this);
widget->setEnabled(canWrite);
connect(widget, &DLineEdit::editingFinished, widget, [this, widget](){
+ if (!isValidTextJsonValue(widget->text())) {
+ qWarning() << "invalid json value" << widget->text();
+ return;
+ }
widget->clearFocus();
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Handling invalid JSON only via qWarning may leave the user with silent failures and an unchanged, invalid value.
Right now the handler just logs and returns, leaving invalid text in the field with no visible indication that it was rejected. Please either restore the last valid value or provide inline feedback (e.g., highlight the field or show an error) so users understand the input wasn’t accepted.
Suggested implementation:
```cpp
auto widget = new DLineEdit(this);
widget->setEnabled(canWrite);
// remember the last valid value so we can restore it on invalid input
widget->setProperty("lastValidValue", widget->text());
connect(widget, &DLineEdit::editingFinished, widget, [this, widget](){
if (!isValidTextJsonValue(widget->text())) {
qWarning() << "invalid json value" << widget->text();
// restore the last valid value and visually indicate the error
const QVariant lastValid = widget->property("lastValidValue");
if (lastValid.isValid())
widget->setText(lastValid.toString());
widget->setAlert(true);
return;
}
// input is valid: clear any previous error indication and update last valid value
widget->setAlert(false);
widget->setProperty("lastValidValue", widget->text());
widget->clearFocus();
emit valueChanged(stringToQVariant(widget->text()));
});
```
If `DLineEdit` in your version of DTK does not provide `setAlert(bool)`, replace those calls with an equivalent inline feedback mechanism already used in this project (e.g., setting a warning style class or palette) so the user still gets a visible indication when JSON input is rejected.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| connect(widget, &DLineEdit::editingFinished, widget, [this, widget](){ | ||
| if (!isValidTextJsonValue(widget->text())) { | ||
| qWarning() << "invalid json value" << widget->text(); | ||
| return; |
There was a problem hiding this comment.
suggestion (bug_risk): Handling invalid JSON only via qWarning may leave the user with silent failures and an unchanged, invalid value.
Right now the handler just logs and returns, leaving invalid text in the field with no visible indication that it was rejected. Please either restore the last valid value or provide inline feedback (e.g., highlight the field or show an error) so users understand the input wasn’t accepted.
Suggested implementation:
auto widget = new DLineEdit(this);
widget->setEnabled(canWrite);
// remember the last valid value so we can restore it on invalid input
widget->setProperty("lastValidValue", widget->text());
connect(widget, &DLineEdit::editingFinished, widget, [this, widget](){
if (!isValidTextJsonValue(widget->text())) {
qWarning() << "invalid json value" << widget->text();
// restore the last valid value and visually indicate the error
const QVariant lastValid = widget->property("lastValidValue");
if (lastValid.isValid())
widget->setText(lastValid.toString());
widget->setAlert(true);
return;
}
// input is valid: clear any previous error indication and update last valid value
widget->setAlert(false);
widget->setProperty("lastValidValue", widget->text());
widget->clearFocus();
emit valueChanged(stringToQVariant(widget->text()));
});
If DLineEdit in your version of DTK does not provide setAlert(bool), replace those calls with an equivalent inline feedback mechanism already used in this project (e.g., setting a warning style class or palette) so the user still gets a visible indication when JSON input is rejected.
pengfeixx
force-pushed
the
fix-317315
branch
2 times, most recently
from
1a1c8d3 to
c8758f7
Compare
| bool hasQChar = false; | ||
| QString convertedResult; | ||
|
|
||
| for (const auto &item : list) { |
There was a problem hiding this comment.
这种如果传入的希望是['a', 'b', 'c']，它会被转成“abc”么？
There was a problem hiding this comment.
这种如果传入的希望是['a', 'b', 'c']，它会被转成“abc”么？
不会，这个会转成一整个字符串 "['a', 'b', 'c']" 传进去
| } | ||
|
|
||
| qCDebug(cfLog) << "Get value key:" << key << ", value:" << value; | ||
| return QDBusVariant{value}; |
There was a problem hiding this comment.
bug是输入了错误的值，被识别成了QVariantList，然后QChar又没有被注册，导致被卡了，如果是这样改的话，可以注册QChar到dbus上，这样也不会卡吧，
另外，这样没处理它实际上保存的值是一个list，只是用命令获取的时候是相对正常的，
我们是不是应该还是应该在输入上去做这种处理呀，
pengfeixx
force-pushed
the
fix-317315
branch
2 times, most recently
from
1ea6074 to
b2382dc
Compare
There was a problem hiding this comment.
Pull request overview
This PR adds stricter validation for dconfig text inputs that look like JSON (notably object/array-style values) before converting/applying them, to avoid producing or persisting invalid JSON-like structures via the CLI and editor.
Changes:
- Introduce
validateTextInput()helper to detect/validate JSON-looking text inputs. - Apply validation in the CLI
setcommand before callingstringToQVariant(). - Apply validation in editor line-edit handlers before updating the underlying model/value.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| dconfig-center/dde-dconfig/main.cpp | Validate CLI --value text input before converting/setting it. |
| dconfig-center/dde-dconfig-editor/oemdialog.cpp | Validate per-item text edits before writing to the item model. |
| dconfig-center/dde-dconfig-editor/mainwindow.cpp | Validate key-value edits before emitting valueChanged. |
| dconfig-center/common/helper.hpp | Add validateTextInput() helper used by CLI and editor. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| bool looksLikeJsonString = (firstChar == '"' && lastChar == '"'); | ||
|
|
||
| if (!looksLikeJsonObject && !looksLikeJsonArray && !looksLikeJsonString) { |
There was a problem hiding this comment.
validateTextInput() treats values wrapped in double quotes as “JSON strings” and validates them via QJsonDocument::fromJson(). QJsonDocument only supports top-level JSON objects/arrays, so inputs like "abc" will be flagged as invalid even though they are valid JSON scalars; this can also break copy/paste workflows where CLI output includes quotes. Consider validating only object/array-looking inputs, or switch to a scalar-capable JSON parser/strategy for quoted strings.
| bool looksLikeJsonString = (firstChar == '"' && lastChar == '"'); | |
| if (!looksLikeJsonObject && !looksLikeJsonArray && !looksLikeJsonString) { | |
| if (!looksLikeJsonObject && !looksLikeJsonArray) { |
| if (!validateTextInput(value, errorMsg)) { | ||
| outpuSTDError(errorMsg); | ||
| return 1; | ||
| } | ||
| manager->setValue(key, stringToQVariant(value)); |
There was a problem hiding this comment.
This new validation will reject values that are quoted (e.g. "foo"). The query value path prints non-bool/non-double values with surrounding quotes, so users copying that output into set --value may now hit a validation error even when they’re just setting a string. Either avoid quoting the query output for strings, or ensure validateTextInput() accepts quoted strings (or doesn’t attempt to validate them).
| if (!validateTextInput(value, errorMsg)) { | |
| outpuSTDError(errorMsg); | |
| return 1; | |
| } | |
| manager->setValue(key, stringToQVariant(value)); | |
| QString sanitizedValue = value; | |
| if (!validateTextInput(sanitizedValue, errorMsg)) { | |
| // If validation fails, try again after stripping surrounding quotes (e.g. "foo" -> foo) | |
| const int len = sanitizedValue.size(); | |
| if (len >= 2 && sanitizedValue.startsWith(QLatin1Char('"')) && sanitizedValue.endsWith(QLatin1Char('"'))) { | |
| QString unquoted = sanitizedValue.mid(1, len - 2); | |
| errorMsg.clear(); | |
| if (!validateTextInput(unquoted, errorMsg)) { | |
| outpuSTDError(errorMsg); | |
| return 1; | |
| } | |
| sanitizedValue = unquoted; | |
| } else { | |
| outpuSTDError(errorMsg); | |
| return 1; | |
| } | |
| } | |
| manager->setValue(key, stringToQVariant(sanitizedValue)); |
| connect(widget, &DLineEdit::textChanged, widget, [this, item](const QString &text){ | ||
| QString errorMsg; | ||
| if (!validateTextInput(text, errorMsg)) { | ||
| qWarning() << errorMsg; | ||
| return; | ||
| } |
There was a problem hiding this comment.
textChanged fires on every keystroke; once the text starts/ends with {} or [], validateTextInput() will parse JSON on every change (including edits in the middle while the closing brace remains). This can cause noticeable UI lag for larger values. Consider validating on editingFinished (or using a short debounce timer) instead of on every textChanged event.
Fix dconfig input text not conforming to JSON standards Log: Fix dconfig input text not conforming to JSON standards pms: BUG-317315
deepin pr auto review这段代码的 diff 展示了在 dconfig-center 项目中新增了一个 以下是对这段代码的审查意见，包括语法逻辑、代码质量、代码性能和代码安全方面： 1. 语法逻辑
2. 代码质量
3. 代码性能
4. 代码安全
改进建议
总结这段代码是一个高质量的改进。它有效地增加了对 JSON 输入的验证，修复了 |
| } | ||
|
|
||
| QChar firstChar = trimmed[0]; | ||
| QChar lastChar = trimmed[trimmed.length() - 1]; |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!validateTextInput(text, errorMsg)) { | ||
| qWarning() << errorMsg; | ||
| return; | ||
| } |
There was a problem hiding this comment.
On invalid JSON-like input, the handler only logs a warning and returns, leaving the editor with text that looks accepted but is not applied to the underlying item/model. This is easy to miss for users. Consider providing in-UI feedback (e.g., dialog/toast/inline error state) and/or reverting the line edit back to the last valid value so the UI and stored value can’t silently diverge.
| connect(widget, &DLineEdit::editingFinished, widget, [this, widget](){ | ||
| QString errorMsg; | ||
| if (!validateTextInput(widget->text(), errorMsg)) { | ||
| qWarning() << errorMsg; |
There was a problem hiding this comment.
When validation fails, this path just logs a warning and returns, but editingFinished has already fired so the user receives no actionable feedback and the displayed text may not match the persisted value. Consider surfacing the error in the UI (dialog/inline error) and/or restoring the previous value and returning focus to the field to prompt correction.
| qWarning() << errorMsg; | |
| qWarning() << errorMsg; | |
| const QString message = errorMsg.isEmpty() | |
| ? tr("The entered value is invalid.") | |
| : errorMsg; | |
| QMessageBox::warning(widget, | |
| tr("Invalid input"), | |
| message); | |
| widget->setFocus(); | |
| widget->selectAll(); |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, pengfeixx The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
/merge |
Fix dconfig input text not conforming to JSON standards
Log: Fix dconfig input text not conforming to JSON standards
pms: BUG-317315
Summary by Sourcery
Validate dconfig text inputs that look like JSON before converting or applying them to prevent invalid JSON structures from being processed.
Bug Fixes:
Enhancements: