Add parse_int to handle all cases of BadRequest from ints passed in

[djay]

• Another should be BadRequest found in the wild
• add changelog
• add parse_int util
• use parse_int for navigation also
• improve bad int message

---

📚 Documentation preview 📚: https://plonerestapi--1858.org.readthedocs.build/

[mister-roboto]

@djay thanks for creating this Pull Request and helping to improve Plone!

TL;DR: Finish pushing changes, pass all other checks, then paste a comment:

@jenkins-plone-org please run jobs

To ensure that these changes do not break other parts of Plone, the Plone test suite matrix needs to pass, but it takes 30-60 min. Other CI checks are usually much faster and the Plone Jenkins resources are limited, so when done pushing changes and all other checks pass either start all Jenkins PR jobs yourself, or simply add the comment above in this PR to start all the jobs automatically.

Happy hacking!

[djay]

@davisagli sorry. I changed my mind. it does make the code cleaner.

[djay]

@davisagli one other possible change is that I get a DesearlisationError from parse_body from plugins like collective.honeypot instead of BadRequest. I could change parse_body to raise a BadRequest instead and fix all such plugins... but it felt like changing the api... on the other hand, it's not like in that case they were properly using that exception.
or I could add a param to parse_body of "raise_badrequest=True"?

[djay]

@davisagli or DeserealisationError could inherit from badrequest...

[davisagli]

@djay I think it's fine the way you have it, catching DeserializationError and raising a BadRequest. It's conceivable that some endpoints might want to handle a DeserializationError by falling back to some other source of data instead of returning a 400.

[davisagli]

@jenkins-plone-org please run jobs

[djay]

@davisagli I like the idea of changing the base exception. Because then I don't hanve to get another change into collective.honeypot and who know what other plugins have this problem.

class DeserializationError(BadRequest):
    """An error happened during deserialization of content."""

    def __init__(self, msg):
        self.msg = msg

    def __str__(self):
        return repr(self.msg)


class QueryParsingError(BadRequest):
    """An error happened while parsing a search query."""

[davisagli]

@djay I'd rather not change the base exception. It's tying an exception that's about parsing too closely to one that's about HTTP responses, and they aren't always done together.

[djay]

@davisagli fair enough. from that perspective parse_body is linked to requests so this seems reasonable?

def json_body(request):
    # TODO We should not read the complete request BODY in memory.
    # Once we have fixed this, we can remove the temporary patches.py.
    # See there for background information.
    try:
        data = json.loads(request.get("BODY") or "{}")
    except ValueError:
        raise BadRequest("No JSON object could be decoded")
    if not isinstance(data, dict):
        raise DeserializationError("Malformed body")
    return data

[davisagli]

@djay I don't understand yet why anything needs to change in json_body. You're already handling the DeserializationError it can raise...

[djay]

@davisagli because then this is not needed. I've no idea how long this would take to get approved and other plugins might also have similar problems - collective/collective.honeypot#26

[davisagli]

@djay I can sort of see the argument for making json_body raise BadRequest which means it would return a 400 by default if the developer doesn't explicitly handle it. Let me sleep on it.

[davisagli]

@djay I want to go back to DeserializationError here. Any code that calls json_body should already be handling DeserializationError. I don't want to force existing code to be updated in cases where it actually wants to handle the error and not return a 400 response.

[djay]

@davisagli turns out DeserializationError is a 400 response. But also turns out collective.sentry doesn't filter out 400 errors from 500.
Now I could change collective.sentry to ignore any 400 status but how then do I sift out in sentry the difference between an empty or rubbish request to any endpoint vs a bug? DeserializationError is used all over for all sorts of problems with request. It's for this reason why I'm still thinking it makes sense to differentiate between completely unparsable request vs another kind of bug.
but I take your point other code might be handling this problem but if they are what else are they going to do but say it was a badrequest?
whats your thoughts?

[davisagli]

@djay I'm prioritizing backwards-compatibility here. Currently it's the responsibility of the code which calls json_body to handle DeserializationError and return an appropriate response. It might be okay to change that but I'm not 100% confident. There is code in the restapi PAS plugin which catches DeserializationError and then falls back to authenticating from a different source. There are a lot of results on a github search for DeserializationError.

Now I could change collective.sentry to ignore any 400 status but how then do I sift out in sentry the difference between an empty or rubbish request to any endpoint vs a bug?

You can't, with or without your suggested change here. A rubbish request could be from a bot or from a bug in a legitimate client. I think the server should return a 400 response either way, and it's not worth highlighting that in error monitoring. The legitimate client can report the 400 response in a way that makes it obvious something was wrong.

If this were new code I'd be okay with making json_body raise BadRequest in both cases, but it's existing code that's used in many places, and I'm not going to accept this change now.

[djay]

@davisagli I removed those commits

[davisagli]

@djay Thank you!

[davisagli]

@jenkins-plone-org please run jobs