fix: add JSON validation for text input fields #136
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
1. Added isValidTextJsonValue helper function to validate JSON strings 2. Integrated JSON validation in three locations: main editor window, OEM dialog, and CLI set command 3. Prevent invalid JSON input from being processed by returning early with warning messages 4. Maintain existing functionality for valid JSON inputs Log: Fixed issue where invalid JSON could be entered in text fields Influence: 1. Test entering valid JSON in text fields - should work normally 2. Test entering invalid JSON in text fields - should be rejected with warning 3. Verify CLI set command rejects invalid JSON with appropriate error message 4. Check that valid JSON with various structures (objects, arrays, strings) is accepted 5. Test edge cases like empty strings, malformed JSON syntax 6. Ensure existing functionality for non-text data types remains unaffected fix: 为文本输入字段添加JSON验证 1. 新增isValidTextJsonValue辅助函数用于验证JSON字符串 2. 在三个位置集成了JSON验证：主编辑器窗口、OEM对话框和CLI设置命令 3. 通过提前返回和警告消息防止处理无效的JSON输入 4. 保持有效JSON输入的现有功能 Log: 修复了文本字段可能输入无效JSON的问题 Influence: 1. 测试在文本字段中输入有效JSON - 应正常工作 2. 测试在文本字段中输入无效JSON - 应被拒绝并显示警告 3. 验证CLI设置命令拒绝无效JSON并显示适当的错误消息 4. 检查各种结构（对象、数组、字符串）的有效JSON是否被接受 5. 测试边缘情况，如空字符串、格式错误的JSON语法 6. 确保非文本数据类型的现有功能不受影响 PMS: BUG-317315 Change-Id: Ifaf792b26b4bcb6c3c950e4ca2b0aad249c9ae4e
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdds centralized JSON validation for text-based configuration inputs and integrates it into the editor UI and CLI set command to prevent invalid JSON from being converted and stored. Sequence diagram for JSON validation in main editor text fieldsequenceDiagram
actor User
participant KeyContent
participant DLineEdit
participant Helper
User->>DLineEdit: inputText
User->>DLineEdit: editingFinished
DLineEdit->>KeyContent: editingFinished signal
KeyContent->>DLineEdit: lambda invoked
DLineEdit->>Helper: isValidTextJsonValue(text)
Helper-->>DLineEdit: bool isValid
alt invalid json
DLineEdit->>DLineEdit: qWarning invalid json value
DLineEdit->>DLineEdit: return without emitting valueChanged
else valid json
DLineEdit->>KeyContent: clearFocus
DLineEdit->>KeyContent: emit valueChanged(stringToQVariant(text))
end
Sequence diagram for JSON validation in CLI set commandsequenceDiagram
actor User
participant CommandManager
participant Helper
participant ConfigManager
User->>CommandManager: setCommand
CommandManager->>CommandManager: parse arguments
alt type is double
CommandManager->>ConfigManager: setValue(key, value.toDouble())
ConfigManager-->>CommandManager: result
CommandManager-->>User: exit 0
else type is text
CommandManager->>Helper: isValidTextJsonValue(value)
Helper-->>CommandManager: bool isValid
alt invalid json
CommandManager->>User: outpuSTDError not a valid json text
CommandManager-->>User: exit 1
else valid json
CommandManager->>ConfigManager: setValue(key, stringToQVariant(value))
ConfigManager-->>CommandManager: result
CommandManager-->>User: exit 0
end
end
Updated class diagram for JSON validation helper integrationclassDiagram
class HelperFunctions {
+static QVariant stringToQVariant(QString s)
+static bool isValidTextJsonValue(QString s)
+static QString qvariantToCmd(QVariant v)
}
class KeyContent {
+void setBaseInfo(ConfigGetter getter, QString language)
+void valueChanged(QVariant value)
}
class OEMDialog {
+QWidget getItemWidget(ConfigGetter getter, DStandardItem item)
+void treeItemChanged(DStandardItem item)
}
class CommandManager {
+int setCommand()
+void outpuSTDError(QString message)
}
class DLineEdit {
+QString text()
+void setText(QString text)
+void setEnabled(bool enabled)
+void clearFocus()
}
class ConfigManager {
+void setValue(QString key, QVariant value)
}
HelperFunctions <.. KeyContent : uses
HelperFunctions <.. OEMDialog : uses
HelperFunctions <.. CommandManager : uses
DLineEdit <.. KeyContent : uses
DLineEdit <.. OEMDialog : uses
ConfigManager <.. CommandManager : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
deepin pr auto review代码审查报告整体评估这段代码添加了一个 代码审查1.
|
![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 the UI handlers you only log invalid JSON with qWarning and then return, which means the user sees no feedback and the field keeps showing the invalid text but doesn’t apply; consider either resetting to the last valid value or showing an inline error/tooltip so users understand why their change was ignored.
- The new isValidTextJsonValue helper treats only strictly valid JSON as acceptable (e.g., unquoted strings and empty input will be rejected); confirm this matches the intended semantics for these text fields, and if not, adjust the helper to allow those cases or normalize them (e.g., auto-wrapping plain text in quotes) before validation.
- You now have JSON validation in several call sites plus stringToQVariant likely doing its own parsing; consider centralizing the parse-and-convert logic in one helper that can return an error state, so you avoid double-parsing and ensure consistent behavior between the GUI and CLI paths.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the UI handlers you only log invalid JSON with qWarning and then return, which means the user sees no feedback and the field keeps showing the invalid text but doesn’t apply; consider either resetting to the last valid value or showing an inline error/tooltip so users understand why their change was ignored.
- The new isValidTextJsonValue helper treats only strictly valid JSON as acceptable (e.g., unquoted strings and empty input will be rejected); confirm this matches the intended semantics for these text fields, and if not, adjust the helper to allow those cases or normalize them (e.g., auto-wrapping plain text in quotes) before validation.
- You now have JSON validation in several call sites plus stringToQVariant likely doing its own parsing; consider centralizing the parse-and-convert logic in one helper that can return an error state, so you avoid double-parsing and ensure consistent behavior between the GUI and CLI paths.
## Individual Comments
### Comment 1
<location> `dconfig-center/dde-dconfig-editor/mainwindow.cpp:724-726` </location>
<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;
+ }
</code_context>
<issue_to_address>
**suggestion:** Invalid JSON is only logged, with no in-UI feedback or rollback of the edited value.
Since you `return` after logging, the invalid JSON leaves the edit text and focus unchanged and never emits `valueChanged`, so from the user’s perspective the change just “does nothing.” If the goal is to reject bad input, please also provide visible feedback (e.g., revert to last valid value or show an error/invalid state) so the failure isn’t silent in the UI.
Suggested implementation:
```cpp
auto widget = new DLineEdit(this);
widget->setEnabled(canWrite);
// track last valid value for rollback on invalid input
widget->setProperty("lastValidText", widget->text());
connect(widget, &DLineEdit::editingFinished, widget, [this, widget](){
if (!isValidTextJsonValue(widget->text())) {
qWarning() << "invalid json value" << widget->text();
// revert to last valid value and select it to provide visible feedback
const QString lastValidText = widget->property("lastValidText").toString();
widget->setText(lastValidText);
widget->selectAll();
return;
}
// update last valid value on successful edit
widget->setProperty("lastValidText", widget->text());
widget->clearFocus();
emit valueChanged(stringToQVariant(widget->text()));
});
```
If the widget’s initial value is set elsewhere (e.g., `widget->setText(initialValue);`), it would be best to also call `widget->setProperty("lastValidText", widget->text());` immediately after that, so the first invalid edit correctly rolls back to the loaded value instead of an empty string.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
724
to
+726
| connect(widget, &DLineEdit::editingFinished, widget, [this, widget](){ | ||
| if (!isValidTextJsonValue(widget->text())) { | ||
| qWarning() << "invalid json value" << widget->text(); |
There was a problem hiding this comment.
suggestion: Invalid JSON is only logged, with no in-UI feedback or rollback of the edited value.
Since you return after logging, the invalid JSON leaves the edit text and focus unchanged and never emits valueChanged, so from the user’s perspective the change just “does nothing.” If the goal is to reject bad input, please also provide visible feedback (e.g., revert to last valid value or show an error/invalid state) so the failure isn’t silent in the UI.
Suggested implementation:
auto widget = new DLineEdit(this);
widget->setEnabled(canWrite);
// track last valid value for rollback on invalid input
widget->setProperty("lastValidText", widget->text());
connect(widget, &DLineEdit::editingFinished, widget, [this, widget](){
if (!isValidTextJsonValue(widget->text())) {
qWarning() << "invalid json value" << widget->text();
// revert to last valid value and select it to provide visible feedback
const QString lastValidText = widget->property("lastValidText").toString();
widget->setText(lastValidText);
widget->selectAll();
return;
}
// update last valid value on successful edit
widget->setProperty("lastValidText", widget->text());
widget->clearFocus();
emit valueChanged(stringToQVariant(widget->text()));
});
If the widget’s initial value is set elsewhere (e.g., widget->setText(initialValue);), it would be best to also call widget->setProperty("lastValidText", widget->text()); immediately after that, so the first invalid edit correctly rolls back to the loaded value instead of an empty string.
BLumia
approved these changes
Jan 26, 2026
mhduiy
approved these changes
Jan 26, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, BLumia, mhduiy 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 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
OEM dialog, and CLI set command
with warning messages
Log: Fixed issue where invalid JSON could be entered in text fields
Influence:
warning
message
strings) is accepted
unaffected
fix: 为文本输入字段添加JSON验证
Log: 修复了文本字段可能输入无效JSON的问题
Influence:
PMS: BUG-317315
Change-Id: Ifaf792b26b4bcb6c3c950e4ca2b0aad249c9ae4e
Summary by Sourcery
Add JSON validation for text-based configuration inputs to prevent processing invalid JSON values.
Bug Fixes: