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

First steps to reduce coupling between lib level and flask #1313

Merged
merged 8 commits into from Dec 3, 2018

Conversation

Projects
None yet
3 participants
@fredreichbier
Member

fredreichbier commented Nov 21, 2018

This PR implements an idea for reducing the coupling to flask by moving all except 1 explicit import of flask in lib/ to a single module privacyidea/lib/framework.py. This module exports functions to access an app-local and request-local dictionary, as well as to read the app configuration.

This is more like a Request-for-comments :-)

This changes quite some code:

  • in the lib level because we use the functions get_app_config and get_app_config_values instead of flask.current_app.config.
  • in privacyidea.lib.config, because we now store the config object in the request-local store, and only access it (on API and lib level) using new functions update_config_object and get_config_object. This will also be useful to fix #1132 later.
  • in privacyidea.lib.crypto because we now store the HSM object in the app-local store, and only access it using a new function get_hsm. This also moves some code form the API level to the lib level.
  • in privacyidea.lib.pooling because we now store the engine registry in the app-local store
  • in privacyidea.lib.lifecycle because we now store the finalizers in the request-local store

The only remaining import of Flask is in the Federation event handler which uses flask.Response to relay the response from the remote privacyIDEA server to the local request. I'm not sure if and how we can change this nicely. :-/

Merging #1313 into master will increase coverage by 0.21%.
The diff coverage is 95.32%.
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1313      +/-   ##
==========================================
+ Coverage   95.79%   96.01%   +0.21%
==========================================
Files         142      143       +1
Lines       17283    17859     +576
==========================================
+ Hits        16557    17148     +591
+ Misses        726      711      -15
Impacted Files Coverage Δ
privacyidea/lib/resolver.py 100% <100%> (+3.37%) ⬆️
privacyidea/lib/tokens/vasco.py 96.72% <100%> (ø) ⬆️
privacyidea/lib/realm.py 100% <100%> (ø) ⬆️
privacyidea/lib/lifecycle.py 100% <100%> (ø) ⬆️
privacyidea/api/validate.py 100% <100%> (ø) ⬆️
privacyidea/lib/pooling.py 92.1% <100%> (+0.21%) ⬆️
privacyidea/lib/eventhandler/usernotification.py 89.74% <100%> (ø) ⬆️
privacyidea/api/ttype.py 95.55% <100%> (ø) ⬆️
privacyidea/lib/policy.py 99.64% <100%> (+0.23%) ⬆️
privacyidea/lib/__init__.py 100% <100%> (ø) ⬆️
... and 20 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 dccc9f4...2041409. Read the comment docs.

  • Thanks for the review! I'll also add some tests to improve coverage.
  • Nice naming! THanks!
  •  File: privacyidea/lib/framework.py:L1-66
  • I like the overall concept.
    I should think a bit about the naming. I think the module name "framework" is nice.
    These names seem a bit long to me "get_application_local_store()". Should be use the property decorator and shorten it .
    I think would like to drop the "get_"-part. But then the call would be
appstore = application_local_store()

which migt also look a bit wired. Unfortunately the property decorator does not work on functions, only on methods.

  • Yes, I also found naming quite hard :-)
    I agree the name is a bit long. I actually think keeping the get_ prefix would be nice, because it is consistent with a lot of other get_ functions in the lib level. But we could shorten it anyway: As we also have get_app_config, we could shorten this to get_app_local_store? Or even get_app_store (don't let Apple read this!) and get_request_store?
  • As a first step, I renamed get_application_local_store to get_app_local_store in
    484c508. This is also more consistent with get_app_config.
  •  File: privacyidea/lib/framework.py:L1-66
  • I was confused by get_app_config_values and get_app_config.
    I was expecting to get the complete config object with get_app_config. But it was the other way round.
    Should we put it into one function? Or should we do
    get_app_config -> return the complete dictionary.
    get_app_configvalue -> return only one value.
    A function get_app_config would always return a dictionary. Otherwise we would not need the abstraction layer.
    Could we drop get_app_configvalue and instead do:
value = get_app_config().get(key, None)
  • I agree, I also found that confusing :-)
    get_app_config already existed prior to this PR, I just moved it from privacyidea.lib.config to privacyidea.lib.framework.
    I think it would be nice to keep a dedicated function to retrieve one config value, because this is needed very often. The complete dictionary is only needed at one place, by the HSM initialization code.
    So what do you think about just renaming
  • get_app_config to get_app_config_value and
  • get_app_config_values to get_app_config
    ?
  • Sounds good to me, if get_app_config returns the complete config and get_app_config_value returns a single value from the config.
  • Alright, will do that!
  • done in 6bf4167
  •  File: privacyidea/lib/policy.py:L161-170
  • Is there a reason why You sometimes use from .framework import ... and sometime from privacyidea.lib.framework import ...?
  • No, I think some of these are from my IDE and some are handwritten :) What style would be better?
  • Personally I prefer absolute paths/modules to relative ones. I don't know if there is a pythonic way. At least we should be consistent I think.
  • A lot of style guides seem to recommend absolute imports too (example). So I changed all new imports to absolute imports in this commit. However, this isn't consistent with other imports in the respective files :-) But I guess that's ok for now?

  

