Fix a bug where JSON content is not always validated #804
Conversation
Motivation:
If an invalid JSON content is submitted to Central Dogma Server using
REST API, the JSON is not validated and it is saved as is.
The file content is formatted as a text so `node.get("content")` returns
`TextNode` and it is regarded as a string although it is a JSON object.
https://github.com/line/centraldogma/blob/9030505a928006d32f3897dc08823dac0d5fd6c2/server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/ChangesRequestConverter.java#L91-L95
Modifications:
- Pass a raw text value to `Change.ofJsonUpsert()` and make an invalidid
JSON validated.
Result:
Central Dogma server now correctly rejects an invalid JSON content.
|
/cc @rainy789 This will fix the bug where the invalid JSON content is pushed that we talked about before. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #804 +/- ##
============================================
+ Coverage 65.60% 65.68% +0.08%
- Complexity 3317 3323 +6
============================================
Files 355 355
Lines 13875 13878 +3
Branches 1498 1499 +1
============================================
+ Hits 9102 9116 +14
+ Misses 3918 3911 -7
+ Partials 855 851 -4
... and 5 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java
Outdated
Show resolved
Hide resolved
server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java
Outdated
Show resolved
Hide resolved
…/api/ContentServiceV1Test.java
…/api/ContentServiceV1Test.java
trustin
changed the title
| '{' + | ||
| " \"path\" : \"/string.json\"," + | ||
| " \"type\" : \"UPSERT_JSON\"," + | ||
| " \"content\" : \"json string\"," + |
There was a problem hiding this comment.
nit: for reviewers
| " \"content\" : \"json string\"," + | |
| " \"content\" : \"json string\"," + /* This string is not a JSON string but a plain text. */ |
| return Change.ofJsonUpsert(path, node.get("content")); | ||
| if (content.isTextual()) { | ||
| // A content can be a serialized JSON so the text value needs to be parsed as JSON. | ||
| return Change.ofJsonUpsert(path, content.textValue()); |
There was a problem hiding this comment.
Question: Can we apply this approach that checks if it's textual in the Change.ofJsonUpsert method?
There was a problem hiding this comment.
"string" is a valid JSON type. So I don't think we can generally apply it to Change.ofJsonUpsert().
This is a special case where a web application sends a stringfied JSON content after editing to the server.
server/src/test/java/com/linecorp/centraldogma/server/internal/api/ContentServiceV1Test.java
Outdated
Show resolved
Hide resolved
…/api/ContentServiceV1Test.java Co-authored-by: minux <[email protected]>
…date-json-content
|
This PR is still WIP. Changed the milestone to the next one. |
Motivation:
If an invalid JSON content is submitted to Central Dogma Server using REST API, the JSON is not validated and it is saved as is.
The file content is formatted as a text so
node.get("content")returnsTextNodeand it is regarded as a string although it is a JSON object.centraldogma/server/src/main/java/com/linecorp/centraldogma/server/internal/api/converter/ChangesRequestConverter.java
Lines 91 to 95 in 9030505
Modifications:
Change.ofJsonUpsert()and make the JSON validated.Result:
Central Dogma server now correctly rejects an invalid JSON content.