Skip to content
This repository has been archived by the owner on Apr 26, 2024. It is now read-only.

Indicate what endpoint came back with a JSON response we were unable to parse #14097

Merged

Conversation

MadLittleMods
Copy link
Contributor

@MadLittleMods MadLittleMods commented Oct 7, 2022

Indicate what endpoint came back with a JSON response we were unable to parse.

Before:

WARNING - POST-11 - Unable to parse JSON: Expecting value: line 1 column 1 (char 0) (b'')

After:

WARNING - POST-11 - Unable to parse JSON from POST /_matrix/client/v3/join/%21ZlmJtelqFroDRJYZaq:hs1?server_name=hs1 response: Expecting value: line 1 column 1 (char 0) (b'')

It's possible to figure out which endpoint these warnings were coming from before but you had to follow the request ID POST-11 to the log line that says Completed request [...]. Including this key information next to the JSON parsing error makes it much easier to reason whether it matters or not.

2022-09-29T08:23:25.7875506Z synapse_main | 2022-09-29 08:21:10,336 - synapse.http.matrixfederationclient - 299 - INFO - POST-11 - {GET-O-13} [hs1] Completed request: 200 OK in 0.53 secs, got 450 bytes - GET matrix://hs1/_matrix/federation/v1/make_join/%21ohtKoQiXlPePSycXwp%3Ahs1/%40charlie%3Ahs2?ver=1&ver=2&ver=3&ver=4&ver=5&ver=6&ver=org.matrix.msc2176&ver=7&ver=8&ver=9&ver=org.matrix.msc3787&ver=10&ver=org.matrix.msc2716v4

As a note, having no body is normal for the /join endpoint and it can handle it (this was followed up in #14600)

try:
content = parse_json_object_from_request(request)
except Exception:
# Turns out we used to ignore the body entirely, and some clients
# cheekily send invalid bodies.
content = {}

Alternatively we could remove these extra logs but they are probably more usually helpful to figure out what went wrong.

Pull Request Checklist

  • Pull request is based on the develop branch
  • Pull request includes a changelog file. The entry should:
    • Be a short description of your change which makes sense to users. "Fixed a bug that prevented receiving messages from other servers." instead of "Moved X method from EventStore to EventWorkerStore.".
    • Use markdown where necessary, mostly for code blocks.
    • End with either a period (.) or an exclamation mark (!).
    • Start with a capital letter.
    • Feel free to credit yourself, by adding a sentence "Contributed by @github_username." or "Contributed by [Your Name]." to the end of the entry.
  • Pull request includes a sign off
  • Code style is correct
    (run the linters)

Before:
```
WARNING - POST-11 - Unable to parse JSON: Expecting value: line 1 column 1 (char 0) (b'')
```

After:
```
WARNING - POST-2 - Unable to parse JSON from POST /_matrix/client/v3/join/%21ZlmJtelqFroDRJYZaq:hs1?server_name=hs1 response: Expecting value: line 1 column 1 (char 0) (b'')
```
@MadLittleMods MadLittleMods changed the title Indicate what endpoint the JSON response came from Indicate what endpoint came back with a invalid JSON response Oct 7, 2022
@MadLittleMods MadLittleMods added the T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks. label Oct 7, 2022
@MadLittleMods MadLittleMods changed the title Indicate what endpoint came back with a invalid JSON response Indicate what endpoint came back with a JSON response we were unable to parse Oct 7, 2022
MadLittleMods added a commit that referenced this pull request Oct 7, 2022
@MadLittleMods MadLittleMods marked this pull request as ready for review October 7, 2022 07:56
@MadLittleMods MadLittleMods requested a review from a team as a code owner October 7, 2022 07:56
Copy link
Contributor

@DMRobertson DMRobertson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes please!

@MadLittleMods MadLittleMods merged commit 1bf2832 into develop Oct 7, 2022
@MadLittleMods MadLittleMods deleted the madlittlemods/attribute-json-parse-errors-to-endpoint branch October 7, 2022 16:39
@MadLittleMods
Copy link
Contributor Author

Thanks for the review @DMRobertson 🐕‍🦺

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, removal, replacement, enabling or disabling functionality, other engineering tasks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants