plone / plone.restapi

RESTful API for Plone.
http://plonerestapi.readthedocs.org/
84 stars 75 forks source link

plone.restapi vs Plone 6.0.3 #1611

Closed ale-rt closed 1 year ago

ale-rt commented 1 year ago

I am moving an application that uses plone.restapi to Plone 6.0.3. I found out that some call fail with the error:

[ale@flo ~]$ curl 'http://localhost:9020/Plone/@calendar_events?start=2023-02-26&end=2023-04-09'   ...
{
  "message": "request parameters and body",
  "type": "NotImplementedError"
}

This seems related to this PR:

That already caused an issue in Plone that was fixed in:

That PR adds the following lines:

        elif url_qs and content_type != "application/x-www-form-urlencoded":
            raise NotImplementedError("request parameters and body")

The problem is that the content_type using plone.restapi is application/json.

Is anybody else having the same issue? Am I using plone.restapi wrong?

davisagli commented 1 year ago

@ale-rt The thing I don't understand about this is: why is Zope throwing an error about having both a querystring and a body when you didn't send a body?

ale-rt commented 1 year ago

I did not get it as well and did not dig into the issue enough to grok it :)

I am temporarily using this branch to make the site work: https://github.com/zopefoundation/Zope/pull/1117

1letter commented 1 year ago

Is anybody else having the same issue? Am I using plone.restapi wrong?

Yes, today the same happend here.

1letter commented 1 year ago

@ale-rt i will check the workaround tomorrow. I have upgraded today a Plone Site from 6.0.0.3 to 6.0.3 and see the error in a custom Rest-API endpoint call. It's a simple GET Request with a parameter.

1letter commented 1 year ago

@ale-rt I can confirm, that the patch helps. Thanks.

ale-rt commented 1 year ago

I will try to dig deeper and make the patch merged.

1letter commented 1 year ago

@ale-rt if you need help, let me know

ale-rt commented 1 year ago

I dug in to the issue and came up with a different patch that I think is more comprehensive.

https://github.com/zopefoundation/Zope/pull/1117

The problem is that the current code does not actually check there is a body in the request.

ale-rt commented 1 year ago

@1letter did you by chance has the issue while using pat-calendar? It seems it is mistakenly adding the Content-Type header here: https://github.com/Patternslib/Patterns/blob/7e391d33ed84bf8461d7c0c04f80ec157c23e549/src/pat/calendar/calendar.js#L311-L318 I will probably transfer this issue there.

1letter commented 1 year ago

@ale-rt no, it was a custom rest-api endpoint call. It's a simple GET Request with a parameter. but the structure is similar like you linked.

1letter commented 1 year ago

@ale-rt with your patch in zope the following test doesn't fail in my Project :

def test_querystringsearch_metadata_fields_get_request_as_anon(self):
    from urllib.parse import quote
    import json

    self.api_session = RelativeSession(self.portal_url, test=self)
    self.api_session.headers.update({"Accept": "application/json"})
    self.api_session.headers.update({"Content-Type": "application/json"})

    data = {
        "query": [
            {
                "i": "portal_type",
                "o": "plone.app.querystring.operation.selection.is",
                "v": ["News Item"],
            }
        ],
        "metadata_fields": ["image_scales", "getIcon"],
    }
    response = self.api_session.get(
        "/@querystring-search",
        proxies=self._proxies,
        params={"query": quote(json.dumps(data))},
    )

    self.assertEqual(response.status_code, 200)
    self.assertEqual(response.headers["X-Cache-Rule"], "plone.content.dynamic")
    self.assertEqual(
        response.headers["X-Cache-Operation"], "plone.app.caching.terseCaching"
    )

Note: The Project use the master branch of plone.restapi with a fix for the GET Request in Endpoint @querystring-search, but it'S not related to these issue

Update: I will check, what happens when i removing the content-type header

ale-rt commented 1 year ago

As you noted, the reason why your test fails should be this line:

self.api_session.headers.update({"Content-Type": "application/json"})

You should not need it (see https://github.com/zopefoundation/Zope/pull/1117#pullrequestreview-1419901927).

1letter commented 1 year ago

@ale-rt You were correct, the useless header with an empty body in the GET request was the problem.

ale-rt commented 1 year ago

I am closing this one because I think the details are now clear: you shall not send a Content-Type header on a GET request :) I opened a PR for pat-calendar: https://github.com/Patternslib/Patterns/pull/1157