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.

Sign up for GitHub

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

Closed
wants to merge 3 commits into from
Closed

fix: Avoid accessing user when running migrations #27708

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
63e25d9
Avoid accessing user when running migrations
danielbachhuber Jan 20, 2025
6364cf1
Move `get_self_capture_api_token` inside of try, catch
danielbachhuber Jan 20, 2025
60ed602
Revert "Move `get_self_capture_api_token` inside of try, catch"
danielbachhuber Jan 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions bin/migrate
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ set -e

SCRIPT_DIR=$(dirname "$(readlink -f "$0")")

export POSTHOG_RUNNING_MIGRATION=true
# NOTE when running in docker, rust might not exist so we need to check for it
if [ -d "$SCRIPT_DIR/../rust" ]; then
bash $SCRIPT_DIR/../rust/bin/migrate-cyclotron
Expand All @@ -26,3 +27,5 @@ python manage.py run_async_migrations --complete-noop-migrations
python manage.py run_async_migrations --check

wait $(jobs -p) # Make sure CH migrations are done before we exit

unset POSTHOG_RUNNING_MIGRATION
25 changes: 13 additions & 12 deletions posthog/apps.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,18 +48,19 @@ def ready(self):
{"git_rev": get_git_commit_short(), "git_branch": get_git_branch()},
)

try:
user = User.objects.filter(last_login__isnull=False).order_by("-last_login").first()
except:
user = None
Comment on lines -51 to -54
Copy link
Member

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?

Copy link
Contributor Author

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

Copy link
Contributor Author

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

Copy link
Contributor Author

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:

posthog/posthog/utils.py

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

Copy link
Contributor Author

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.


local_api_key = get_self_capture_api_token(user)

if SELF_CAPTURE and local_api_key:
posthoganalytics.api_key = local_api_key
posthoganalytics.host = settings.SITE_URL
else:
posthoganalytics.disabled = True
if not os.getenv("POSTHOG_RUNNING_MIGRATION"):
try:
user = User.objects.filter(last_login__isnull=False).order_by("-last_login").first()
except:
user = None

local_api_key = get_self_capture_api_token(user)

if SELF_CAPTURE and local_api_key:
posthoganalytics.api_key = local_api_key
posthoganalytics.host = settings.SITE_URL
else:
posthoganalytics.disabled = True

# load feature flag definitions if not already loaded
if not posthoganalytics.disabled and posthoganalytics.feature_flag_definitions() is None:
Expand Down
Loading