Uninett / zinolib

Python library for zino
Apache License 2.0
1 stars 3 forks source link

Raise NotConnectedError in UpdateHandler.check_connection #71

Closed hmpf closed 4 days ago

hmpf commented 5 days ago

… instead of AttributeError when something nasty has happened to the session object.

For first part of #68

codecov[bot] commented 5 days ago

Codecov Report

Attention: Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.

Project coverage is 70.25%. Comparing base (f13d7ad) to head (614f019).

Files Patch % Lines
src/zinolib/controllers/zino1.py 0.00% 3 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## main #71 +/- ## ========================================== - Coverage 70.35% 70.25% -0.10% ========================================== Files 13 13 Lines 1464 1466 +2 ========================================== Hits 1030 1030 - Misses 434 436 +2 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

hmpf commented 5 days ago

I am not sure it is a sufficient fix for first part of #68. It addresses the traceback specifically yes, but I see several more places in UpdateHandler code where same error might occur (The list is not exhaustive!):

1. `connect()` in UpdateHandler checks EventManager's session **before** calling the now fixed `check_connection`

Fair.

Instead of always returning the session object, connect_push_channel will raise a "NotConnectedError" if the connection could not be made.

2. `update()` and `remove()` in UpdateHandler call EventManager (including manager's methods that don't call `_verify_session`) yet never call `check_connection`

Not applicable.

update calls self.manager.get_updated_event_for_id(event_id) which eventually calls _verify_session before it tries talking on the net.

remove does not need a working connection, it only manipulates the local dict of events and the removed_ids set.

3. more?

Not in UpdateHandler.

Furthermore, while first part in #68 was exposed when calling UpdateHandler, it concerns the session from EventManager. Shouldn't we address the problem in EventManager?

SessionAdapter actually. See new commit.

hmpf commented 4 days ago

Last commit removed.