Parse json validation

[TrevisGordan]

This pull request introduces a new servlet function named parse_json, aimed at simplifying and standardizing the process of parsing JSON objects from query parameters. This function complements our existing utility functions such as parse_integer and parse_string, offering a unified approach to parameter parsing across our application.

The necessity for parse_json arises from the requirement to handle JSON formatted data in query parameters (in filterparameters, on room endpoints), which, lacked a dedicated parsing mechanism. By incorporating parse_json, we streamline the parsing process and also enforce stricter validation of input data, ensuring that only valid JSON objects are processed, preventing internal server errors. This is achieved through the inclusion of an INVALID_PARAM error response, which is triggered when the parsed data fails to meet the JSON object criteria. The error message explicitly states that the parameter "must be a valid JSON object," thereby providing clear feedback for troubleshooting.

Key Features:

• Unified JSON Parsing: Harmonizes with existing parsing functions, offering a consistent parsing approach.
• Enhanced Validation: Incorporates error handling for non-compliant JSON input, improving parameter validation. This should fix JSON Filter Parameter Validation Error on Room Event Endpoint #16922.

Implementation Snippet:

Below is an example snippet demonstrating the usage of parse_json to parse a JSON object from the filter query parameter:

filter_json = parse_json(request, "filter", encoding="utf-8")

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.
• Code style is correct
  (run the linters)

[clokep]

I think the original long term plan here was to replace the JSON parsing with Pydantic (see #13147), but this doesn't seem to preclude that at all.

[TrevisGordan]

Yes, I've also seen that thanks. Exactly. Coming from FastAPI, I'm a big fan of Pydantic and would advocate for its use in model validation. This, though, is meant for a step before—checking if the input is actually JSON, particularly in the case of query parameters. From what I've seen, this is mostly used for the filtering functionality. I even already thought about it as a point for consideration on "where to go from here" for the next steps. I found a filter validation function (jsonschema) def check_valid_filter(self, user_filter_json: JsonDict) -> None: but not utilized on the endpoints—to further ensure that in the case of valid JSON, it is a valid filter. This could be uniformly done with Pydantic.

[clokep]

Makes sense! I don't think there are other spots that JSON is passed as a query parameter, it is a rather odd choice.

[anoadragon453]

Overall looks great, thank you for putting this together! I have a smattering of wording changes, and a request for tests.

[anoadragon453]

Generally we try to keep changelog entries short, and acknowledge that the audience is system administrators. Such an audience won't care to know the details of the implementation, but rather than user-facing impact. My suggestion would be:

Suggested change

	Adds parse_json servlet function for standardized JSON parsing from query parameters, ensuring enhanced data validation and error handling.
	Introduces INVALID_PARAM error response for invalid JSON objects, improving parameter validation feedback.
	Adds validation check to prevent 500 internal server error on invalid Json Filter request.
	Return `400 M_INVALID_PARAM` upon receiving invalid JSON in query parameters across various client and admin endpoints, rather than an internal server error.

[TrevisGordan]

Thanks for the Insights! - I'll keep that in mind. 

[anoadragon453]

Are you happy to accept my suggestion here? I prefer the suggested version of the changelog for the reasons in my initial comment.

[anoadragon453]

LGTM otherwise!

[anoadragon453]

Generally you should only precede a variable name with an underscore in Python if you'd like to label the output of a function, but not actually use it. Here we are using _valid_filter_str, so it should not have a leading underscore.

Could you remove it, along with _invalid_filter_str and from other tests please?

[TrevisGordan]

Ahh took me a bit! But now I think I know what you mean.
You are referring to the use of a single underscore, _, as a throwaway variable, commonly used for temporary or insignificant values, as in:

for _ in range(32):
    print('Hello, World.')

this woulde be correct. But in this case, following PEP 8, _single_leading_underscore signals internal use, here serving as a minor internal string helper. It marks variables that are temporary or specific to this test's context, distinguishing between main test logic and setup details. However, I'm also more than happy to adjust this for you too! ;) Always wanted to cite a PEP tho. 😉

[anoadragon453]

Thanks for citing the PEP! It's interesting to read where this convention came from.

In Synapse, we certainly do use leading underscores for internal function/method names and private class variables (self._internal_var). This is to signal to code external to classes that they shouldn't try to access this field (it is internal).

However, we don't use this convention for local variable names - so at least for this codebase, I would remove the leading underscore.

[anoadragon453]

Huge apologies for taking so long to get back to this - I've only now been getting back to reviewing Synapse PRs. I have only a couple small things below, but otherwise this looks good to go.

Thanks for your patience.

[anoadragon453]

Thanks for citing the PEP! It's interesting to read where this convention came from.

In Synapse, we certainly do use leading underscores for internal function/method names and private class variables (self._internal_var). This is to signal to code external to classes that they shouldn't try to access this field (it is internal).

However, we don't use this convention for local variable names - so at least for this codebase, I would remove the leading underscore.

[anoadragon453]

Are you happy to accept my suggestion here? I prefer the suggested version of the changelog for the reasons in my initial comment.

[TrevisGordan]

@anoadragon453 Ah no worries! Thanks for coming back to it.
I made the requested changes. :)
I updated the new test cases accordingly for "NOT_JSON"

I re-ran the linters.
I re-ran the tests (passed)
I also ran systests (passed)

[anoadragon453]

LGTM, thank you very much!

[TrevisGordan]

✅ 🎉 - @anoadragon453 Please don't forget to merge before we lose sync to upstream. :)

[anoadragon453]

Oops, apparently forgot to press the button. Thanks for the poke!