Repository: opensearch-project/opensearch-py
Pull request #810 (2024-08-25 12:24:14)

Fixed timeout argument in Transport
### Description
Fixed bug https://github.com/opensearch-project/opensearch-py/issues/809

### Issues Resolved
- Timeout argument in Transport.perform_request is not used, because it is redefined just below


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check [here](https://github.com/opensearch-project/OpenSearch/blob/main/CONTRIBUTING.md#developer-certificate-of-origin).


borisevich-a-v: Sorry, I cannot satisfy DCO due to my account settings. But PR is signed off correctly
borisevich-a-v: > Add a test please?
Sure, I'll add

> I am not sure what the issue with DCO is, why can’t it be fixed with your account?
Hope it will not be a problem. I have two Github accounts. And for this one I have secondary email address for a while. 


borisevich-a-v: Added test and also fix the same problem with ignore, if you don't mind
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/opensearch-project/opensearch-py/pull/810?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 70.29%. Comparing base [(`ba715b9`)](https://app.codecov.io/gh/opensearch-project/opensearch-py/commit/ba715b9d3f0609d25bfb0210b4b52234601e4dd8?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project) to head [(`d91a556`)](https://app.codecov.io/gh/opensearch-project/opensearch-py/commit/d91a5560fa6e5f04edb4e1e1b511828453d2a38b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project).
> Report is 56 commits behind head on main.

<details><summary>Additional details and impacted files</summary>


```diff
@@            Coverage Diff             @@
##             main     #810      +/-   ##
==========================================
- Coverage   71.95%   70.29%   -1.67%     
==========================================
  Files          91      113      +22     
  Lines        8001     8866     +865     
==========================================
+ Hits         5757     6232     +475     
- Misses       2244     2634     +390     
```

</details>

[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/opensearch-project/opensearch-py/pull/810?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project).   
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=opensearch-project).

borisevich-a-v: @dblock 
Added tests, fixed AsyncTransport as well
Tried to add code in the same code style, if there is any mistakes let me know
dblock: > > I am not sure what the issue with DCO is, why can’t it be fixed with your account?
> 
> Hope it will not be a problem. I have two GitHub accounts due to job requirements. And for this one I have secondary email address for a while.

I am not supposed to approve PRs without passing DCO :) Let's try to fix it.

You can configure the email address per repo. I do this routinely on a work computer for anything non-work.

```
git config user.name ...
git config user.email ...
```

Does this work?
borisevich-a-v: > Does this work?

The problem was with my emails. I have satisfied DCO :)

Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 0a604fb3..672fd065 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
 ### Deprecated
 ### Removed
 ### Fixed
+- Fix `Transport.perform_request`'s arguments `timeout` and `ignore` variable usage ([810](https://github.com/opensearch-project/opensearch-py/pull/810))
 ### Security
 
 ### Dependencies
diff --git a/opensearchpy/_async/transport.py b/opensearchpy/_async/transport.py
index bc9e257f..2d631ee7 100644
--- a/opensearchpy/_async/transport.py
+++ b/opensearchpy/_async/transport.py
@@ -371,11 +371,13 @@ class AsyncTransport(Transport):
             underlying :class:`~opensearchpy.Connection` class for serialization
         :arg body: body of the request, will be serialized using serializer and
             passed to the connection
+        :arg timeout: timeout of the request. If it is not presented as argument
+            will be extracted from `params`
         """
         await self._async_call()
 
         method, params, body, ignore, timeout = self._resolve_request_args(
-            method, params, body
+            method, params, body, ignore, timeout
         )
 
         for attempt in range(self.max_retries + 1):
diff --git a/opensearchpy/transport.py b/opensearchpy/transport.py
index 2385b2b8..559d0387 100644
--- a/opensearchpy/transport.py
+++ b/opensearchpy/transport.py
@@ -404,9 +404,11 @@ class Transport:
             underlying :class:`~opensearchpy.Connection` class for serialization
         :arg body: body of the request, will be serialized using serializer and
             passed to the connection
+        :arg timeout: timeout of the request. If it is not presented as argument
+            will be extracted from `params`
         """
         method, params, body, ignore, timeout = self._resolve_request_args(
-            method, params, body
+            method, params, body, ignore, timeout
         )
 
         for attempt in range(self.max_retries + 1):
