Fix: #676 Handle text/plain response content type correctly #677
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
|
|
|
Please feel free to add a screenshot. It's a good practice to have screenshots when you're updating the UI part. |
|
Got it! I'm locking in on it now and will have it fixed very soon! |
|
hi @KariHall619 , you can see also the following usage. It can close the related issue once this PR is merged.
|
LinuxSuRen
requested changes
May 15, 2025
console/atest-ui/src/views/net.ts
Outdated
|
|
||
| const contentType = response.headers.get('Content-Type') || ''; | ||
| if (contentType.startsWith('text/plain')) { | ||
| return response.text(); |
There was a problem hiding this comment.
Actually it might always return in JSON format. The response is from atest itself instead of the target API. For example, when sending an HTTP GET request of https://gitee.com/linuxsuren/api-testing/raw/master/README.md, you need to check the response of this.
Fix parsing and displaying of text/plain responses by: - Improving text handling in DefaultResponseProcess function (net.ts) - Adding safe JSON parsing in TestCase.vue with fallback to text display
KariHall619
changed the title
- Add explicit error logging with console.debug in catch blocks - Add detailed comments explaining intended exception handling - Fix "Handle this exception or don't catch it at all" maintainability warnings - Clarify that non-JSON responses are expected and properly handled
LinuxSuRen
reviewed
May 15, 2025
console/atest-ui/src/views/net.ts
Outdated
| const message = await response.json().then((data: any) => data.message) | ||
| throw new Error(message) | ||
| } catch { | ||
| const text = await response.text() |
There was a problem hiding this comment.
The code will always run to here due to line 37.
LinuxSuRen
reviewed
May 15, 2025
console/atest-ui/src/views/net.ts
Outdated
| } | ||
|
|
||
| // Get the complete response text first | ||
| const responseText = await response.text(); |
There was a problem hiding this comment.
You can get the content type, such as:
const contentType = response.headers.get('content-type') || '';
LinuxSuRen
reviewed
May 15, 2025
| // We intentionally display as plain text instead of attempting JSON parsing | ||
| console.debug("Response body is not valid JSON, displaying as plain text:", error); | ||
| testResult.value.bodyText = e.body; | ||
| testResult.value.bodyObject = null; |
There was a problem hiding this comment.
Type 'null' is not assignable to type '{}'.ts-plugin(2322)
There was a problem hiding this comment.
You can check the content type, then handle JSON or not.
Refactored `DefaultResponseProcess` to intelligently handle HTTP responses:
1. Explicitly checks the 'Content-Type' header.
2. If 'text/plain', directly processes the response as text using `response.text()`.
3. For other content types, attempts to parse as JSON using `response.json()`.
4. If JSON parsing fails (e.g., the content is not valid JSON despite the
header, or it's another non-JSON type like XML), the response is
cloned (`response.clone()`) before reading its body as text. This
correctly handles scenarios where the initial `response.json()` call
has already consumed the response body stream.
This approach ensures the response body is consumed only once per effective read attempt (once for text, or once for JSON attempt + once for text fallback on a cloned response if needed), resolving concerns about potential multiple reads on the original response stream and improving robustness in parsing diverse response types.
Replaced logical OR (||) with nullish coalescing operator (??)
for fetching the 'Content-Type' header in `DefaultResponseProcess`.
This change makes the assignment safer, as `??` only falls back
to the default value (empty string) if `response.headers.get('Content-Type')`
is strictly `null` or `undefined`, aligning with modern JavaScript best practices.
Refined error handling in `DefaultResponseProcess` (net.ts):
- For non-OK HTTP responses:
- Robustly extracts error messages by attempting to parse the
response body first as JSON, then as text, using `response.clone()`
before each attempt to prevent stream consumption issues.
- Logs intermediate parsing failures to `console.debug`.
- Throws a more informative error message.
- For OK HTTP responses:
- When falling back to text after a `response.json()` failure,
enhanced `console.debug` logging and comments to clarify
that this is intentional content negotiation.
These changes aim to satisfy static analysis tools regarding
unhandled exceptions by making error handling more explicit,
logging relevant information, and clarifying intent in catch blocks.
|
LinuxSuRen
approved these changes
May 25, 2025
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.
#676
Fix text/plain response handling
Issue
When sending a request with Content-Type text/plain;charset=UTF-8, the tool incorrectly attempts to parse the response as JSON, resulting in parsing errors for non-JSON content like "测试".
Fix
Modified
DefaultResponseProcessfunction to check the Content-Type header and handle text/plain responses appropriately:response.text()instead ofresponse.json()response.json()Testing
Verified that GET requests to http://localhost:8080/api/hello/message?key=testMessage with text/plain responses now display correctly without JSON parsing errors.