API for testing whether persistence is available

[chlowell]

It would be helpful to have an API for silently testing whether a persistence method is available, in particular LibsecretPersistence. The best way today is to attempt to instantiate the persistence, which logs an error on Linux when PyGObject isn't installed:

Runtime dependency of PyGObject is missing.
Depends on your Linux distro, you could install it system-wide by something like:
    sudo apt install python3-gi python3-gi-cairo gir1.2-secret-1
If necessary, please refer to PyGObject's doc:
https://pygobject.readthedocs.io/en/latest/getting_started.html

It's a reasonable thing to log but an application developer using msal-extensions through another library may interpret it to mean that mid-tier library requires PyGObject (see e.g. Azure/azure-sdk-for-python/issues/12152 and Azure/azure-sdk-for-python/issues/19857).

[rayluo]

Is this pattern adequate?

[chlowell]

I already do that. The trouble is that although azure-identity speculatively instantiates LibsecretPersistence and anticipates failure, msal-extensions logs this error during the attempt. An application developer unfamiliar with the implementation details of azure-identity may take that error to mean azure-identity requires PyGObject (default logging configuration doesn't output source information).

To be clear, I think it's reasonable to log this message when the caller expects LibsecretPersistence to work. However, a mid-tier library like azure-identity doesn't necessarily expect it to work and can't express that expectation because libsecret.py logs the error on import. So I'm imagining an API like:

if LibsecretPersistence.supported():
    # instantiate LibsecretPersistence because I have reason to believe it might work
else:
    # LibsecretPersistence definitely won't work, don't bother trying it

A reasonable alternative to adding such an API is to just not log the error. You could instead raise an exception with the same message in LibsecretPersistence.__init__. Callers expecting LibsecretPersistence to work would get the message explaining why it didn't while callers anticipating failure would be able to handle it silently.

[rayluo]

Thanks for the explanation! That makes sense. Looks like I would need to refrain from the "log-and-reraise" pattern from now on.

Regarding the solution, I like your alternative proposal better, because it does not need to invent yet another new and non-standardized API. How about this fix in MSAL EX?

try:
    import gi
except ImportError:
    long_message = "Runtime dependency of PyGObject is missing. Yada yada yada..."
    raise ImportError(long_message)

The side effect is the original exception's message and error trace were both lost. This could be considered as a good encapsulation, depends on whom you ask. It won't make much difference in this particular case, though.

[chlowell]

I like the alternative better too. You can preserve the original exception on Python 3 in a 2.7-compatible way:

import six

try:
    import gi
except ImportError as ex:
    message = "Runtime dependency of PyGObject is missing. Yada yada yada..."
    six.raise_from(ex, ImportError(message))

But that's just an FYI and another reason to look forward to dropping 2.7 support. On 2.7 six.raise_from(ex, ...) will simply raise ex, so the caller wouldn't see your message.

I suppose the "raise instead of log" pattern could apply to trial_run() as well.

[jiasli]

Drop Python 2 to utilize Exception Chaining

Being able to utilize Python 3's Exception Chaining raise ... from ... gives us another reason to drop Python 2 support in MSAL (AzureAD/microsoft-authentication-library-for-python#406). Thus, the original error can be accessed as __cause__ of the new error.

Using LibsecretPersistence in Azure CLI

Azure CLI also aims to remove this "catch-and-log" pattern for LibsecretPersistence and let the end user explicitly control whether encryption is enabled (Azure/azure-cli#19506). Including the recommendation in the error message as #92 (comment) does will certainly help Azure CLI handle such scenario.

How Azure CLI wraps exceptions

Azure CLI's error handling framework uses error_msg as the error message and recommendation as the recommendation/solution message:

https://github.com/Azure/azure-cli/blob/3eea7fd773722b7a3c40b8115f29a3bad39b70bc/src/azure-cli-core/azure/cli/core/azclierror.py#L28

class AzCLIError(CLIError):
    """ Base class for all the AzureCLI defined error classes.
    DO NOT raise this error class in your codes. """

    def __init__(self, error_msg, recommendation=None):

There is a feature request to support original_error or use raise ... from ... statement (Azure/azure-cli#16348).

[rayluo]

utilize Python 3's Exception Chaining raise ... from ... gives us another reason to drop Python 2 support

To be precise, Python 3's exception chaining would already happen automatically. Quoted from its document: "Exception chaining happens automatically when an exception is raised inside an except or finally section." The rare time we would consider that raise ... from ... in Python 3 is when trying to explicitly disable exception chaining by using raise ... from None, but that is a rare need.

It is, however, still true that we would eventually migrate to Python 3, in order to have that "automatic exception chaining" even when our code base does NOT contain raise ... from ....

[jiasli]

Per my understanding, there is some difference between implicit/automatic exception chaining and explicit raise ... from ... exception chaining.

This is discussed in more detail in https://www.python.org/dev/peps/pep-3134/

Sometimes it can be useful for an exception handler to intentionally re-raise an exception, either to provide extra information or to translate an exception to another type. The __cause__ attribute provides an explicit way to record the direct cause of an exception.

while implicit exception chaining is more like something wrong/unexpected happened during the exception handling logic:

For an implicitly chained exception, this PEP proposes the name __context__ because the intended meaning is more specific than temporal precedence but less specific than causation: an exception occurs in the context of handling another exception.

Anyway, this nuance won't affect the logic, as long as MSAL raises the original exception, instead of logging it.