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

Handle file not found exceptions on Python 2.7 #69

Merged
merged 2 commits into from
Aug 25, 2020
Merged

Conversation

chlowell
Copy link
Contributor

@chlowell chlowell commented Aug 14, 2020

On Python 3, calling PersistedTokenCache.find() on a nonexistent cache file is a no-op, so this code works:

from msal_extensions import FilePersistence, PersistedTokenCache

persistence = FilePersistence('nope')
cache = PersistedTokenCache(persistence=persistence)
cache.find('')

On Python 2.7 it raises a platform-specific error: WindowsError on Windows, OSError on Linux (I haven't tried it on macOS). The difference in behavior is due to PersistedTokenCache anticipating IOError when a file isn't found. Neither WindowsError nor OSError is a subclass of IOError. All these errors are subclasses of EnvironmentError though, and that class has the errno attribute, so a simple fix is to handle EnvironmentError here.

@rayluo
Copy link
Contributor

rayluo commented Aug 14, 2020

Thanks for the finding! How urgent is this? We have been working on some relevant PRs here, and could cut a release within a week or two. Will that work?

@chlowell
Copy link
Contributor Author

There are workarounds but it affects our core getting started scenario, so one week is much better than two. Will the release be 0.2.3?

@rayluo
Copy link
Contributor

rayluo commented Aug 17, 2020

Well then, let's aim for a release with this fix at no later than next Monday (that is one week).

PR wise, it would be better to see some test case(s) to guard a potential future regression. If Charles can provide some, it would be great; otherwise @abhidnya13 and I will still pick this up within next couple days.

@abhidnya13
Copy link
Contributor

Thanks @chlowell for the code snippet. I created a branch to add a test case for this.
We can see that -

  • BEFORE state - where the test fails for Python 2.7 ONLY. Our build in travis ci does not test Python 2.7 on Mac but I tested this on my Mac with Python 2.7 and we get the same OSError as in Linux.
  • AFTER state - where we change the code to use Environment Error and the test case passes on all the platforms and all the versions.
    @rayluo I can create a PR to be merged in this branch for adding such a test to verify. How do you want to go about this ?

@rayluo
Copy link
Contributor

rayluo commented Aug 25, 2020

@abhidnya13 Thanks very much for the before-and-after comparison!

Either creating a new PR (and linking back to this one), or adding your test case to this PR. They are both good.

Copy link
Contributor

@abhidnya13 abhidnya13 left a comment

Choose a reason for hiding this comment

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

Added a test case for this bug. The change looks good to me ! Thanks Charles !

Copy link
Contributor

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Good collaboration! Merging.

@rayluo rayluo merged commit c537cf2 into AzureAD:dev Aug 25, 2020
This was referenced Sep 1, 2020
@chlowell chlowell deleted the patch-1 branch September 8, 2020 16:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants