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

feat(urls): Add error logging for provider imports / Remove provider import exception handling #3536

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

Antvirf
Copy link

@Antvirf Antvirf commented Nov 27, 2023

Background

At the moment, failing to import a provider and set up its URLs fails silently, leading to confusing error messages when certain paths aren't found:

NoReverseMatch at /login/
Reverse for 'saml_login' not found. 'saml_login' is not a valid view function or pattern name.

Most recently, I encountered this with the SAML provider as it currently relies on libxmlsec - the correct installation of which on an ARM Mac is difficult at the moment. It took me a long time to figure out that my URL paths weren't working due to an import issue, as I would have expected the library to tell me if something is wrong. Someone else had also encountered something similar with openssl previously.

Solution

Remove the try/except so that the import error surfaces immediately.

Output example

In my case, with a broken libxmlsec install, the output when starting the server is:

.... # traceback
ImportError: dlopen(/Users/MYUSER/MYPROJECT/venv/lib/python3.11/site-packages/xmlsec.cpython-311-darwin.so, 0x0002): symbol not found in flat namespace '_xmlSecDSigNs'

Submitting Pull Requests checklist

  • Make sure you use semantic commit messages.
    Examples: "fix(google): Fixed foobar bug", "feat(accounts): Added foobar feature".
  • All Python code must formatted using Black, and clean from pep8 and isort issues.

@Antvirf
Copy link
Author

Antvirf commented Nov 28, 2023

@pennersr would be great to get your thoughts - do you want to keep catching these exceptions as of current, or would you prefer just throwing the exception?

@pennersr
Copy link
Owner

Except for backwards compatibility, there is no good reason to catch those import errors. I've also bumped into a few occasions where catching the ImportError was silently hiding a different issue. So I would prefer to drop catching import errors.

@coveralls
Copy link

coveralls commented Nov 29, 2023

Coverage Status

coverage: 95.757% (+0.01%) from 95.745%
when pulling fe5798d on Antvirf:fix/url-import-error-handling
into 04cf83d on pennersr:main.

@Antvirf Antvirf force-pushed the fix/url-import-error-handling branch from 9180ec1 to fe5798d Compare November 30, 2023 00:12
@Antvirf
Copy link
Author

Antvirf commented Nov 30, 2023

Except for backwards compatibility, there is no good reason to catch those import errors. I've also bumped into a few occasions where catching the ImportError was silently hiding a different issue. So I would prefer to drop catching import errors.

Understood, changed accordingly to just remove the try/catch.

@Antvirf Antvirf changed the title feat(urls): Add error logging for provider imports feat(urls): Add error logging for provider imports / Remove provider import exception handling Nov 30, 2023
@pennersr pennersr merged commit f784cdf into pennersr:main Nov 30, 2023
12 of 33 checks passed
@Antvirf Antvirf deleted the fix/url-import-error-handling branch December 1, 2023 00:44
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.

3 participants