@@ -473,7 +475,14 @@ class Transport:
         """
         return self.connection_pool.close()
 
-    def _resolve_request_args(self, method: str, params: Any, body: Any) -> Any:
+    def _resolve_request_args(
+        self,
+        method: str,
+        params: Any,
+        body: Any,
+        ignore: Collection[int],
+        timeout: Optional[Union[int, float]],
+    ) -> Any:
         """Resolves parameters for .perform_request()"""
         if body is not None:
             body = self.serializer.dumps(body)
@@ -498,13 +507,13 @@ class Transport:
                 # bytes/str - no need to re-encode
                 pass
 
-        ignore = ()
-        timeout = None
         if params:
-            timeout = params.pop("request_timeout", None)
             if not timeout:
-                timeout = params.pop("timeout", None)
-            ignore = params.pop("ignore", ())
+                timeout = params.pop("request_timeout", None) or params.pop(
+                    "timeout", None
+                )
+            if not ignore:
+                ignore = params.pop("ignore", ())
             if isinstance(ignore, int):
                 ignore = (ignore,)
 


Test patch:
diff --git a/test_opensearchpy/test_async/test_transport.py b/test_opensearchpy/test_async/test_transport.py
index e3048a48..51388ddd 100644
--- a/test_opensearchpy/test_async/test_transport.py
+++ b/test_opensearchpy/test_async/test_transport.py
@@ -175,6 +175,20 @@ class TestTransport:
             "headers": {"x-opaque-id": "request-1"},
         } == t.get_connection().calls[1][1]
 
+    async def test_perform_request_all_arguments_passed_correctly(self) -> None:
+        t: Any = AsyncTransport([{}], connection_class=DummyConnection)
+        method, url, params, body = "GET", "/test_url", {"params": "test"}, "test_body"
+        timeout, ignore, headers = 11, ("ignore",), {"h": "test"}
+
+        await t.perform_request(method, url, params, body, timeout, ignore, headers)
+
+        assert t.get_connection().calls == [
+            (
+                (method, url, params, body.encode()),
+                {"headers": headers, "ignore": ignore, "timeout": timeout},
+            )
+        ]
+
     async def test_request_with_custom_user_agent_header(self) -> None:
         t: Any = AsyncTransport([{}], connection_class=DummyConnection)
 
diff --git a/test_opensearchpy/test_transport.py b/test_opensearchpy/test_transport.py
index ccef21df..96422a3f 100644
--- a/test_opensearchpy/test_transport.py
+++ b/test_opensearchpy/test_transport.py
@@ -260,6 +260,23 @@ class TestTransport(TestCase):
             "http://google.com:1234", t.connection_pool.connections[1].host
         )
 
+    def test_perform_request_all_arguments_passed_correctly(self) -> None:
+        t: Any = Transport([{}], connection_class=DummyConnection)
+        method, url, params, body = "GET", "/test_url", {"params": "test"}, "test_body"
+        timeout, ignore, headers = 11, ("ignore",), {"h": "test"}
+
+        t.perform_request(method, url, params, body, timeout, ignore, headers)
+
+        self.assertEqual(
+            t.get_connection().calls,
+            [
+                (
+                    (method, url, params, body.encode()),
+                    {"headers": headers, "ignore": ignore, "timeout": timeout},
+                )
+            ],
+        )
+
     def test_request_will_fail_after_x_retries(self) -> None:
         t: Any = Transport(
             [{"exception": ConnectionError(None, "abandon ship", Exception())}],