Skip to content

fix: Slice on unevaluated QuerySet for Organization.has_single_owner#55715

Merged
dashed merged 1 commit into
masterfrom
hybrid-cloud/fix-has_single_owner
Sep 5, 2023
Merged

fix: Slice on unevaluated QuerySet for Organization.has_single_owner#55715
dashed merged 1 commit into
masterfrom
hybrid-cloud/fix-has_single_owner

Conversation

@dashed

@dashed dashed commented Sep 5, 2023

Copy link
Copy Markdown
Member

Found this while fixing tests for split silos.

This was a regression from 131596d, and the implementation of def has_single_owner had this optimization prior to the refactor: https://github.com/getsentry/sentry/blame/1acfc6443fa031fb932115c69ddbe12c01bd8b8e/src/sentry/models/organization.py#L310-L316

Slicing on unevaluated QuerySet would add the appropriate LIMIT keywords on the generated SQL query:

@dashed dashed requested review from a team September 5, 2023 21:25
@dashed dashed self-assigned this Sep 5, 2023
@dashed dashed requested a review from a team as a code owner September 5, 2023 21:25
@@ -346,9 +346,11 @@ def default_owner_id(self):

def has_single_owner(self):
owners = list(

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Here, owners should return at most 2 records.

@github-actions github-actions Bot added the Scope: Backend Automatically applied to PRs that change backend components label Sep 5, 2023

@cathteng cathteng left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

thanks for catching this!

@codecov

codecov Bot commented Sep 5, 2023

Copy link
Copy Markdown

Codecov Report

Merging #55715 (964e010) into master (ea14f74) will decrease coverage by 0.01%.
Report is 16 commits behind head on master.
The diff coverage is 85.00%.

@@            Coverage Diff             @@
##           master   #55715      +/-   ##
==========================================
- Coverage   80.13%   80.12%   -0.01%     
==========================================
  Files        5049     5051       +2     
  Lines      216398   216505     +107     
  Branches    36617    36647      +30     
==========================================
+ Hits       173412   173481      +69     
- Misses      37682    37706      +24     
- Partials     5304     5318      +14     
Files Changed Coverage
...app/components/profiling/flamegraph/flamegraph.tsx ø
static/app/utils/profiling/units/units.ts ø
static/app/utils/replays/replayReader.tsx ø
static/app/views/monitors/details.tsx ø
.../performance/landing/views/allTransactionsView.tsx ø
...pp/views/performance/landing/views/backendView.tsx ø
...e/landing/widgets/components/performanceWidget.tsx ø
...ance/landing/widgets/components/widgetChartRow.tsx ø
...rmance/landing/widgets/components/widgetHeader.tsx ø
...ing/widgets/transforms/transformTrendsDiscover.tsx ø
... and 13 more

@dashed dashed merged commit c690e58 into master Sep 5, 2023
@dashed dashed deleted the hybrid-cloud/fix-has_single_owner branch September 5, 2023 22:06
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Scope: Backend Automatically applied to PRs that change backend components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants