Skip to content

Changes as an effect of changing persistent storage model. #80

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

Merged
merged 12 commits into from
Mar 21, 2021
Merged

Conversation

rohe
Copy link
Contributor

@rohe rohe commented Mar 20, 2021

Instead of having class instances doing the storage we instead want to put the burden on the frontend.
This will among other things decrease the number of DB calls significantly.
The frontend will have the methods dump, flush and load to store, clear and retrieve state.

@rohe rohe requested a review from jschlyter March 20, 2021 08:10
@codecov-io
Copy link

codecov-io commented Mar 20, 2021

Codecov Report

Merging #80 (cd543a9) into master (214d0a8) will decrease coverage by 1.22%.
The diff coverage is 44.85%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #80      +/-   ##
==========================================
- Coverage   78.39%   77.16%   -1.23%     
==========================================
  Files          41       41              
  Lines        4193     4283      +90     
  Branches      808      822      +14     
==========================================
+ Hits         3287     3305      +18     
- Misses        648      716      +68     
- Partials      258      262       +4     
Impacted Files Coverage Δ
src/cryptojwt/jwk/hmac.py 81.39% <0.00%> (ø)
src/cryptojwt/key_issuer.py 77.04% <23.80%> (-3.90%) ⬇️
src/cryptojwt/key_bundle.py 78.42% <44.61%> (-4.15%) ⬇️
src/cryptojwt/key_jar.py 76.31% <51.11%> (-3.69%) ⬇️
src/cryptojwt/__init__.py 91.66% <100.00%> (+0.36%) ⬆️
src/cryptojwt/jws/dsa.py 81.81% <100.00%> (-0.33%) ⬇️

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 836d3de...cd543a9. Read the comment docs.

pyproject.toml Outdated
@@ -22,7 +22,7 @@ exclude_lines = [

[tool.poetry]
name = "cryptojwt"
version = "1.4.1"
version = "1.4.2"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems like a minor version bump, 1.5.0 perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@@ -751,7 +751,7 @@ def difference(self, bundle):

return [k for k in self._keys if k not in bundle]

def dump(self):
def dump(self, cutoff: Optional[list] = None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Somewhat confusing attribute name (at least for me). Perhaps excluded_attrs is better?

The type should be Optional[List[str]] I think (I assume attribute names are strings)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Horticultural term :-)
Imaging that you are dealing with something that looks like a tree and there are branches you don't want to be included so you cut them off.
Yes, attribute names are strings.

"""
Returns the content as a dictionary.

:param exclude: Issuer that should not be include in the dump
Copy link
Collaborator

Choose a reason for hiding this comment

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

  • exclude_issuer (perhaps make it exclude_issuers and a Optional[List[str]] to allow multiple issuers to be excluded?)
  • exclude_attrs (or exclude_attributes)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK then.

@@ -32,7 +32,6 @@ def __init__(
remove_after=3600,
httpc=None,
httpc_params=None,
storage=None,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we assume that no one (excluding closing family) has used the storage class yet?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes !

jschlyter
jschlyter previously approved these changes Mar 20, 2021
Copy link
Collaborator

@jschlyter jschlyter left a comment

Choose a reason for hiding this comment

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

All good, perhaps we should use plural for some kwargs (see comments)

@@ -751,7 +752,7 @@ def difference(self, bundle):

return [k for k in self._keys if k not in bundle]

def dump(self):
def dump(self, exclude_attribute: Optional[List[str]] = None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

exclude_attributes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@@ -350,16 +352,17 @@ def __len__(self):
nr += len(kb)
return nr

def dump(self, exclude=None):
def dump(self, exclude_attribute: Optional[List[str]] = None) -> dict:
Copy link
Collaborator

Choose a reason for hiding this comment

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

exclude_attributes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

"""
Returns the key jar content as dictionary

:param exclude_issuer: A list of issuers you don't want included.
Copy link
Collaborator

Choose a reason for hiding this comment

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

exclude_issuers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK

@rohe rohe merged commit e1eabfd into master Mar 21, 2021
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