Access (auth) refactor & database transactions #46

jean-pasquier · 2020-11-19T20:41:07Z

Changes:

Move accesses functions from routes/accesses.py to crud/accesses.py
Use database transaction for atomicity for user/device creation

1st point lead to split crud module into submodules base (legacy) & accesses (new). I would need some feedback on this part. I needed to change patching during unit tests, fetch_all does not seem to work.

frgfm

Thanks for the PR! You can revert the changes on imports and crud patching in the unittests, that should solve most of the unittest issues!

frgfm · 2020-11-21T15:08:36Z

src/app/api/routes/users.py

@@ -26,14 +24,15 @@ async def update_my_info(payload: UserInfo, me: UserRead = Security(get_current_
 @router.put("/update-pwd", response_model=UserInfo)
 async def update_my_password(payload: Cred, me: UserRead = Security(get_current_user, scopes=["me"])):
    entry = await crud.get_entry(users, me.id)
-    await update_access_pwd(payload, entry["access_id"])
+    await crud.update_access_pwd(payload, entry["access_id"])


You forgot to do the same for the admin version further down below ;)

src/app/api/routes/users.py

src/app/db/tables.py

frgfm · 2020-11-21T15:11:48Z

src/app/db/tables.py

@@ -75,7 +75,7 @@ class EventType(str, enum.Enum):
    Column("id", Integer, primary_key=True),
    Column("login", String(50)),
    Column("owner_id", Integer, ForeignKey("users.id")),
-    Column("access_id", Integer, ForeignKey("accesses.id"), unique=True),
+    Column("access_id", Integer, ForeignKey("accesses.id", ondelete="CASCADE"), unique=True),


So deleting the device will delete the access? If so, shouldn't we do the same for the user?

And shall we do it also for the owner_id ? If we delete the users, the devices will disappear?

Yes I thought that I also had added the on delete cascade on user but I forgot.

I figured out that it is the opposite: deleting the access deletes the device/user. I am working on fixing the device/user deletion.

I will add the mechanism owner id as well !

Another issue when we use the on cascade mechanism -> as it perform at db level we have to find a way to mock it in our unit tests

frgfm · 2020-11-21T15:13:56Z

src/tests/test_accesses.py

-    monkeypatch.setattr(crud, "delete", pytest.mock_delete)
+    monkeypatch.setattr(base, "get", pytest.mock_get)
+    monkeypatch.setattr(base, "fetch_all", pytest.mock_fetch_all)
+    monkeypatch.setattr(base, "fetch_one", pytest.mock_fetch_one)


Since you made the crud init, you can keep the previous version (you have to patch the thin imported in the route, and does not import base, but crud)

Actually it is not that simple (I think, I am not masting import mechanisms) because it seams that mocking crud.get will keep untouched the original crud.base.get function and if it imported afterward the original is used instead of the mocked version. So far the only way I could mock all crud function was to mock both crud._ and crud.base._ (_ beeing each function). Do you see ?

Mmmh, what error did you get with my suggestion?

Because the import for the accesses is over here: https://github.com/pyronear/pyro-api/blob/master/src/app/api/routes/accesses.py#L3

So if we patch this very import, it should be working without any problem 🤔

frgfm · 2020-11-21T15:14:57Z

src/tests/test_accesses.py

@@ -62,100 +58,3 @@ def test_fetch_accesses(test_app, monkeypatch):
    assert response.status_code == 200
    assert response.json() == [{k: v for k, v in entry.items() if k != "hashed_password"}
                               for entry in mock_access_table]


If we remove the unittests for the routes, we need to adapt them to test the crud.accesses functions. I don't think it would be wise to decrease coverage by that much

frgfm · 2020-11-21T15:15:14Z

src/tests/test_alerts.py

+    monkeypatch.setattr(base, "fetch_all", pytest.mock_fetch_all)
+    monkeypatch.setattr(base, "post", pytest.mock_post)
+    monkeypatch.setattr(base, "put", pytest.mock_put)
+    monkeypatch.setattr(base, "delete", pytest.mock_delete)


florianriche · 2020-11-22T11:14:12Z

src/app/api/routes/devices.py


 router = APIRouter()


 @router.post("/", response_model=DeviceOut, status_code=201)
+@database.transaction()


Why add this decorator only for route that create objects?

I planned to add the decorator when transaction is needed (writing in several tables with atomicity), user/device creation was the first use case, did I miss other ones ?

I realised that route decorator was not the best practice: it makes it impossible to test. We may need to find a way to mock a database transaction or we'll have to rewrite operations into pytest mocks. @frgfm do you see what I mean ?

I am no where close to be knowledgeable about database transactions since I just discovered that thanks to you @jeanpasquier75. What is the expected behaviour difference after decorating the route? reducing execution time? output type change?

If it's beneficial to the API, even if we can't test it per se, I would argue that we should still put it. If it doesn't bring much and we can't test it, then perhaps let's leave it out

florianriche · 2020-11-22T11:14:50Z

src/app/api/routes/accesses.py

-async def create_access(payload: AccessAuth):
-    return await post_access(payload.login, payload.password, payload.scopes)
-
-
 @router.get("/{access_id}/", response_model=AccessRead)


If we want to remove the access routes (it is fine by me if we think we don't need it anymore for debugging purpose), why leave those routes?

I think he mentioned that we don't want to expose creation/modification of accesses but reading is fine

Yes reading is fine while it does not break database consistency

florianriche · 2020-11-22T11:17:11Z

src/app/db/tables.py

@@ -75,7 +75,7 @@ class EventType(str, enum.Enum):
    Column("id", Integer, primary_key=True),
    Column("login", String(50)),
    Column("owner_id", Integer, ForeignKey("users.id")),
-    Column("access_id", Integer, ForeignKey("accesses.id"), unique=True),
+    Column("access_id", Integer, ForeignKey("accesses.id", ondelete="CASCADE"), unique=True),


And shall we do it also for the owner_id ? If we delete the users, the devices will disappear?

frgfm · 2020-11-24T22:24:49Z

@jeanpasquier75 any update? I'm changing the release target as this isn't critical for the incoming demo

frgfm · 2020-11-30T19:57:42Z

So @jeanpasquier75, what should we do with this PR?
Considering the (very favorable) pace of PRs/modif of the repo, it's better to process PR rather quickly 😅

jean-pasquier · 2020-12-01T21:57:17Z

So @jeanpasquier75, what should we do with this PR?
Considering the (very favorable) pace of PRs/modif of the repo, it's better to process PR rather quickly 😅

Sorry for the delay 😅 I will dry to come up with something by the end of the week. As you mentioned in #76 this PR was supposed to fix access deletion with the SQL cascade mechanism as well

frgfm · 2020-12-05T11:52:31Z

@jeanpasquier75 #76 was merged, let us know if you think we should keep this PR for th upcoming release or staging it for the next one!

jean-pasquier · 2020-12-05T15:58:16Z

Closing this outdated PR resolved in #90

frgfm added 30 commits November 12, 2020 22:20

feat: Switched heartbeat from POST to PUT method type

7aef69d

feat: Switched location update route from POST to PUT type

6c19302

refactor: Removed type casting

5130624

test: Updated site unittests

fa52620

test: Extended scope of unittests on sites

4384407

feat: Merged master

5453380

style: Added extra typing

27f239d

test: Refactored site unittests

ed0968c

test: Updated events' unittests

cfa3fc0

test: Fixed assert in login unittests

1ebb949

test: Added unittest for security

12a01e7

refactor: Renamed route function

d178fa2

test: Fixed unittests naming

78613ce

test: Improved tests of events and sites

48b692b

test: Refactored unittests of accesses

db2cc2b

style: Fixed lint

8247ac0

style: Added extra typing

a2a66fd

style: Lint change

6298622

test: Refactored login route unittests

9b27c1e

test: Updated alerts unittests

5b6bdaa

test: Refactored media unittests

66cb5a1

test: Updated installation unittests

60e940e

refactor: Enforced password minimal length

cc70003

style: Renamed password update route function

2ad2126

refactor: Refactored routing functions

a0b5856

refactor: Refactored routes

ad4d0fb

test: Refactored users unittests

686a15b

style: Renamed mock tables

6fb9b21

style: Renamed monkeypatch

f3b8f1b

fix: Fixed query filter

25e21b1

frgfm and others added 9 commits November 14, 2020 17:03

test: Fixed assert error message

3b6127c

style: Specified arguments

a73533c

refactor: Refactored schemas

2b279a8

test: Reduced patching

6f10141

refactor: crud module split in submodules base & accesses

1476822

tests: update _patch_session to mock crud.base

edc8a13

feat: write accesses using db transactions

3d14b75

feat: db users/devices on delete cascade the access

be8b88d

Merge branch 'master' into access-refacto

23348ed

jean-pasquier added type: improvement New feature or request help wanted Extra attention is needed endpoint: users endpoint: cameras labels Nov 19, 2020

jean-pasquier added this to the 0.1.0 milestone Nov 19, 2020

jean-pasquier requested review from florianriche and frgfm November 19, 2020 20:41

jean-pasquier self-assigned this Nov 19, 2020

frgfm requested changes Nov 21, 2020

View reviewed changes

florianriche reviewed Nov 22, 2020

View reviewed changes

frgfm modified the milestones: 0.1.0, 0.1.1 Nov 24, 2020

frgfm added the question Further information is requested label Nov 28, 2020

jean-pasquier mentioned this pull request Dec 5, 2020

Crud refactoring & db transactions #90

Merged

jean-pasquier closed this Dec 5, 2020

jean-pasquier deleted the access-refacto branch December 5, 2020 16:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Access (auth) refactor & database transactions #46

Access (auth) refactor & database transactions #46

jean-pasquier commented Nov 19, 2020

frgfm left a comment

frgfm Nov 21, 2020

frgfm Nov 21, 2020

florianriche Nov 22, 2020

jean-pasquier Nov 23, 2020

jean-pasquier Nov 23, 2020

frgfm Nov 21, 2020

jean-pasquier Nov 23, 2020

frgfm Nov 23, 2020

frgfm Nov 21, 2020

frgfm Nov 21, 2020

florianriche Nov 22, 2020

jean-pasquier Nov 23, 2020

frgfm Nov 23, 2020

florianriche Nov 22, 2020

frgfm Nov 22, 2020

jean-pasquier Nov 23, 2020

florianriche Nov 22, 2020

frgfm commented Nov 24, 2020

frgfm commented Nov 30, 2020

jean-pasquier commented Dec 1, 2020

frgfm commented Dec 5, 2020

jean-pasquier commented Dec 5, 2020

Access (auth) refactor & database transactions #46

Access (auth) refactor & database transactions #46

Conversation

jean-pasquier commented Nov 19, 2020

frgfm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frgfm commented Nov 24, 2020

frgfm commented Nov 30, 2020

jean-pasquier commented Dec 1, 2020

frgfm commented Dec 5, 2020

jean-pasquier commented Dec 5, 2020