Skip to content
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

Deduped recieved scopes #6581

Merged
merged 6 commits into from
May 13, 2024
Merged

Deduped recieved scopes #6581

merged 6 commits into from
May 13, 2024

Conversation

Thingus
Copy link
Contributor

@Thingus Thingus commented May 8, 2024

Closes #6580

I have:

  • Formatted any Python files with black
  • Brought the branch up to date with master
  • Added any relevant Github labels
  • Added tests for any new additions
  • Added or updated any relevant documentation
  • Added an Architectural Decision Record (ADR), if appropriate
  • Added an MPLv2 License Header if appropriate
  • Updated the Changelog

Description

Passes the output of flowapi.permissions.query_to_scopes through a set() before returning

Copy link

cypress bot commented May 8, 2024

Passing run #22487 ↗︎

0 4 0 0 Flakiness 0

Details:

Merge branch 'master' into dedupe-recieved-scopes
Project: FlowAuth Commit: 5532acbc53
Status: Passed Duration: 00:46 💡
Started: May 13, 2024 3:20 PM Ended: May 13, 2024 3:21 PM

Review all test suite changes for PR #6581 ↗︎

@@ -210,7 +210,9 @@ async def query_to_scopes(query_dict):
except Exception:
raise BadQueryError
agg_unit = await get_agg_unit(query_dict)
return [f"{agg_unit}:{tl_query_name}:{query_name}" for query_name in query_list]
return list(
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this to be a list, or could we return a set instead? (I don't think it matters much either way, but if we return a set it's perhaps clearer that the scopes will be non-duplicated)

Copy link
Contributor Author

@Thingus Thingus May 9, 2024

Choose a reason for hiding this comment

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

It's compared to other lists when we check it against the requested scopes; now I say that, a set would be better there too, but that probably belongs in a different PR

@Thingus Thingus added the FlowAPI Issues related to the FlowKit API label May 9, 2024
Copy link
Member

@jc-harrison jc-harrison left a comment

Choose a reason for hiding this comment

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

👍

@Thingus Thingus added the ready-to-merge Label indicating a PR is OK to automerge label May 10, 2024
Copy link

codecov bot commented May 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.11%. Comparing base (3c06eed) to head (5532acb).

Additional details and impacted files
@@             Coverage Diff             @@
##           master    #6581       +/-   ##
===========================================
- Coverage   92.31%   77.11%   -15.20%     
===========================================
  Files         268      268               
  Lines       10586    10586               
  Branches      855      855               
===========================================
- Hits         9772     8163     -1609     
- Misses        676     2169     +1493     
- Partials      138      254      +116     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mergify mergify bot merged commit 7488c12 into master May 13, 2024
40 of 41 checks passed
@mergify mergify bot deleted the dedupe-recieved-scopes branch May 13, 2024 15:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FlowAPI Issues related to the FlowKit API ready-to-merge Label indicating a PR is OK to automerge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Same time queries producing duplicate scopes
2 participants