@fredreichbier fredreichbier requested a review from privacyidea/core Nov 21, 2018

@codecov

This comment has been minimized.

codecov bot commented Nov 21, 2018

Codecov Report

Merging #1313 into master will decrease coverage by 0.37%.
The diff coverage is 98.13%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1313      +/-   ##
==========================================
- Coverage   96.39%   96.01%   -0.38%     
==========================================
  Files         141      143       +2     
  Lines       17175    17796     +621     
==========================================
+ Hits        16556    17087     +531     
- Misses        619      709      +90
Impacted Files Coverage Δ
privacyidea/lib/resolver.py 100% <100%> (ø) ⬆️
privacyidea/lib/tokens/vasco.py 96.72% <100%> (ø) ⬆️
privacyidea/lib/realm.py 100% <100%> (ø) ⬆️
privacyidea/lib/lifecycle.py 100% <100%> (ø) ⬆️
privacyidea/api/validate.py 100% <100%> (ø) ⬆️
privacyidea/lib/pooling.py 92.1% <100%> (+0.21%) ⬆️
privacyidea/lib/eventhandler/usernotification.py 89.74% <100%> (ø) ⬆️
privacyidea/api/ttype.py 95.55% <100%> (ø) ⬆️
privacyidea/lib/policy.py 99.82% <100%> (+0.41%) ⬆️
privacyidea/lib/__init__.py 100% <100%> (ø) ⬆️
... and 30 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 3d8b761...67a9f82. Read the comment docs.

from flask_babel import gettext as _
def get_application_local_store():

This comment has been minimized.

@cornelinux

cornelinux Nov 21, 2018

Member

I like the overall concept.
I should think a bit about the naming. I think the module name "framework" is nice.
These names seem a bit long to me "get_application_local_store()". Should be use the property decorator and shorten it .

I think would like to drop the "get_"-part. But then the call would be

appstore = application_local_store()

which migt also look a bit wired. Unfortunately the property decorator does not work on functions, only on methods.

This comment has been minimized.

@fredreichbier

fredreichbier Nov 21, 2018

Member

Yes, I also found naming quite hard :-)
I agree the name is a bit long. I actually think keeping the get_ prefix would be nice, because it is consistent with a lot of other get_ functions in the lib level. But we could shorten it anyway: As we also have get_app_config, we could shorten this to get_app_local_store? Or even get_app_store (don't let Apple read this!) and get_request_store?

This comment has been minimized.

@fredreichbier

fredreichbier Nov 22, 2018

Member

As a first step, I renamed get_application_local_store to get_app_local_store in
484c508. This is also more consistent with get_app_config.

return g._request_local_store
def get_app_config_values():

This comment has been minimized.

@cornelinux

cornelinux Nov 21, 2018

Member

I was confused by get_app_config_values and get_app_config.
I was expecting to get the complete config object with get_app_config. But it was the other way round.

Should we put it into one function? Or should we do

get_app_config -> return the complete dictionary.
get_app_configvalue -> return only one value.

A function get_app_config would always return a dictionary. Otherwise we would not need the abstraction layer.

Could we drop get_app_configvalue and instead do:

value = get_app_config().get(key, None)

This comment has been minimized.

@fredreichbier

fredreichbier Nov 21, 2018

Member

I agree, I also found that confusing :-)

get_app_config already existed prior to this PR, I just moved it from privacyidea.lib.config to privacyidea.lib.framework.

I think it would be nice to keep a dedicated function to retrieve one config value, because this is needed very often. The complete dictionary is only needed at one place, by the HSM initialization code.

So what do you think about just renaming

  • get_app_config to get_app_config_value and
  • get_app_config_values to get_app_config

?

This comment has been minimized.

@cornelinux

cornelinux Nov 21, 2018

Member

Sounds good to me, if get_app_config returns the complete config and get_app_config_value returns a single value from the config.

This comment has been minimized.

@fredreichbier

fredreichbier Nov 21, 2018

Member

Alright, will do that!

This comment has been minimized.

@fredreichbier
@fredreichbier

This comment has been minimized.

Member

fredreichbier commented Nov 21, 2018

Thanks for the review! I'll also add some tests to improve coverage.

from flask_babel import gettext as _
def get_application_local_store():

This comment has been minimized.

@cornelinux
return g._request_local_store
def get_app_config_values():

This comment has been minimized.

@cornelinux
@cornelinux

This comment has been minimized.

Member

cornelinux commented Nov 22, 2018

Nice naming! THanks!

from privacyidea.lib.config import (get_token_classes, get_token_types,
Singleton)
from privacyidea.lib.framework import get_app_config_value

This comment has been minimized.

@plettich

plettich Nov 26, 2018

Contributor

👍

@cornelinux cornelinux merged commit df002e8 into master Dec 3, 2018

0 of 3 checks passed

ci/circleci CircleCI is running your tests
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@cornelinux cornelinux deleted the 331/decouple-flask branch Dec 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment