Meilisearch is sending a body on GET requests #762

dtomlinson91 · 2023-05-19T19:10:37Z

Description
Meilisearch is sending a body on a HTTP GET when using the client.

Meilisearch is running on GKE using image getmeili/meilisearch:v1.1.1. The instance is brand new with an empty index addresses.

import json
import meilisearch


MEILISEARCH_URL = "https://***"
MEILISEARCH_API_KEY = "***"


def main():
    client = meilisearch.Client(url=MEILISEARCH_URL, api_key=MEILISEARCH_API_KEY)
    resp = client.get_all_stats()
    print(json.dumps(resp))

Expected behavior
Meilisearch should send an empty body for a HTTP GET and return:

{
  "databaseSize": 192512,
  "lastUpdate": "2023-05-19T18:31:15.009505693Z",
  "indexes": {
    "addresses": {
      "numberOfDocuments": 0,
      "isIndexing": false,
      "fieldDistribution": {}
    }
  }
}

Current behavior
Meilisearch sits behind an external GCP HTTPS load balancer. The load balancer is opionated and rejects the request with a 400:

Load balancer log entry:

{
  "@type": "type.googleapis.com/google.cloud.loadbalancing.type.LoadBalancerLogEntry",
  "remoteIp": "***",
  "statusDetails": "body_not_allowed"
}

Response from request:

<html>
   <head>
      <meta http-equiv="content-type" content="text/html;charset=utf-8">
      <title>400 Bad Request</title>
   </head>
   <body text=#000000 bgcolor=#ffffff>
      <h1>Error: Bad Request</h1>
      <h2>Your client has issued a malformed or illegal request.</h2>
      <h2></h2>
   </body>
</html>

Screenshots or Logs
Debugging locally you can see there is a body being sent of 'null':

Looking through previous issues it looks like #586 attempted to fix this, but it's still sending a body of null.

Changing

meilisearch-python/meilisearch/_httprequests.py

Line 48 in f788922

data=json.dumps(body) if body else "" if body == "" else "null",

to:

                    data=json.dumps(body) if body else "" if body == "" else None,

Sends a body of None:

and results in a successful request.

I'm happy to a raise a PR unless sending a body of 'null' is intentional?

The text was updated successfully, but these errors were encountered:

sanders41 · 2023-05-19T19:17:18Z

I don't think this is intentional, and I know why it is happening. I'm verifying now that changing it won't break anything.

If I find fixing it doesn't break anything and you want the PR I can report back with what change is needed. I also don't mind updating it if you don't have time. Either way is good for me.

sanders41 · 2023-05-19T19:43:58Z

Alright, what we need to do is update send_requests to:

    def send_request(
        self,
        http_method: Callable,
        path: str,
        body: Optional[Union[Dict[str, Any], List[Dict[str, Any]], List[str], str]] = None,
        content_type: Optional[str] = None,
    ) -> Any:
        if content_type:
            self.headers["Content-Type"] = content_type
        try:
            request_path = self.config.url + "/" + path
            if not body:
                request = http_method(
                    request_path,
                    timeout=self.config.timeout,
                    headers=self.headers,
                )
            elif isinstance(body, bytes):
                request = http_method(
                    request_path,
                    timeout=self.config.timeout,
                    headers=self.headers,
                    data=body,
                )
            else:
                request = http_method(
                    request_path,
                    timeout=self.config.timeout,
                    headers=self.headers,
                    data=json.dumps(body) if body else "" if body == "" else "null",
                )
            return self.__validate(request)

        except requests.exceptions.Timeout as err:
            raise MeilisearchTimeoutError(str(err)) from err
        except requests.exceptions.ConnectionError as err:
            raise MeilisearchCommunicationError(str(err)) from err

@alallema doing this breaks several tests, but in ever case the test contains a type error sending None to an update settings method when it is not Optional. What I think we should do it change all of these tests to use the reset method instead because this is what the update is trying to do. This would need to be updated for displayed attributes, distinct_attribute, filterable attributes, ranking rules, searchable attributes, and sortable attributes

Taking displayed attributes as an example, the fix is to change:

-    response = index.update_displayed_attributes(None)
+    response = index.reset_displayed_attributes()

sanders41 · 2023-05-20T00:37:36Z

I just had another thought. I haven't tested they yet, but we can probably also simplify data=json.dumps(body) if body else "" if body == "" else "null", to data=json.dumps(body) with this change also.

sanders41 · 2023-05-20T10:27:06Z

I made a slight tweak to allow things like [] and {} in the body to be sent. I can't think of a time you could want to, but it will serialize to json so I suppose there could be a valid use case.

    def send_request(
        self,
        http_method: Callable,
        path: str,
        body: Optional[Union[Dict[str, Any], List[Dict[str, Any]], List[str], str]] = None,
        content_type: Optional[str] = None,
    ) -> Any:
        if content_type:
            self.headers["Content-Type"] = content_type
        try:
            request_path = self.config.url + "/" + path
            if body is None or body == "":
                request = http_method(
                    request_path,
                    timeout=self.config.timeout,
                    headers=self.headers,
                )
            elif isinstance(body, bytes):
                request = http_method(
                    request_path,
                    timeout=self.config.timeout,
                    headers=self.headers,
                    data=body,
                )
            else:
                request = http_method(
                    request_path,
                    timeout=self.config.timeout,
                    headers=self.headers,
                    data=json.dumps(body),
                )
            return self.__validate(request)

        except requests.exceptions.Timeout as err:
            raise MeilisearchTimeoutError(str(err)) from err
        except requests.exceptions.ConnectionError as err:
            raise MeilisearchCommunicationError(str(err)) from err

@dtomlinson91 if you want a PR feel fee to take it, if not I can do it.

