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.

Sign up for GitHub

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

Jump to bottom

Exit synadm early on fatal errors, refactor & fix query() #168

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open

Exit synadm early on fatal errors, refactor & fix query() #168

wants to merge 22 commits into from

Conversation

JOJ0
Copy link
Owner

@JOJ0 JOJ0 commented Jan 24, 2025 •
edited
Loading

Addresses general error handling issues discussed in: #164

  • What actually is a fatal error where it does not make sense to continue? I picked a few already. There might be more. Please suggest!
  • Is it really a good idea to define log_fatal_exit() directly in this module as a regular function? Should it live inside ApiRequest? Or move it to a "utils-like" place? Will we use it outside this module too?
  • Some refactoring along the way
  • Add type hints (we can discuss if this is the right place and time....)
  • Fix docstring
    • arguments done
    • return value: FIXME, isn't this entirely wrong? resp.json() returns lists or dicts. Or what does json string mean in this context? It's no string in reality....

@JOJ0 JOJ0 marked this pull request as draft January 24, 2025 06:40
@JOJ0 JOJ0 requested a review from JacksonChen666 January 24, 2025 07:41
@JacksonChen666 JacksonChen666 linked an issue Jan 24, 2025 that may be closed by this pull request
room power-levels should fail gracefully when Synapse not reachable #164
Open
@JOJ0 JOJ0 marked this pull request as ready for review January 24, 2025 08:34
@JOJ0 JOJ0 changed the title Exit synadm early on fatal errors Exit synadm early on fatal errors, refactor & fix query() Jan 24, 2025
@JacksonChen666
Copy link
Collaborator

  • What actually is a fatal error where it does not make sense to continue? I picked a few already. There might be more. Please suggest!

I guess check for anything that logs errors and exits. Searching for raise SystemExit is a good starting point.

  • Is it really a good idea to define log_fatal_exit() directly in this module as a regular function? Should it live inside ApiRequest? Or move it to a "utils-like" place? Will we use it outside this module too?

Maybe do move it to some utils place (tying into the last point)

  • return value: FIXME, isn't this entirely wrong? resp.json() returns lists or dicts. Or what does json string mean in this context? It's no string in reality....

JSONs can also be strings, numbers, booleans, nulls, and the usually expected lists and dicts. In most (or even all) cases, it's dicts and possibly lists. So it can be kind of anything, but for specific API methods, we might as well just infer from the example output in Synapse Admin API docs, and for the query function, just JSON decoded I guess (noting that other types are possible too).

JacksonChen666
JacksonChen666 reviewed Jan 29, 2025
View reviewed changes
Copy link
Collaborator

@JacksonChen666 JacksonChen666 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looked at it, made some commits. Hope to get some feedback on some of my commits, if any.

JOJ0
JOJ0 commented Jan 31, 2025
View reviewed changes
synadm/api.py Outdated Show resolved Hide resolved
synadm/api.py Show resolved Hide resolved
synadm/api.py Outdated Show resolved Hide resolved
synadm/api.py Outdated Show resolved Hide resolved
JOJ0 and others added 20 commits January 31, 2025 11:07
@JOJ0 @JacksonChen666
Exit synadm on fatal requests exceptions
4183d86
@JOJ0 @JacksonChen666
Sort api module imports with isort
ce1e977
@JOJ0 @JacksonChen666
Refactor query args handling more pythonic
18b4991
@JOJ0 @JacksonChen666
Refactor query kwargs handling more pythonic
a66f894
@JOJ0 @JacksonChen666
Move query args before keyword args to please linter
dd2c2ec
@JOJ0 @JacksonChen666
Add query function type hints, return only
2a06f91
@JOJ0 @JacksonChen666
Add query function type hints, arguments only
8fc2a70
@JOJ0 @JacksonChen666
Fix query function docstring (arguments only)
c5093f7 
- Add missing arguments.
- Remove (types) since we have type hints now.
- Reword here and there.
@JacksonChen666
fix ConnectionError not being caught
5524e71
@JacksonChen666
add optional messages to log_fatal_exit
11c1cff
@JacksonChen666
more detailed message for connection error
0f9bcd0
@JacksonChen666
don't catch InvalidURL or MissingSchema exceptions
4f8a9f8 
if these happen, it's pretty much a bug in our code, and so having the
full stacktrace would help (if the user doesn't provide the command)
@JacksonChen666
print traceback upon log_fatal_exit
e1324c2
@JacksonChen666
separate message from error logging
0f42b30 
make it more clear
@JacksonChen666
add args docs
67f3b9f
@JacksonChen666 @JOJ0
Apply suggestions from code review
0a6ab35 
Co-authored-by: J0J0 Todos <[email protected]>
@JacksonChen666 @JOJ0
Apply another suggestion
dcade53 
Co-authored-by: J0J0 Todos <[email protected]>
@JacksonChen666
reword format_exception string join part
7df42c9
@JacksonChen666
fix typo
5f76e98
@JacksonChen666
rewrite return section of docs for query api
252d599
JOJ0
JOJ0 commented Jan 31, 2025
View reviewed changes
synadm/api.py Show resolved Hide resolved
JOJ0 added 2 commits January 31, 2025 18:28
@JOJ0
Explicitely catch requests ConnectionError
4bb093d 
and don't overwrite built-in ConnectionError with import statement.
@JOJ0
Sort imports again with isort
8fa1503
@JOJ0
Copy link
Owner Author

JOJ0 commented Jan 31, 2025

Good to go? @JacksonChen666

@JOJ0
Copy link
Owner Author

JOJ0 commented Jan 31, 2025 •
edited
Loading

Good to go? @JacksonChen666

Ah moving to utils place......but actually: We don't have a place for it. I'd rather not invent one now. We do that if we ever need it elsewhere. Agreed?

(we only have _helper.py and _common.py in package cli which is not imported here, so...)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JacksonChen666 JacksonChen666 JacksonChen666 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

room power-levels should fail gracefully when Synapse not reachable
2 participants
@JOJ0 @JacksonChen666