Skip to content

Refactor config attributes loading #251

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Refactor config attributes loading #251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9431398
Add key and value check for mandatory_dict_keys
ioparaskev Jul 10, 2019
7219b69
Refactor config attributes loading
ioparaskev Jul 10, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
79 changes: 53 additions & 26 deletions src/satosa/satosa_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ class SATOSAConfig(object):
A configuration class for the satosa proxy. Verifies that the given config holds all the
necessary parameters.
"""

sensitive_dict_keys = ["STATE_ENCRYPTION_KEY"]
mandatory_dict_keys = ["BASE", "BACKEND_MODULES", "FRONTEND_MODULES",
"INTERNAL_ATTRIBUTES", "COOKIE_STATE_NAME"]
Expand All @@ -30,11 +31,12 @@ def __init__(self, config):
:param config: Can be a file path or a dictionary
:return: A verified SATOSAConfig
"""
parsers = [self._load_dict, self._load_yaml]
for parser in parsers:
self._config = parser(config)
if self._config is not None:
break
self._config = self._parse_config(config)

if not self._config:
raise SATOSAConfigurationError(
"Missing configuration or unknown format"
Copy link
Member

Choose a reason for hiding this comment

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

How can we know which of the two it is?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We didn't and we don't

Copy link
Member

Choose a reason for hiding this comment

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

How can we provide this information?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Uknown format means parsing failed or returned empty dict whereas missing configuration could mean that either an empty config argument was given to the constructor or that the argument was a file that couldn't be opened. The first case was/is only indirectly checked. The second will be displayed in the logs (if _load_yaml cannot open the file for example). I'm not saying that this is the way it should be done, I'm just saying how it is currently implemented

Copy link
Member

Choose a reason for hiding this comment

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

I'm not saying that this is the way it should be done, I'm just saying how it is currently implemented

Yes, I know; and what I'm saying is that we should make clear what went wrong. How we make it clearer is what we are discussing.

Ideas to improve this:

  • Provide config as part of the message and let the user figure out which of the two it is implicitly
  • Make more fine grained checks and provide the exact message

Copy link
Contributor Author

Choose a reason for hiding this comment

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

These should probably be written in a new issue, otherwise in a PR they will be lost

)
Copy link
Member

Choose a reason for hiding this comment

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

Should this be part of _parse_config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. This statement used to be in the beginning of _verify_dict which was passed the self._config as an argument. It was not used as a generic error, only to state the failure of the initial config parsing

Copy link
Member

Choose a reason for hiding this comment

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

