pysaml2
pysaml2 copied to clipboard
Refactor datetime
This changeset introduces a new module, namely saml2.datetime. It implements the concept of datetime and duration object and isolates all operations on those, under a common namespace. The new module ensures that creation, conversion and operations on those objects are kept in one place, are correct and in accordance to the specs.
The way the new module is written is such that it is not tied to a third party. The dependency should be easily swapped by filling in the required parsers; which should be as easy as changing the following two lines: saml2/datetime/__init__.py#L48 and saml2/datetime/duration.py#L5
aniso8601 was chosen to provide the datetime and duration parsers, as:
- it is focused on the standard and not generic date-time parsing
- it implements the duration part of the standard, that many other libraries do not
- it is concise and has a small codebase
- it does not use expensive regexes; in fact it uses no regexes (of course by itself this does not mean much)
- it has nice error handling; wrapping errors with custom exceptions
Things to improve:
- the documentation needs more work
- new tests are on their way
Motive for this was the discussion for issue #445 and the relevant thread on the mailing list. It overrides pull-request #517 .
Review is better done per commit. The reordering and cleanup of imports can be ignored.
Moreover, part of this PR is the introduction of two more modules:
compat.py: a module that gathers all compatibility code between python3 and python2errors.py: a module that gathers all errors for pysaml2. At the moment it provide the top-level generic errorSaml2Errorclass. All errors in the library should be a subclass ofSaml2Error.
All Submissions:
-
[x] Have you checked to ensure there aren't other open Pull Requests for the same update/change?
-
[x] Have you added an explanation of what problem you are trying to solve with this PR?
-
[x] Have you added information on what your changes do and why you chose this as your solution?
-
[x] Have you written new tests for your changes? (on their way)
-
[x] Does your submission pass tests?
-
[x] This project follows PEP8 style guide. Have you run your code against the 'flake8' linter?
Codecov Report
Merging #518 into master will increase coverage by
0.08%. The diff coverage is83.5%.
@@ Coverage Diff @@
## master #518 +/- ##
==========================================
+ Coverage 65.17% 65.26% +0.08%
==========================================
Files 100 109 +9
Lines 25691 25741 +50
==========================================
+ Hits 16745 16799 +54
+ Misses 8946 8942 -4
| Impacted Files | Coverage Δ | |
|---|---|---|
| src/saml2/mcache.py | 0% <0%> (ø) |
:arrow_up: |
| src/saml2/authn.py | 0% <0%> (ø) |
:arrow_up: |
| src/saml2/httputil.py | 0% <0%> (ø) |
:arrow_up: |
| src/saml2/datetime/timezone.py | 100% <100%> (ø) |
|
| src/saml2/request.py | 70.71% <100%> (ø) |
:arrow_up: |
| src/saml2/client_base.py | 76.17% <100%> (+0.28%) |
:arrow_up: |
| src/saml2/errors.py | 100% <100%> (ø) |
|
| src/saml2/entity.py | 83.86% <100%> (+0.07%) |
:arrow_up: |
| src/saml2/cache.py | 84.53% <100%> (+1.39%) |
:arrow_up: |
| src/saml2/assertion.py | 87.5% <100%> (+0.35%) |
:arrow_up: |
| ... and 31 more |
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update c3d2f15...e6fb4a3. Read the comment docs.
What is the status of this PR? I have another (small) changeset that adds support for explicit timezones in the date string parsing. However, this PR being merged would obviate the need for that work.
My actual change is at https://github.com/IdentityPython/pysaml2/compare/master...CBitLabs:fix/explicit_offset?expand=1
Bump.... I'd really like to see this merged (once the conflicts are fixed).
This Is a very important code refactor 🖖
Please can we get this merged...... (after fixing the conflicts)
my 2 cents here since I haven't caught up with the discussion and the PR: I tried testing this locally and I noticed the following:
When this PR was opened, latest version of package aniso8601 was 3.0.2
Package aniso8601 is now in version 7.0.0 (in version 6.0.0 it is mentioned that version 7.0.0 will drop py2 support but pypi stills shows compatibility). In version 4.0.0 the UTCOffset class (which is used in pysaml2 compat.py (from aniso8601.timezone import UTCOffset as _Timezone) is moved to a new file called builder.py and from 5.0.0 this class is moved to a new file called utcoffset.py.
So I suggest we pin the aniso8601 to at least version 5.0.0 and make the change to compat.py