revert JSON validation #141
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
Reviewer's GuideReverts prior changes to JSON string handling and validation logic while keeping the updated JSON validation behavior, re-enables previously skipped user-data removal tests, adjusts a subpath used in tests, wraps non-numeric command output in quotes, and drops an obsolete helper unit test file from the test build. Class diagram for helper functions and CommandManager usageclassDiagram
class Helper {
+static QString qvariantToString(QVariant v)
+static QString qvariantToStringCompact(QVariant v)
+static QVariant stringToQVariant(QString s)
+static bool isValidTextJsonValue(QString s)
}
class CommandManager {
+int getCommand()
}
CommandManager ..> Helper : uses
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, you mix rawsandtrimmed(parsing usess, structural checks usetrimmed); consider consistently using the trimmed version for both parsing and the{/[prefix check to avoid subtle whitespace-dependent behavior. - Wrapping
qvariantToString(result)in extra quotes inCommandManager::getCommandmay double-encode JSON values (objects/arrays/numbers) while also changing how plain strings are represented; it may be safer to only add quotes when the underlying QVariant is a plain string and otherwise preserve the original JSON/text representation.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `isValidTextJsonValue`, you mix raw `s` and `trimmed` (parsing uses `s`, structural checks use `trimmed`); consider consistently using the trimmed version for both parsing and the `{`/`[` prefix check to avoid subtle whitespace-dependent behavior.
- Wrapping `qvariantToString(result)` in extra quotes in `CommandManager::getCommand` may double-encode JSON values (objects/arrays/numbers) while also changing how plain strings are represented; it may be safer to only add quotes when the underlying QVariant is a plain string and otherwise preserve the original JSON/text representation.
## Individual Comments
### Comment 1
<location> `dconfig-center/dde-dconfig/main.cpp:229` </location>
<code_context>
outpuSTD(QString::number(result.toDouble()));
} else {
- outpuSTD(qvariantToString(result));
+ outpuSTD(QString("\"%1\"").arg(qvariantToString(result)));
}
} else if (method == "name") {
</code_context>
<issue_to_address>
**issue (bug_risk):** Wrapping `qvariantToString` in quotes without escaping introduces malformed output for strings containing quotes or newlines.
With the new behavior:
- `qvariantToString(result)` now returns the raw string (no JSON-style escaping).
- This call wraps that raw value in `"%1"` without escaping.
So for a value like `foo"bar`, the output becomes:
```text
"foo"bar"
```
which is not a valid single quoted string and will break consumers expecting JSON-like or shell-safe output. This is a regression from the previous implementation, where `QJsonValue::fromVariant` handled quoting/escaping.
If the intent is JSON-like output, the escaping/quoting should live in one place only, e.g.:
- Make `qvariantToString` return a proper JSON string literal for string variants, and call it directly here, or
- Add a helper dedicated to producing correctly escaped/quoted CLI output instead of wrapping the raw string locally.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| outpuSTD(QString::number(result.toDouble())); | ||
| } else { | ||
| outpuSTD(qvariantToString(result)); | ||
| outpuSTD(QString("\"%1\"").arg(qvariantToString(result))); |
There was a problem hiding this comment.
issue (bug_risk): Wrapping qvariantToString in quotes without escaping introduces malformed output for strings containing quotes or newlines.
With the new behavior:
qvariantToString(result)now returns the raw string (no JSON-style escaping).- This call wraps that raw value in
"%1"without escaping.
So for a value like foo"bar, the output becomes:
"foo"bar"
which is not a valid single quoted string and will break consumers expecting JSON-like or shell-safe output. This is a regression from the previous implementation, where QJsonValue::fromVariant handled quoting/escaping.
If the intent is JSON-like output, the escaping/quoting should live in one place only, e.g.:
- Make
qvariantToStringreturn a proper JSON string literal for string variants, and call it directly here, or - Add a helper dedicated to producing correctly escaped/quoted CLI output instead of wrapping the raw string locally.
This reverts commit 528403c.
mhduiy
approved these changes
Feb 10, 2026
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: 18202781743, 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 |
deepin pr auto review代码审查报告总体概述这段代码主要修改了配置中心(dconfig-center)中关于JSON值处理和验证的逻辑，简化了部分函数实现，移除了对JSON值的严格验证，并调整了相关测试用例。 详细审查意见1. 语法和逻辑问题1.1
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Summary by Sourcery
Adjust JSON value validation and string conversion behavior while updating related CLI output and tests.
Bug Fixes:
Enhancements:
Build:
Tests: