Skip to content

Conversation

@asottile-sentry
Copy link
Contributor

No description provided.

@asottile-sentry asottile-sentry changed the title Asottile django 5 ref: upgrade to django 5.0 Feb 1, 2024
@github-actions github-actions bot added the Scope: Backend Automatically applied to PRs that change backend components label Feb 1, 2024
@github-actions

This comment was marked as off-topic.

@github-actions

This comment was marked as off-topic.

@codecov
Copy link

codecov bot commented Feb 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2de2a21) 80.07% compared to head (966cd42) 81.33%.
Report is 3 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #64360      +/-   ##
==========================================
+ Coverage   80.07%   81.33%   +1.25%     
==========================================
  Files        5237     5218      -19     
  Lines      231064   230905     -159     
  Branches    45322    45303      -19     
==========================================
+ Hits       185025   187804    +2779     
+ Misses      40023    37249    -2774     
+ Partials     6016     5852     -164     
Files Coverage Δ
src/sentry/auth/providers/saml2/forms.py 41.93% <100.00%> (ø)
...c/sentry/auth/providers/saml2/onelogin/provider.py 83.33% <100.00%> (ø)
src/sentry/new_migrations/monkey/__init__.py 76.47% <100.00%> (ø)

... and 261 files with indirect coverage changes

Copy link
Member

@wedamija wedamija left a comment

Choose a reason for hiding this comment

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

Looks good to me, I just want to check that you verified that Queryset.update hasn't significantly changed, since we should update this if so

def update_with_returning(self, returned_fields: list[str], **kwargs):
"""
Copied and modified from `Queryset.update()` to support `RETURNING <returned_fields>`
"""
self._not_support_combined_queries("update") # type: ignore[attr-defined]
if self.query.is_sliced:
raise TypeError("Cannot update a query once a slice has been taken.")
self._for_write = True
query = self.query.chain(sql.UpdateQuery)
query.add_update_values(kwargs) # type: ignore[attr-defined]
# Inline annotations in order_by(), if possible.
new_order_by = []
for col in query.order_by:
if annotation := query.annotations.get(col):
if getattr(annotation, "contains_aggregate", False):
raise exceptions.FieldError(
f"Cannot update when ordering by an aggregate: {annotation}"
)
new_order_by.append(annotation)
else:
new_order_by.append(col)
query.order_by = tuple(new_order_by)
# Clear any annotations so that they won't be present in subqueries.
query.annotations = {}
with transaction.mark_for_rollback_on_error(using=self.db):
try:
query_sql, query_params = query.get_compiler(self.db).as_sql()
query_sql += f" RETURNING {', '.join(returned_fields)} "
using = router.db_for_write(self.model)
with connections[using].cursor() as cursor:
cursor.execute(query_sql, query_params)
result_ids = cursor.fetchall()
except EmptyResultSet:
# If Django detects that the query cannot return any results it'll raise
# EmptyResultSet before we even run the query. Catch it and just return an
# empty array of result ids
result_ids = []
self._result_cache = None
return result_ids

@asottile-sentry
Copy link
Contributor Author

Looks good to me, I just want to check that you verified that Queryset.update hasn't significantly changed, since we should update this if so

def update_with_returning(self, returned_fields: list[str], **kwargs):
"""
Copied and modified from `Queryset.update()` to support `RETURNING <returned_fields>`
"""
self._not_support_combined_queries("update") # type: ignore[attr-defined]
if self.query.is_sliced:
raise TypeError("Cannot update a query once a slice has been taken.")
self._for_write = True
query = self.query.chain(sql.UpdateQuery)
query.add_update_values(kwargs) # type: ignore[attr-defined]
# Inline annotations in order_by(), if possible.
new_order_by = []
for col in query.order_by:
if annotation := query.annotations.get(col):
if getattr(annotation, "contains_aggregate", False):
raise exceptions.FieldError(
f"Cannot update when ordering by an aggregate: {annotation}"
)
new_order_by.append(annotation)
else:
new_order_by.append(col)
query.order_by = tuple(new_order_by)
# Clear any annotations so that they won't be present in subqueries.
query.annotations = {}
with transaction.mark_for_rollback_on_error(using=self.db):
try:
query_sql, query_params = query.get_compiler(self.db).as_sql()
query_sql += f" RETURNING {', '.join(returned_fields)} "
using = router.db_for_write(self.model)
with connections[using].cursor() as cursor:
cursor.execute(query_sql, query_params)
result_ids = cursor.fetchall()
except EmptyResultSet:
# If Django detects that the query cannot return any results it'll raise
# EmptyResultSet before we even run the query. Catch it and just return an
# empty array of result ids
result_ids = []
self._result_cache = None
return result_ids

I didn't, but it doesn't look like it has received any updates that we care about. there's a proposal to make returning a thing but it hasn't been added yet

@wedamija
Copy link
Member

wedamija commented Feb 5, 2024

Looks good to me, I just want to check that you verified that Queryset.update hasn't significantly changed, since we should update this if so

def update_with_returning(self, returned_fields: list[str], **kwargs):
"""
Copied and modified from `Queryset.update()` to support `RETURNING <returned_fields>`
"""
self._not_support_combined_queries("update") # type: ignore[attr-defined]
if self.query.is_sliced:
raise TypeError("Cannot update a query once a slice has been taken.")
self._for_write = True
query = self.query.chain(sql.UpdateQuery)
query.add_update_values(kwargs) # type: ignore[attr-defined]
# Inline annotations in order_by(), if possible.
new_order_by = []
for col in query.order_by:
if annotation := query.annotations.get(col):
if getattr(annotation, "contains_aggregate", False):
raise exceptions.FieldError(
f"Cannot update when ordering by an aggregate: {annotation}"
)
new_order_by.append(annotation)
else:
new_order_by.append(col)
query.order_by = tuple(new_order_by)
# Clear any annotations so that they won't be present in subqueries.
query.annotations = {}
with transaction.mark_for_rollback_on_error(using=self.db):
try:
query_sql, query_params = query.get_compiler(self.db).as_sql()
query_sql += f" RETURNING {', '.join(returned_fields)} "
using = router.db_for_write(self.model)
with connections[using].cursor() as cursor:
cursor.execute(query_sql, query_params)
result_ids = cursor.fetchall()
except EmptyResultSet:
# If Django detects that the query cannot return any results it'll raise
# EmptyResultSet before we even run the query. Catch it and just return an
# empty array of result ids
result_ids = []
self._result_cache = None
return result_ids

I didn't, but it doesn't look like it has received any updates that we care about. there's a proposal to make returning a thing but it hasn't been added yet

Yeah, that seems to have been hanging around for years so I'm not holding my breath on it happening any time soon!

@asottile-sentry
Copy link
Contributor Author

Looks good to me, I just want to check that you verified that Queryset.update hasn't significantly changed, since we should update this if so

def update_with_returning(self, returned_fields: list[str], **kwargs):
"""
Copied and modified from `Queryset.update()` to support `RETURNING <returned_fields>`
"""
self._not_support_combined_queries("update") # type: ignore[attr-defined]
if self.query.is_sliced:
raise TypeError("Cannot update a query once a slice has been taken.")
self._for_write = True
query = self.query.chain(sql.UpdateQuery)
query.add_update_values(kwargs) # type: ignore[attr-defined]
# Inline annotations in order_by(), if possible.
new_order_by = []
for col in query.order_by:
if annotation := query.annotations.get(col):
if getattr(annotation, "contains_aggregate", False):
raise exceptions.FieldError(
f"Cannot update when ordering by an aggregate: {annotation}"
)
new_order_by.append(annotation)
else:
new_order_by.append(col)
query.order_by = tuple(new_order_by)
# Clear any annotations so that they won't be present in subqueries.
query.annotations = {}
with transaction.mark_for_rollback_on_error(using=self.db):
try:
query_sql, query_params = query.get_compiler(self.db).as_sql()
query_sql += f" RETURNING {', '.join(returned_fields)} "
using = router.db_for_write(self.model)
with connections[using].cursor() as cursor:
cursor.execute(query_sql, query_params)
result_ids = cursor.fetchall()
except EmptyResultSet:
# If Django detects that the query cannot return any results it'll raise
# EmptyResultSet before we even run the query. Catch it and just return an
# empty array of result ids
result_ids = []
self._result_cache = None
return result_ids

I didn't, but it doesn't look like it has received any updates that we care about. there's a proposal to make returning a thing but it hasn't been added yet

Yeah, that seems to have been hanging around for years so I'm not holding my breath on it happening any time soon!

🤞 yeahhhh -- though I've gotten to delete a lot of the hacky things we've got over time -- one day we'll have less of this stuff 😭

this avoids a transitional warning for updating the default from http

we can technically remove this assignment once we are on django 6
send_robust got way more complicated in django 5.x to handle async signals
@asottile-sentry asottile-sentry marked this pull request as ready for review February 5, 2024 22:45
@asottile-sentry asottile-sentry requested review from a team as code owners February 5, 2024 22:45
@asottile-sentry asottile-sentry requested a review from a team February 5, 2024 22:45
@asottile-sentry asottile-sentry merged commit c4dc41d into master Feb 6, 2024
@asottile-sentry asottile-sentry deleted the asottile-django-5 branch February 6, 2024 14:23
@github-actions github-actions bot locked and limited conversation to collaborators Feb 22, 2024
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.

4 participants