dtomlinson91 · 2023-05-20T15:20:21Z

I made a slight tweak to allow things like [] and {} in the body to be sent. I can't think of a time you could want to, but it will serialize to json so I suppose there could be a valid use case.

    def send_request(
        self,
        http_method: Callable,
        path: str,
        body: Optional[Union[Dict[str, Any], List[Dict[str, Any]], List[str], str]] = None,
        content_type: Optional[str] = None,
    ) -> Any:
        if content_type:
            self.headers["Content-Type"] = content_type
        try:
            request_path = self.config.url + "/" + path
            if body is None or body == "":
                request = http_method(
                    request_path,
                    timeout=self.config.timeout,
                    headers=self.headers,
                )
            elif isinstance(body, bytes):
                request = http_method(
                    request_path,
                    timeout=self.config.timeout,
                    headers=self.headers,
                    data=body,
                )
            else:
                request = http_method(
                    request_path,
                    timeout=self.config.timeout,
                    headers=self.headers,
                    data=json.dumps(body),
                )
            return self.__validate(request)

        except requests.exceptions.Timeout as err:
            raise MeilisearchTimeoutError(str(err)) from err
        except requests.exceptions.ConnectionError as err:
            raise MeilisearchCommunicationError(str(err)) from err

@dtomlinson91 if you want a PR feel fee to take it, if not I can do it.

Happy for you to do it, you have done most of the work figuring out what needs changing after all 😁

alallema · 2023-05-23T11:52:15Z

Thanks @sanders41 for replying and your PR.

I don't think this is intentional,

Indeed this is not the case.

@alallema doing this breaks several tests, but in ever case the test contains a type error sending None to an update settings method when it is not Optional. What I think we should do it change all of these tests to use the reset method instead because this is what the update is trying to do. This would need to be updated for displayed attributes, distinct_attribute, filterable attributes, ranking rules, searchable attributes, and sortable attributes

The issue is that some of these methods: distinct_attribute, ranking rules, searchable attributes, and sortable attributes should be able to reset their parameters by sending null. That's why these tests have been created update_setting_with_none. So I'm afraid it will be a breaking change if we change this behavior, and I don't find a way to fix this without breaking it do you?

sanders41 · 2023-05-23T11:58:09Z

We can inspect the http_method for it's name, if http_method.__name__ == "get". The problem is this breaks the mock tests. Let me see if I can think of a solution for that.

sanders41 · 2023-05-23T12:03:19Z

One more thing, if I do find a solution what about the type errors? None of those methods I mentioned allow None to be sent.

sanders41 · 2023-05-23T12:36:00Z

I solved the mock test issue with inspecting the __name__. So if you like the new implementation the only outstanding question is the type issue. But that would have been there all along so it isn't something new.

alallema · 2023-05-23T13:08:31Z

None of those methods I mentioned allow None to be sent.

You are so right 😕 ... Sorry to have made you change everything and thank you to find a good solution.
But it bugs me not to be able to give the user the possibility to use this option if the API allows it. Can we just change the signature of the method by something like:

def update_ranking_rules(self, body: Union[List[str], None]) -> TaskInfo:

?

sanders41 · 2023-05-23T13:12:52Z

Yes, changing the signature like you mentioned would fix the problem. Would you rather me add it to the PR I have open or do it in a separate one since it is a different issue? I'm good either way.

alallema · 2023-05-23T13:22:19Z

@sanders41, if this is not a problem, two ERPs should be better since as you mentioned they are two different problems.
Thank you so much 😊

765: Allow None for the body when updating settings r=alallema a=sanders41 # Pull Request ## Related issue Fixes the `None` typing issue discussed [here](#762 (comment)) ## What does this PR do? - Allows `None` to be sent in the `body` parameter for settings updates. ## PR checklist Please check if your PR fulfills the following requirements: - [x] Does this PR fix an existing issue, or have you listed the changes applied in the PR description (and why they are needed)? - [x] Have you read the contributing guidelines? - [x] Have you made sure that the title is accurate and descriptive of the changes? Thank you so much for contributing to Meilisearch! Co-authored-by: Paul Sanders <[email protected]> Co-authored-by: Amélie <[email protected]>

sanders41 mentioned this issue May 20, 2023

Don't send body for get requests #763

Merged

3 tasks

sanders41 added the bug Something isn't working label May 20, 2023

sanders41 mentioned this issue May 23, 2023

Allow None for the body when updating settings #765

Merged

3 tasks

meili-bors bot closed this as completed in 5d51424 May 23, 2023

sanders41 mentioned this issue Oct 24, 2023

Support user dictionary loading #870

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Meilisearch is sending a body on GET requests #762

Meilisearch is sending a body on GET requests #762

dtomlinson91 commented May 19, 2023

sanders41 commented May 19, 2023

sanders41 commented May 19, 2023

sanders41 commented May 20, 2023

sanders41 commented May 20, 2023

dtomlinson91 commented May 20, 2023

alallema commented May 23, 2023

sanders41 commented May 23, 2023

sanders41 commented May 23, 2023

sanders41 commented May 23, 2023

alallema commented May 23, 2023

sanders41 commented May 23, 2023

alallema commented May 23, 2023

Meilisearch is sending a body on GET requests #762

Meilisearch is sending a body on GET requests #762

Comments

dtomlinson91 commented May 19, 2023

sanders41 commented May 19, 2023

sanders41 commented May 19, 2023

sanders41 commented May 20, 2023

sanders41 commented May 20, 2023

dtomlinson91 commented May 20, 2023

alallema commented May 23, 2023

sanders41 commented May 23, 2023

sanders41 commented May 23, 2023

sanders41 commented May 23, 2023

alallema commented May 23, 2023

sanders41 commented May 23, 2023

alallema commented May 23, 2023