-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix: Avoid accessing user when running migrations #27708
Conversation
try: | ||
user = User.objects.filter(last_login__isnull=False).order_by("-last_login").first() | ||
except: | ||
user = None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this try/except
be catching that error? Also, the error happens when loading posthog_team
, so this is kinda weird, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error is caused by local_api_key = get_self_capture_api_token(user)
Good point about try/except
, though. I simplified the PR 6364cf1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like it was introduced in #27669. I'll leave to @skoob13 and @pauldambra to review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, it looks like 6364cf1 would break the fallback behavior:
Lines 500 to 515 in 6364cf1
def get_self_capture_api_token(user: Optional[Union["AbstractBaseUser", "AnonymousUser"]]) -> Optional[str]: | |
from posthog.models import Team | |
# Get the current user's team (or first team in the instance) to set self capture configs | |
team: Optional[Team] = None | |
if user and getattr(user, "team", None): | |
team = user.team # type: ignore | |
else: | |
try: | |
team = Team.objects.only("api_token").first() | |
except Exception: | |
pass | |
if team: | |
return team.api_token | |
return None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted back to the original behavior because I think bypassing any user config is a more appropriate fix for the migration context. Waiting for additional input before landing, though.
Size Change: 0 B Total Size: 1.16 MB ℹ️ View Unchanged
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will leave the final review to the other guys tagged in the PR, but this LGTM
This reverts commit 6364cf1.
I think the environment variable is unnecessary because this code is supposed to run only in debugging |
See https://posthog.slack.com/archives/C0113360FFV/p1737412494845589
Changes
Fixes this issue when running
DEBUG=1 ./bin/migrate
:The app crashes during bootstrap because the column doesn't exist yet. It seems like it should be fine to avoid loading user context when a migration is running, though.
How did you test this code?
Verified the migration ran as expected and that the application still loaded as expected.