🐛 (api.py): Raise correct error when timeout #45

2023-08-10T11:59:54+02:00

Zerka30 commented

2023-08-10 11:59:54 +02:00

(Migrated from github.com)

Related to : https://github.com/lucasheld/uptime-kuma-api/issues/44

I'll let you try it for yourself, and let me know if this function has any dependencies elsewhere.

Related to : https://github.com/lucasheld/uptime-kuma-api/issues/44 I'll let you try it for yourself, and let me know if this function has any dependencies elsewhere.

lucasheld commented

2023-08-12 17:19:42 +02:00

(Migrated from github.com)

I think that is not quite correct.
The Uptime Kuma errors are in r.get("msg"). Now they are no longer intercepted, because an Expection is not thrown in this case.

I would do it like this:

def _call(self, event, data=None) -> Any:
    try:
        r = self.sio.call(event, data, timeout=self.timeout)
    except socketio.exceptions.TimeoutError:
        raise Timeout(f"Timed out while waiting for event {event}")
    if isinstance(r, dict) and "ok" in r:
        if not r["ok"]:
            raise UptimeKumaException(r.get("msg"))
        r.pop("ok")
    return r

I think that is not quite correct. The Uptime Kuma errors are in r.get("msg"). Now they are no longer intercepted, because an Expection is not thrown in this case. I would do it like this: ```python def _call(self, event, data=None) -> Any: try: r = self.sio.call(event, data, timeout=self.timeout) except socketio.exceptions.TimeoutError: raise Timeout(f"Timed out while waiting for event {event}") if isinstance(r, dict) and "ok" in r: if not r["ok"]: raise UptimeKumaException(r.get("msg")) r.pop("ok") return r ```

Zerka30 commented

2023-08-16 10:46:35 +02:00

(Migrated from github.com)

Hum 🤔 , wdym exactly?

Because the way I've done it, it will return the correct error if socket.io returns a TimeoutError, and for any other type of error it will return an UptimeKumaException with the error message contained in r.get("msg")

Hum :thinking: , wdym exactly? Because the way I've done it, it will return the correct error if socket.io returns a `TimeoutError`, and for any other type of error it will return an `UptimeKumaException` with the error message contained in `r.get("msg")`

Zerka30 commented

2023-08-25 12:06:39 +02:00

(Migrated from github.com)

@lucasheld , I ping you, to correct if necessary

lucasheld (Migrated from github.com) reviewed 2023-08-29 23:03:36 +02:00

uptime_kuma_api/api.py

lucasheld (Migrated from github.com) commented

2023-08-29 23:03:24 +02:00

r is not defined when self.sio.call fails.

`r` is not defined when `self.sio.call` fails.

uptime_kuma_api/api.py

					
				@ -503,1 +503,3 @@

				        r = self.sio.call(event, data, timeout=self.timeout)

				        try:

				            r = self.sio.call(event, data, timeout=self.timeout)

				        except socketio.exceptions.TimeoutError:

lucasheld (Migrated from github.com) commented

2023-08-29 23:03:11 +02:00

The response of self.sio.call is not always a dict (for example api.need_setup). In this case r.pop("ok") fails. You have deleted the code that checks if the response type is a dict.

Also you have deleted the code that checks the r["ok"] value. The UptimeKumaException is now no longer raised when Uptime Kuma responds with an error message (when r["ok"] is false).

The response of `self.sio.call` is not always a dict (for example `api.need_setup`). In this case `r.pop("ok")` fails. You have deleted the code that checks if the response type is a dict. Also you have deleted the code that checks the `r["ok"]` value. The UptimeKumaException is now no longer raised when Uptime Kuma responds with an error message (when `r["ok"]` is false).

lucasheld commented

2023-08-29 23:11:58 +02:00

(Migrated from github.com)

I think that is not quite correct. The Uptime Kuma errors are in r.get("msg"). Now they are no longer intercepted, because an Expection is not thrown in this case.

I would do it like this:

def _call(self, event, data=None) -> Any:
    try:
        r = self.sio.call(event, data, timeout=self.timeout)
    except socketio.exceptions.TimeoutError:
        raise Timeout(f"Timed out while waiting for event {event}")
    if isinstance(r, dict) and "ok" in r:
        if not r["ok"]:
            raise UptimeKumaException(r.get("msg"))
        r.pop("ok")
    return r

I have tested this code. It works as expected.

The only problem I noticed is that there are still exceptions directly from socketio (for example socketio.exceptions.ConnectionError).
Then these Excepetions should either be caught too or we have to use separate timeout exceptions, one for the logic within uptime-kuma-api and one for socketio timeouts. Currently, I prefer the latter.

> I think that is not quite correct. The Uptime Kuma errors are in r.get("msg"). Now they are no longer intercepted, because an Expection is not thrown in this case. > > I would do it like this: > > ```python > def _call(self, event, data=None) -> Any: > try: > r = self.sio.call(event, data, timeout=self.timeout) > except socketio.exceptions.TimeoutError: > raise Timeout(f"Timed out while waiting for event {event}") > if isinstance(r, dict) and "ok" in r: > if not r["ok"]: > raise UptimeKumaException(r.get("msg")) > r.pop("ok") > return r > ``` I have tested this code. It works as expected. The only problem I noticed is that there are still exceptions directly from socketio (for example `socketio.exceptions.ConnectionError`). Then these Excepetions should either be caught too or we have to use separate timeout exceptions, one for the logic within uptime-kuma-api and one for socketio timeouts. Currently, I prefer the latter.

👍 1

Zerka30 commented

2023-08-31 12:15:21 +02:00

(Migrated from github.com)

Alright, I understand and now its fix ! :D

This pull request can be merged automatically.

You are not authorized to merge this pull request.

View command line instructions

Checkout

From your project repository, check out a new branch and test the changes.

git fetch -u origin Zerka30/return-timeout:Zerka30/return-timeout

git checkout Zerka30/return-timeout

Merge

Merge the changes and update on Forgejo.

Warning: The "Autodetect manual merge" setting is not enabled for this repository, you will have to mark this pull request as manually merged afterwards.

git checkout master

git merge --no-ff Zerka30/return-timeout

git checkout Zerka30/return-timeout

git rebase master

git checkout master

git merge --ff-only Zerka30/return-timeout

git checkout Zerka30/return-timeout

git rebase master

git checkout master

git merge --no-ff Zerka30/return-timeout

git checkout master

git merge --squash Zerka30/return-timeout