[Groups] Add group logic to remaining routes by florianriche · Pull Request #167 · pyronear/pyro-api

florianriche · 2021-04-30T20:49:31Z

Context

This PR is the last part of the group implementation. After having implemented the group logic for the fetching route, we have added the group logic to the remaining routes.

Content:

Group logic in Sites
Group logic in Installations
Group logic in Alerts
Group logic in Events
Group logic in Media

Routes for which I have a strong doubt have a # TODO comment in them.

The commits are separated to ease the review, there is one commit per object.

This PR should close #125

codecov · 2021-04-30T20:57:21Z

Codecov Report

Merging #167 (1d4e59b) into master (99417e3) will decrease coverage by 0.07%.
The diff coverage is 90.09%.

@@            Coverage Diff             @@
##           master     #167      +/-   ##
==========================================
- Coverage   91.09%   91.02%   -0.08%     
==========================================
  Files          34       34              
  Lines        1191     1248      +57     
==========================================
+ Hits         1085     1136      +51     
- Misses        106      112       +6

Flag	Coverage Δ
unittests	`91.02% <90.09%> (-0.08%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/app/api/crud/groups.py	`87.50% <56.25%> (-4.17%)`	⬇️
src/app/api/routes/media.py	`86.30% <69.23%> (-1.94%)`	⬇️
src/app/api/routes/alerts.py	`98.76% <100.00%> (+0.28%)`	⬆️
src/app/api/routes/devices.py	`100.00% <100.00%> (ø)`
src/app/api/routes/events.py	`100.00% <100.00%> (ø)`
src/app/api/routes/installations.py	`100.00% <100.00%> (ø)`
src/app/api/routes/sites.py	`100.00% <100.00%> (ø)`
src/app/api/routes/users.py	`100.00% <100.00%> (ø)`

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 99417e3...1d4e59b. Read the comment docs.

frgfm

Thanks for the PR!
Overall, I think we only need to change route signature formatting (stick to black when it's not a one-liner), and most importantly, optimize the group checking part of this PR. I added a few comments for this

frgfm · 2021-05-01T10:12:28Z

+    if entry is None:
+        return None
    return entry["group_id"]


Three things to consider :

if we keep this logic, the implementation is fine by me, we can also switch to a one-liner

return entry['group_id'] if isinstance(entry, dict) else None

only a question but : shouldn't we raise an error if the entry-id doesn't exist?

if we want to start optimizing SQL queries, here we actually only need the group_id field

frgfm · 2021-05-01T10:13:59Z

+async def get_alert(alert_id: int = Path(..., gt=0),
+                    requester=Security(get_current_access,
+                    scopes=[AccessType.admin, AccessType.user])):


Let's keep black formatting when we have several lines:

async def get_alert( alert_id: int = Path(..., gt=0), requester=Security(get_current_access, _=[AccessType.admin, AccessType.user]) ):

Not sure I get it, didn't you ask in another review to have several lines in those cases?

Sure, but 1 line per function argument
Besides, I think we can annotation typing to requester

frgfm · 2021-05-01T10:14:53Z

+    requested_group_id = await get_entity_group_id(alerts, alert_id)
+    await check_group_read(requester.id, requested_group_id)
    return await crud.get_entry(alerts, alert_id)


over these 3 lines, we are querying the alerts table twice, we should try to improve this I think

Only way I see to slightly speed up the process with the current implementation we have would be to do

retrieved_alert = await crud.get_entry(alerts, alert_id) requested_group_id = await get_entity_group_id(devices, retrieved_alert["device_id"]) await check_group_read(requester.id, requested_group_id) return retrieved_alert

but not sure it is worth it, it makes the code harder to read and not very logical.

frgfm · 2021-05-01T10:15:22Z

+    requested_group_id = await get_entity_group_id(alerts, alert_id)
+    await check_group_update(requester.id, requested_group_id)
    return await crud.update_entry(alerts, payload, alert_id)


same suggestion as above roughly, not sure we can really speed up the process

frgfm · 2021-05-01T10:16:31Z

    """
    Based on a site_id, updates information about the specified site
    """
+    # TODO: validate this one


validate the general design of group checking you mean?

Validate that the route need to haveccess restrcted to group

frgfm · 2021-05-01T10:17:33Z

        [2, 1, 401, "Permission denied"],
        [1, 999, 404, "Entry not found"],
        [1, 0, 422, None],
+        [4, 1, 401, "You can't access this ressource"],


A "Permission denied" would be better to unify error messages

HMm, I personnally think that it would be helpful for the user to let him know that he is allowed to use that route but that he should check its input

Perhaps let's make it a more detailed message then, because I understand it the same way as "Permission denied" (which can be applied to route or sub entry getting)

What I mean is: the route will fail anyway, the user won't know where and in one case that person will get "Permission denied" and another "You can't access this resource". I would understand that they meant there is a difference but I wouldn't have any clue what it is 😅

frgfm

Thanks for the PR! If you feel like it, I think we need to add a good explanation of how access is handled in the API in our README. While our documentation is very detailed for each route, the general process might sound obscure to most

florianriche added 7 commits April 29, 2021 22:42

feat: add group logic to alerts

00db6ce

feat: handle nonexisting entries in get_entity_group_id

917ac69

feat: add group logic to events

e7bb171

feat: add group logic to installations

29148b9

feat: add group logic to media

689e5bd

feat: add group logic to site

81423da

chore: flake8

b9e2c00

florianriche requested review from fe51, frgfm and jean-pasquier and removed request for fe51, frgfm and jean-pasquier April 30, 2021 20:49

florianriche self-assigned this Apr 30, 2021

florianriche added type: improvement New feature or request endpoint: detections endpoint: events endpoint: groups endpoint: installations endpoint: media endpoint: sites labels Apr 30, 2021

frgfm reviewed May 1, 2021

View reviewed changes

frgfm added this to the 0.1.2 milestone May 1, 2021

florianriche added 2 commits May 3, 2021 23:43

chore: black formatting

f0a03e7

chore: black formatting (2)

1d4e59b

frgfm approved these changes May 5, 2021

View reviewed changes

frgfm mentioned this pull request May 7, 2021

[docs] Added explanations about access management in the API #168

Closed

frgfm merged commit db53279 into master May 7, 2021

frgfm deleted the add_group_logic branch May 7, 2021 13:31

frgfm added the ext: tests label Jun 16, 2022

Uh oh!

Uh oh!

Conversation

florianriche commented Apr 30, 2021

Context

Content:

Uh oh!

codecov Bot commented Apr 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

frgfm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

florianriche May 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

frgfm left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented Apr 30, 2021 •

edited

Loading

florianriche May 3, 2021 •

edited

Loading