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

Safe qt importer #804

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

Conversation

fdan
Copy link

@fdan fdan commented Feb 11, 2021

If the tank authentication is used in a headless mode, it will raise an unhandled exception as shown below.

Using the get method to retrieve modules in qt_importer.py will avoid the index error and is also just simpler.

  File "/foobar/pipeline/config/studio/install/core/python/sgtk/__init__.py", line 16, in <module>
    import tank
  File "/foobar/pipeline/config/studio/install/core/python/tank/__init__.py", line 110, in <module>
    from . import authentication
  File "/foobar/pipeline/config/studio/install/core/python/tank/authentication/__init__.py", line 35, in <module>
    from .shotgun_authenticator import ShotgunAuthenticator
  File "/foobar/pipeline/config/studio/install/core/python/tank/authentication/shotgun_authenticator.py", line 13, in <module>
    from .sso_saml2 import has_sso_info_in_cookies, has_unified_login_flow_info_in_cookies
  File "/foobar/pipeline/config/studio/install/core/python/tank/authentication/sso_saml2/__init__.py", line 15, in <module>
    from .core.errors import (  # noqa
  File "/foobar/pipeline/config/studio/install/core/python/tank/authentication/sso_saml2/core/__init__.py", line 15, in <module>
    from .sso_saml2_core import SsoSaml2Core  # noqa
  File "/foobar/pipeline/config/studio/install/core/python/tank/authentication/sso_saml2/core/sso_saml2_core.py", line 46, in <module>
    from .username_password_dialog import UsernamePasswordDialog
  File "/foobar/pipeline/config/studio/install/core/python/tank/authentication/sso_saml2/core/username_password_dialog.py", line 27, in <module>
    from ...ui.qt_abstraction import QtCore, QtGui
  File "/foobar/pipeline/config/studio/install/core/python/tank/authentication/ui/qt_abstraction.py", line 22, in <module>
    QtWebEngineWidgets = _importer.QtWebEngineWidgets
  File "/foobar/pipeline/config/studio/install/core/python/tank/util/qt_importer.py", line 89, in QtWebEngineWidgets
    return self._modules["QtWebEngineWidgets"]```

@@ -55,35 +55,35 @@ def QtCore(self):
"""
:returns: QtCore module, if available.
"""
return self._modules["QtCore"] if self._modules else None
return self._modules.get("QtCore")
Copy link

Choose a reason for hiding this comment

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

The previous syntax implies self._modules may not be set when called so you may need this instead:

Suggested change
return self._modules.get("QtCore")
return self._modules.get("QtCore") if self._modules else None

This covers QtCore or the module not being present in the dict and the dict itself not being set.

Copy link
Author

Choose a reason for hiding this comment

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

If the _modules dict is initialised to an empty dict, get should still be safe, i.e.

>>> d = {}
>>> print(d.get('foobar'))
None

If the issue is covering a scenario where _modules is not an attr of the object at all, I don't think we'd need to do that as it's in the constructor?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer the suggestion by @halil3d above, instead of initializing self._modules in the constructor to an empty dictionary, because for a couple of minor reasons:

  1. If _modules is None, this clearly indicates that the import failed for the requested Qt interface. An empty dict may suggest a different error during the import.
  2. The modules property returns the the value of _modules - so we would be changing the return value from None to {} if the import failed. Which means if callers are checking specifically for None, this would break their code.

@staceyoue staceyoue self-requested a review September 26, 2022 20:02
Copy link
Contributor

@staceyoue staceyoue left a comment

Choose a reason for hiding this comment

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

LGTM - just one minor comment.

@@ -55,35 +55,35 @@ def QtCore(self):
"""
:returns: QtCore module, if available.
"""
return self._modules["QtCore"] if self._modules else None
return self._modules.get("QtCore")
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer the suggestion by @halil3d above, instead of initializing self._modules in the constructor to an empty dictionary, because for a couple of minor reasons:

  1. If _modules is None, this clearly indicates that the import failed for the requested Qt interface. An empty dict may suggest a different error during the import.
  2. The modules property returns the the value of _modules - so we would be changing the return value from None to {} if the import failed. Which means if callers are checking specifically for None, this would break their code.

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