pysaml2 icon indicating copy to clipboard operation
pysaml2 copied to clipboard

Refactor datetime

Open c00kiemon5ter opened this issue 7 years ago • 6 comments

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 python2
  • errors.py: a module that gathers all errors for pysaml2. At the moment it provide the top-level generic error Saml2Error class. All errors in the library should be a subclass of Saml2Error.

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?

c00kiemon5ter avatar Jul 06 '18 18:07 c00kiemon5ter

Codecov Report

Merging #518 into master will increase coverage by 0.08%. The diff coverage is 83.5%.

Impacted file tree graph

@@            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 data Powered by Codecov. Last update c3d2f15...e6fb4a3. Read the comment docs.

codecov-io avatar Jul 06 '18 19:07 codecov-io

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

pjsg avatar Oct 19 '18 15:10 pjsg

Bump.... I'd really like to see this merged (once the conflicts are fixed).

pjsg avatar Dec 19 '18 15:12 pjsg

This Is a very important code refactor 🖖

peppelinux avatar Feb 26 '19 22:02 peppelinux

Please can we get this merged...... (after fixing the conflicts)

pjsg avatar Jun 18 '19 20:06 pjsg

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

ioparaskev avatar Jul 25 '19 15:07 ioparaskev