Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: meaningful error when authentication fails #101

Merged
merged 1 commit into from
Apr 20, 2024

Conversation

maiminhp
Copy link
Contributor

@maiminhp maiminhp commented Apr 17, 2024

Fixes #83

Example error:

$ python groups.py 
Traceback (most recent call last):
  File "/home/<redacted>/Spond/groups.py", line 33, in <module>
    asyncio.run(main())
  File "/usr/lib/python3.10/asyncio/runners.py", line 44, in run
    return loop.run_until_complete(main)
  File "/usr/lib/python3.10/asyncio/base_events.py", line 649, in run_until_complete
    return future.result()
  File "/home/<redacted>/Spond/groups.py", line 14, in main
    groups = await s.get_groups()
  File "/home/<redacted>/Spond/spond/spond.py", line 60, in wrapper
    raise e
  File "/home/<redacted>/Spond/spond/spond.py", line 57, in wrapper
    await self.login()
  File "/home/<redacted>/Spond/spond/spond.py", line 41, in login
    raise AuthenticationError(err_msg)
spond.spond.AuthenticationError: Login failed. Response received: {'errorKey': 'wrongEmailOrPassword', 'message': 'Wrong email or password.'}

@elliot-100 elliot-100 marked this pull request as draft April 20, 2024 12:53
@elliot-100 elliot-100 requested a review from Olen April 20, 2024 13:15
@elliot-100
Copy link
Collaborator

Thanks for this @ptmminh.

I think the ValueError should be a more specific AuthenticationError.

e.g.

class AuthenticationError(Exception)
  pass
...
raise AuthenticationError(err_msg)

I don't know enough about aiohttp to work out the 'Unclosed connector' problem, but can be dealt with separately; this pull request delivers value without it.

@Olen
Copy link
Owner

Olen commented Apr 20, 2024

Since the login now raises an exception, this exception should be caught by the functons that calls the login function.

To close the connector, all you need to do is catch the exception and do someting like

    except AuthenticationError:
        print("Authentication error")
        await s.clientsession.close()
        sys.exit()

@maiminhp
Copy link
Contributor Author

maiminhp commented Apr 20, 2024

Thanks @elliot-100 and @Olen for the review! All very helpful remarks, I've made adjustments based on your suggestions. Now the connector even closes (see example trace stack above as proof).

I also created a decorator to handle the authentication check to avoid duplicated code in each method that calls login.

Also quite new to this repo, I usually rebased interactively to keep main clean and forced push after but happy to follow whatever convention you currently have and what's easiest for you to review.

@maiminhp maiminhp marked this pull request as ready for review April 20, 2024 18:48
@Olen
Copy link
Owner

Olen commented Apr 20, 2024

Great work.
The repo and workflows could probably be improved. It was just my personal little thing for a long time, but with more contributions more structure would be good.

@elliot-100 already made a lot of improvements in this regard, and I am grateful for that.

@Olen
Copy link
Owner

Olen commented Apr 20, 2024

It even passes the new test-script @elliot-100 just added.

@Olen Olen merged commit 1aaf7ff into Olen:main Apr 20, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

When authentication fails, error isn't meaningful and session/connector isn't closed.
3 participants