Let me rephrase the question.
Should the check if not self._config: raise error() be inside _parse_config?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope. _parse_config parses the argument config. It could as well be a staticmethod or function if the _load_dict and _load_yaml where not instance methods (and they could as well be external functions, there is no need to be coupled inside the SATOSAConfig object

Copy link
Member

@c00kiemon5ter c00kiemon5ter Jul 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

This is not answering what I am asking. Whether those are functions or methods is irrelevant to the question. What I am asking is who is responsible for checking if the result of an action is an error.

You do not need to think about this particular case at all; see the example bellow. You have two choices, the first is what is being done now - the caller is checking for errors

def foo(x):
    value = bar(x)
    if has_error(value):
        generate_error()
    do_something_with(value)

def bar(x):
    value = get_value(x)
    return value

and a second choice, where the callee is checking for the error

def foo(x):
    value = bar(x)
    do_something_with(value)

def bar(x):
    value = get_value(x)
    if has_error(value):
        generate_error()
    return value

How can we compare those two and choose one over the other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends is the answer. It depends on what kind of error is this. Is it an evaluation error or it is a logic error? bar could throw errors if they have to do with the processing of x but if there is a logical error with the processing of x, should bar know the logic of foo and throw the error? I would say no

Copy link
Member

Choose a reason for hiding this comment

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

OK, so what we do here is we parse the config. And parsing fails - none of the parsers can parse the given thing. By what you said, _parse_config should then throw the error, because it failed to do its job. The return value of None is not a logical error, it is a parsing error; a return value sentinel that signifies the failure of the operation. For this specific operation, an evaluation error would be a config that was parsed but has inconsistent values for the different options it holds, ie it would be a validation error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed in this case, it is a parsing error and the parsing function should throw this


# Load sensitive config from environment variables
for key in SATOSAConfig.sensitive_dict_keys:
Expand All @@ -44,26 +46,49 @@ def __init__(self, config):

self._verify_dict(self._config)

self._load_plugins()
self._load_internal_attributes()

def _parse_config(self, config):
return next(
filter(
lambda conf: conf is not None,
map(
lambda parser: parser(config),
(self._load_dict, self._load_yaml),
),
),
None,
)
Copy link
Member

Choose a reason for hiding this comment

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

I would write this, as:

value = next(
    (
        conf
        for parser in [self._load_dict, self._load_yaml]
        for conf in [parser(config)]
        if conf
    ),
    None,
)
return value

it is cleaner, and more "pythonic"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For "pythonic" (whatever this vague term means) I agree. For cleaner I think it's more subjective. I always found nested comprehensions to be less clear than nested maps (especially when nesting > 2)

Copy link
Member

@peppelinux peppelinux Jul 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

If I should put a debugger there I'm sure that there will be problems.
Pythonic is elegant, compact, winking and whatever but probably it could be better a simplified and debuggable approach. Just put import pdb; pdb.set_trace() before that code before choosing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you put the debugger inside the comprehension to debug it? On the contrary I think it's easier to debug filter & map since each map call can be debugged on it's own, whereas a nested comprehension must be run fully or rewritten in smaller parts to be debugged.

Copy link
Member

Choose a reason for hiding this comment

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

You simply can't! This is why it would be better to avoid that kind of generalization. This is just a personal view, I like the others code style, I also imitate these if needed and with curiosity. I'm jut getting older, that's all. Because of this I'd prefer a simple and very debuggable code. I've get into this thread to exchange some words about my personal view, now I've done, you can simply ignore me and go ahead 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you really need to drop a debugger, it is very easy to convert the comprehension to a regular for-loop and place it under the right scope. While, you need to think more to convert the map/filter style into a regular for-loop.

You don't have to convert anything in the map/filter, you simply evaluate each part (the map part, or/and the filter part). There is no need to write any for loop.

Map/filter power is composition which cannot be achieved with comprehensions. Try having a 4 times nested comprehension (for complex constructs) to see how readable it is (I'm not sure even if it is achievable). Whereas map/filter does not change at all, you compose each map as an argument to the next map. List comprehensions have a limit since they are a syntactic construct whereas map is a higher order function that can be composed with much less limitation.
As I said, I personally always found nested list comprehensions (especially crowded with if statements) more difficult to process (due to the change in order) and I've come to prefer map/filter exactly because I can compose them one part at a time and join them without having to think the order of the for statement like in comprehensions.

Readability for this specific discussion, depends on the experience one has with map/filter usage and concepts and it is a personal taste as well. If we had this conversation 10 years ago, mutating everything like crazy and having 10 levels deep for loops could be a normal thing that one would consider "readable". There is a reason concepts like map/filter and other functional concepts are being introduced and used more and more today even (if Guido wants to rant about how hard he finds map/filter/reduce and prefers comprehensions). The same thing can be said about the usage of next. One may be more familiar with a for loop with a break statement.

Copy link
Member

Choose a reason for hiding this comment

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

you simply evaluate each part

you cannot evaluate each part, as the two are nested. I would understand this if the code was in the form

intermediate_result = map(foo, bar)
final_result = filter(baz, intermediate_result)

which directly translates to comprehensions

intermediate_result = (foo(x) for x in bar)
final_result = (x for x in intermediate_result if baz(x))

Map/filter power is composition

we agree on that.

which cannot be achieved with comprehensions

I can't see why and I'm interested in understanding this.

join them without having to think the order of the for statement

what is the order of the for statement?

For this specific snippet, there is nothing being composed. Creating lambdas on the fly is not composition. You would be composing if these were functions that were being reused to achieve a greater effect. Instead, this is just hardcoded behaviour in a map/filter construct. Just because someone is using map/filter/reduce, it does not mean that they're composing.

Copy link
Contributor Author

@ioparaskev ioparaskev Jul 12, 2019 •
edited by c00kiemon5ter
Loading

Choose a reason for hiding this comment

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

you simply evaluate each part

you cannot evaluate each part, as the two are nested. I would understand this if the code was in the form

in order to debug the following

(
        conf
        for parser in [self._load_dict, self._load_yaml]
        for conf in [parser(config)]
        if conf
)

you have to evaluate its parts. to evaluate them you have to write a for loop.

in order to evaluate this:

filter(lambda conf: conf is not None, map(lambda parser: parser(config), (self._load_dict, self._load_yaml)))

you evaluate map(lambda parser: parser(config), (self._load_dict, self._load_yaml)) and you evaluate filter(lambda conf: conf is not None on the result. there is no need to think of any for loop, that's what I'm saying

Map/filter power is composition

we agree on that.

which cannot be achieved with comprehensions

I can't see why and I'm interested in understanding this.

I actually meant functional decomposition (splitting a problem into a set of functions). Composition is another feature that is not builtin in python (you have to create a function that will create composite function whereas in other languages there is a operator to do that)
Let me rephrase this. You cannot use them the way you use map/filter to process things. If you want to create list comprehensions with multiple checks and processing you have to write a new nested comprehension with extra variables.

For example:

def remainder_of_two(x):
    return int(x) % 2

def add_star(x):
    return "*{}".format(x)

I want to flatten a list of integers a=[[1,2],[3,4]], transform them to string, get those that are odd and add a * character to them. I have the above functions.
With map/reduce/filter, I feed each operation into the other:

list(map(add_star, filter(remainder_of_two, map(str, reduce(add, a)))))

reading from right to left, I flatten the list, then I get the string representation of them, then I filter out those that are even, then I add the star character

With list comprehensions and if statements:

[add_star(y) for y in [x for b in a for x in b if remainder_of_two(str(x)) != 0]]

Now how do I read this to describe what is done? Can I read from right to left? No. Can I read from left to right? Probably, let's try: I add a star character to a variable that is the one that is contained in a list of other variables that the string representation of that sub-variable when applied the remainder_of_two function, returns != 0. Is this narrative correct? Not exactly because I never mentioned the flattening of the list of lists explicitly. That kind of processing with list comprehensions needs extra variables. map/filter/reduce are higher order functions and don't need those (unless you pass a lambda). (sidenote: until python 3 list comprehensions were leaking scope of their variables (that is, they were accessing outside variables instead of creating a local scope).

join them without having to think the order of the for statement

what is the order of the for statement?

As I said before, narrating the operation in one's head, feeding results in functions is simpler than having to keep variables in brain memory. In our case:
I want to apply a list of functions in a config, and if their result is truthish, I want to get the result:

(
        conf
        for parser in [self._load_dict, self._load_yaml]
        for conf in [parser(config)]
        if conf
)

How do we express this above? I have a result called conf that -skip to line 3- when applied to a variable called parser -skip to line 2- where parser is a loop over a list of functions, -skipt to line 4- if conf is truthish. What I mean by this extremely naive narration, is that in comprehensions you have to read the whole expression to understand what's happening. Whereas in map/filter in our case:

filter(lambda conf: conf is not None, map(lambda parser: parser(config), (self._load_dict, self._load_yaml)))

Reading for right to left: I have a list of functions, and I apply them to a config, and then the results I feed them to a check to see if they are truthish and keep those that are.

For this specific snippet, there is nothing being composed. Creating lambdas on the fly is not composition. You would be composing if these were functions that were being reused to achieve a greater effect. Instead, this is just hardcoded behaviour in a map/filter construct. Just because someone is using map/filter/reduce, it does not mean that they're composing.

Indeed there is no composition here since there aren't any composite functions

Copy link
Member

@c00kiemon5ter c00kiemon5ter Jul 13, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

in order to debug the following [...] you have to evaluate its parts.

yes.

to evaluate them you have to write a for loop.

no. As with map/filter you evaluate the different parts.

in order to evaluate this: [...]
you evaluate map()
and you evaluate filter() on the result

Which means that you evaluate its parts.
And you can only do that if these are written separately, not nested.

I think that you are assuming that the way map/filter work in python, is the same as in most other languages, but this is not the case. The map() and filter() functions return map and filter objects respectively. map and filter objects are actually iterators that yield (they work just like generators) items. This means that map and filter are not separately evaluated. When a function pulls an item (pull-functions are next(), list(), etc) the filter-fn will retrieve the next item from the given iterable; this iterable is the map-iterator; to get the next item of the map-iterator, the map-fn retrieves the next item from the given iterable; the map-fn applies its callable to the item, saves its state and returns a single result; then the filter-fn applies its callable to the item it got from the map-iterator, saves its state and returns a single result. The map/filter-fn never evaluate completely (or anything), if a pull-fn does not ask them to.

This works exactly as comprehensions work, and it is the reason you need to wrap the outermost function (filter in this case) with a pull-function, as you do with comprehensions.

In most other languages, map and filter do not return an iterator, but a complete result. That used to be the case for python2; but for python3 the effect is the same as with comprehensions.

there is no need to think of any for loop, that's what I'm saying

yes. The reason I mentioned the transformation to the for loop, was to be able to invoke a debugger inside the loop.

If you're saying there is not reason to look at the for-keyword of the comprehension, then you can hide it behind a function, just like map/filter do. Just because you do not see the for keyword, it does not mean that there is no loop.

The only good thing with map/reduce is that you can replace them with versions that can parallelize the operation safely. (which is not needed here and in the majority of the cases)

I want to

  • flatten a list of integers a=[[1,2],[3,4]],
  • transform them to string,
  • get those that are odd
  • and add a * character to them.

With map/reduce/filter, I feed each operation into the other:

list(
    map(                       # add a * character
        add_star,
        filter(                # get odds
            remainder_of_two, 
            map(
                str,           # convert to str
                reduce(add, a) # flatten
            )
        )
    )
)

reading from right to left ...

Reading from right to left is the opposite of what people do. To be precise, this reads inside-out (or bottom-top if you will).

With list comprehensions and if statements:

[
    add_star(y)
    for y in [
        x
        for b in a 
        for x in b 
        if remainder_of_two(
            str(x)
        )
    ]
]

There is no need for this nesting. This should be written as

list(
    add_star(s)                        # add a * character
    for sublist in a for x in sublist  # flatten
    for s in [str(x)]                  # convert to str
    if remainder_of_two(s)             # check for odd
)

or even

list(
    v
    for sublist in a for x in sublist  # flatten
    for s in [str(x)]                  # convert to str
    if remainder_of_two(s)             # check for odd
    for v in [add_star(y)]             # add a * character
)

Reading is fine, from top to bottom.

How do we express this above? I have a result called conf that -skip to line 3- ...

What you're doing here is the same as this:

In order to read this:

filter(lambda conf: conf is not None, map(lambda parser: parser(config), (self._load_dict, self._load_yaml)))

you have to find the inner most function, then find the leftmost argument, then apply it to each item of the remaining arguments, then imagine the results, then skip to next outer function, then find the leftmost argument, then apply it to the remaining arguments including the one you imagined before, then imagine the results, and so on.

(which, again, is not true, because in practice map and filter return iterators.)

Likewise, I can read the comprehension as:

for each parser, apply it to config and generate a conf, if that is truthish keep it

I doubt that you find it easier to locate the inner-most function, and immediately reason about it, while you find it hard to read that comprehension.

In general, comprehensions read from top to bottom, left to right, and accumulate results in the very first line. Instead map/filter read inside-out, and demand understanding of the arguments.
I can read both, but I can read comprehensions easier, because the order of the statements is natural. Badly written code will read bad no matter if it is within a comprehension or a map/filter.

Having said all this, I do not think that there is any other place where you would nest functions one inside the other like you do with map and filter. You would not do:

command3(command2(command1(arg1, arg2)))

instead you would split this into multiple lines that feed results into each other.

res1 = command1(arg1, arg2)
res2 = command2(res1)
res3 = command3(res2)

so, I cannot understand how this changes with map and filter, and how it is helpful to anyone to read it nested like that. Do people really write nested code like that with map/filter/reduce? Do they try to convert their whole programs into a one-liner? Where does this come from?

That kind of processing with list comprehensions needs extra variables

The only reason you do not have variables is because of this nesting. Variables is not a bad thing that we should avoid. Instead, variables are pretty helpful to document in-code the intermediate results.

Copy link
Contributor Author

@ioparaskev ioparaskev Jul 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

In most other languages, map and filter do not return an iterator, but a complete result. That used to be the case for python2; but for python3 the effect is the same as with comprehensions.

Well it depends. In Clojure for example maps are lazy (realized in chunks to be exact) whereas in Haskell they are not (they are non-strict which is not the same) although they act like the same

there is no need to think of any for loop, that's what I'm saying

yes. The reason I mentioned the transformation to the for loop, was to be able to invoke a debugger inside the loop.

If you're saying there is not reason to look at the for-keyword of the comprehension, then you can hide it behind a function, just like map/filter do. Just because you do not see the for keyword, it does not mean that there is no loop.

I said it to point the fact that you simply evaluate each part without rewriting it whereas in list comprehensions you have to rewrite it if it has many parts (i.e nested comprehension with if statement)

How do we express this above? I have a result called conf that -skip to line 3- ...

What you're doing here is the same as this:

In order to read this:

filter(lambda conf: conf is not None, map(lambda parser: parser(config), (self._load_dict, self._load_yaml)))

you have to find the inner most function, then find the leftmost argument, then apply it to each item of the remaining arguments, then imagine the results, then skip to next outer function, then find the leftmost argument, then apply it to the remaining arguments including the one you imagined before, then imagine the results, and so on.

(which, again, is not true, because in practice map and filter return iterators.)

Likewise, I can read the comprehension as:

for each parser, apply it to config and generate a conf, if that is truthish keep it

I doubt that you find it easier to locate the inner-most function, and immediately reason about it, while you find it hard to read that comprehension.

In general, comprehensions read from top to bottom, left to right, and accumulate results in the very first line. Instead map/filter read inside-out, and demand understanding of the arguments.
I can read both, but I can read comprehensions easier, because the order of the statements is natural. Badly written code will read bad no matter if it is within a comprehension or a map/filter.

Well from top to bottom in the example I read a v and the declaration for what v is, is at the bottom of the statement. That's what I meant that in comprehensions you use goto in your mind whereas in map/filter/reduce you read inside out and chain the parts together (you read, "I first do this, then I do that, then I do that etc" whereas in comprehensions you have to remember variable names and jump inside the comprehension to read). So comprehensions don't exactly read from top to bottom because if you read them from top to bottom you'll have to remember what v is. So you choose either goto and not top-to-bottom reading either remembering what that variable v in the first line will be defined 3-4 lines later.

Having said all this, I do not think that there is any other place where you would nest functions one inside the other like you do with map and filter. You would not do:

so, I cannot understand how this changes with map and filter, and how it is helpful to anyone to read it nested like that. Do people really write nested code like that with map/filter/reduce? Do they try to convert their whole programs into a one-liner? Where does this come from?

They don't I'm simply mentioning this to underline why I think map/filter is more powerful; because it chains better

That kind of processing with list comprehensions needs extra variables

The only reason you do not have variables is because of this nesting. Variables is not a bad thing that we should avoid. Instead, variables are pretty helpful to document in-code the intermediate results.

Variables is not a bad thing indeed. I simply mentioned them that you have to remember variables when reading a comprehension

I think we're beating a dead horse here, we will probably not reach an agreement. I think this ends up to personal taste. People using mostly Python might find comprehensions more intuitive because they are more exposed to them and the community & BDFL supports them. I started this discussion to underline the fact that "clearer" and "pythonic" are not objective terms and should be taken with a grain of salt. Especially "pythonic" can lead to assumptions that are counter-intuitive for other languages (i.e by default ordered dicts, assignment expressions as in pep572 etc)


def _load_plugins(self):
def load_plugin_config(config):
plugin_config = self._parse_config(config)
if not plugin_config:
raise SATOSAConfigurationError(
"Failed to load plugin config '{}'".format(config)
Copy link
Member

Choose a reason for hiding this comment

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

Why is this message different than the one on line 38?

)
else:
return plugin_config
Copy link
Member

Choose a reason for hiding this comment

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

We should try to have a straight codepath. else is not needed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thought that "explicit is better than implicit" for the eye here, but sure

Copy link
Member

@c00kiemon5ter c00kiemon5ter Jul 11, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

if/else says that at this point there is one of two things that you can follow. This is not the case here. What we do here is just check and handle an error case. To make this clearer forget that the else part is a return statement. return is just a command, and it is only by chance that there only one command here. If you imagine we had 4 commands would you still nest them under the else branch?

def foo(x):
    value = command1(x)
    if has_error(value):
        generate_error()
    else:
        command2(value)
        command3(value)
        command4(value)

or

def foo(x):
    value = command1(x)
    if has_error(value):
        generate_error()
    command2(value)
    command3(value)
    command4(value)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends is the answer again. I would probably have a validation function call to throw the error and hide the if from foo so as to keep in main view one logical exit path

Copy link
Member

Choose a reason for hiding this comment

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

to keep in main view one logical exit path

I feel we're saying the same thing. To keep one logical path you would not put the rest of the code under the else branch.


# Read plugin configs from dict or file path
for key in ["BACKEND_MODULES", "FRONTEND_MODULES", "MICRO_SERVICES"]:
plugin_configs = []
for config in self._config.get(key, []):
for parser in parsers:
plugin_config = parser(config)
if plugin_config:
plugin_configs.append(plugin_config)
break
else:
raise SATOSAConfigurationError('Failed to load plugin config \'{}\''.format(config))
self._config[key] = plugin_configs

for parser in parsers:
_internal_attributes = parser(self._config["INTERNAL_ATTRIBUTES"])
if _internal_attributes is not None:
self._config["INTERNAL_ATTRIBUTES"] = _internal_attributes
break
self._config[key] = list(
map(
lambda x: load_plugin_config(x),
self._config.get(key, []) or [],
)
)
Copy link
Member

Choose a reason for hiding this comment

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

it is cleaner with a comprehension

# generate
pconfs = {
    ptype: [
        load_plugin_config(pconf)
        for pconf self._config.get(ptype) or []
    ]
    for ptype in ["BACKEND_MODULES", "FRONTEND_MODULES", "MICRO_SERVICES"]:
}

# mutate
self._config.update(pconfs)


def _load_internal_attributes(self):
self._config["INTERNAL_ATTRIBUTES"] = self._parse_config(
self._config["INTERNAL_ATTRIBUTES"]
)

if not self._config["INTERNAL_ATTRIBUTES"]:
raise SATOSAConfigurationError("Could not load attribute mapping from 'INTERNAL_ATTRIBUTES.")
raise SATOSAConfigurationError(
"Could not load attribute mapping from 'INTERNAL_ATTRIBUTES."
Copy link
Member

Choose a reason for hiding this comment

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

A third different message for the same action. Can we make those messages similar?

)

def _verify_dict(self, conf):
"""
Expand All @@ -76,12 +101,14 @@ def _verify_dict(self, conf):
:param conf: config to verify
:return: None
"""
if not conf:
raise SATOSAConfigurationError("Missing configuration or unknown format")

for key in SATOSAConfig.mandatory_dict_keys:
if key not in conf:
raise SATOSAConfigurationError("Missing key '%s' in config" % key)
if not conf.get(key, None):
Copy link
Member

Choose a reason for hiding this comment

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

in .get(x, None), None is not needed

raise SATOSAConfigurationError(
"Missing key {key} or value for {key} in config".format(
key=key
)
)
Copy link
Member

Choose a reason for hiding this comment

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

If I have three keys missing, should I have to try three times to get the complete information?
Instead, can we turn this into an actual validator?


for key in SATOSAConfig.sensitive_dict_keys:
if key not in conf and "SATOSA_{key}".format(key=key) not in os.environ:
Expand Down
36 changes: 33 additions & 3 deletions tests/satosa/test_satosa_config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
from unittest.mock import mock_open, patch

import pytest
from satosa.exception import SATOSAConfigurationError

from satosa.exception import SATOSAConfigurationError
from satosa.satosa_config import SATOSAConfig
Expand All @@ -15,8 +14,8 @@ def non_sensitive_config_dict(self):
config = {
"BASE": "https://example.com",
"COOKIE_STATE_NAME": "TEST_STATE",
"BACKEND_MODULES": [],
"FRONTEND_MODULES": [],
"BACKEND_MODULES": [{"foo": "bar"}],
"FRONTEND_MODULES": [{"foo": "bar"}],
"INTERNAL_ATTRIBUTES": {"attributes": {}}
}
return config
Expand Down Expand Up @@ -73,3 +72,34 @@ def test_can_read_endpoint_configs_from_file(self, satosa_config_dict, modules_k

with pytest.raises(SATOSAConfigurationError):
SATOSAConfig(satosa_config_dict)

def test_missing_mandatory_dict_keys_raises_exception(self, satosa_config_dict):
for key in SATOSAConfig.mandatory_dict_keys:
patched_dict = satosa_config_dict
del patched_dict[key]
with pytest.raises(SATOSAConfigurationError):
SATOSAConfig(satosa_config_dict)

def test_empty_mandatory_dict_key_vals_raises_exception(self, satosa_config_dict):
for key in SATOSAConfig.mandatory_dict_keys:
patched_dict = satosa_config_dict
patched_dict[key] = None
with pytest.raises(SATOSAConfigurationError):
SATOSAConfig(satosa_config_dict)

def test_can_skip_unset_microservices(self, satosa_config_dict):
satosa_config_dict["MICRO_SERVICES"] = None
config = SATOSAConfig(satosa_config_dict)
assert config["MICRO_SERVICES"] == []

def test_invalid_internal_attributes_raises_exception(self, satosa_config_dict):
satosa_config_dict["INTERNAL_ATTRIBUTES"] = ["dummy.yaml"]
expected_config = {}

with patch("builtins.open", mock_open(read_data=json.dumps(expected_config))), \
pytest.raises(SATOSAConfigurationError):
SATOSAConfig(satosa_config_dict)

def test_invalid_conf_raises_exception(self):
with pytest.raises(SATOSAConfigurationError) as e:
SATOSAConfig({})