How to ignore everybody, carve your own path #1
Conversation
pmarecki
left a comment
pmarecki
left a comment
There was a problem hiding this comment.
... with limited time comes a limited review; enjoy and argue if you think I'm wrong!
| @@ -0,0 +1,4 @@ | |||
| class SecurityBadRequest(Exception): | |||
There was a problem hiding this comment.
why separate files for each Exception?
| @@ -0,0 +1,4 @@ | |||
| class SecurityUnauthorized(Exception): | |||
There was a problem hiding this comment.
Not a good idea to have our specific exceptions all inherit from some generic type (I know, I have it too...); this way we cannot catch "all errors related to our system"; would be better to have "SecurityException", and then these two to inherit from it.
| @@ -0,0 +1,15 @@ | |||
| from sesja.models.securityManager import SecurityManager | |||
There was a problem hiding this comment.
unpythonic convention on file names; they should be all-lowercase, with underscores if really needed, https://peps.python.org/pep-0008/
| @@ -0,0 +1,29 @@ | |||
| class SecurityGroup: | |||
| keys: set[int] | |||
There was a problem hiding this comment.
I'm not a fan of these "type hints" written as if they were class variables; I'd keep them in constructor only.
| group_id: int | ||
|
|
||
| def __init__(self, gid: int): | ||
| self.keys = set() |
There was a problem hiding this comment.
these fields should not be public; you don't want sb to access them if he has reference to an instance of SecurityGroup... I guess we will see that in the tests...
| group = SecurityGroup(3) | ||
| key = Key(1, "MARY'S KEY") | ||
| s_manager = SecurityManager() | ||
| s_manager.create_group_example() |
There was a problem hiding this comment.
meh... in this PR... I don't se the SecurityManager having a method .create_group_example()... maybe I missed it somehow... however...
-- SecurityManager should never have such methods intended for tests only; you can add them to a separate module like test_utils.py or sth.
I recall I've seen class methods with something like @Testmethod annotations effectively making them private everywhere but in the tests, but I'm not a fan of such solutions (even if I wrote such...)
| def test_can_not_remove_key(self): | ||
| mgr = SecurityManager() | ||
| mgr.add_key_to_group(0, 1, 0) | ||
| self.assertRaises(SecurityUnauthorized, lambda: mgr.remove_key_from_group(2 * random.randint(1, 10 ** 3), 1, 0)) |
There was a problem hiding this comment.
not very clear from the test why it should raise SecUnauthorized; if you mean that the executor_id is not in the admin group -- change this tests name (e.g. non_admin_cannot_remove_groups); and also there is no need for some random executor id; just use "2" or sth; no need to have some random factors when they are not needed
| def test_example_group(self): | ||
| s_manager = SecurityManager() | ||
| s_manager.create_group_example() | ||
| print(len(s_manager.groups)) |
There was a problem hiding this comment.
you should never leave print in tests in the final code for code review; it's fine to play, but remove them when you're finished.
| s_manager.add_key_to_group(2, 1, 0) | ||
|
|
||
| s_manager.remove_key_from_group(2, 1, 0) | ||
| assert s_manager.groups[0].keys == set() |
There was a problem hiding this comment.
so... you are pulling the internals (.groups, and then .keys) from references; as mentioned earlier -- they should have been private; not sure what the assert shoud check; if you want to assert the set is empty use len(key) == 0,
s = set()
s.add(10)
s.remove(10)
assert len(s) == 0 # True
better yet -- since we want these to be private, add methods is_empty() where needed, and access the private sets/dicts from within them.
| @@ -1,56 +0,0 @@ | |||
| import unittest | |||
There was a problem hiding this comment.
no tests for LockManager ?
2 participants
Why does python live on land?? Because it is above C level.