Skip to content

Update script set-up for fast API#11901

Closed
jimchamp wants to merge 1 commit into
masterfrom
fix-script-setup
Closed

Update script set-up for fast API#11901
jimchamp wants to merge 1 commit into
masterfrom
fix-script-setup

Conversation

@jimchamp

@jimchamp jimchamp commented Feb 19, 2026

Copy link
Copy Markdown
Collaborator

Follows #11816
Related comment: https://github.com/internetarchive/openlibrary/pull/11816/changes#r2830586065

Adds additional step during the script's initialization that exposes the get_current_user() method.

Other scripts that have similar initialization processes and depend on get_current_user() will almost certainly need to updated in order to run without error.

Technical

Testing

Screenshot

Stakeholders

Copilot AI review requested due to automatic review settings February 19, 2026 23:10

Copilot AI 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.

Pull request overview

This PR updates a migration script’s initialization so it correctly establishes the OpenLibrary “site” context needed by the newer FastAPI-compatible auth flow (specifically openlibrary.accounts.get_current_user() used by RunAs).

Changes:

  • Import setup_site from openlibrary.utils.request_context.
  • Call setup_site() during script setup after infogami._setup() to initialize the request-context site ContextVar.

@RayBB RayBB left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

@jimchamp good catch here.

I think in this case the way to go is going to be changing the get current user method or the runas class.

I'm not super familiar with how it works but we shouldn't need to complicate all the script by adding this since, as far as I know, they don't have a request context anyway.

My question is, does this script really need to get the 'current user' ? Is there even one?

The code makes me think we just need to create a user instance with a specific auth token.

@RayBB

RayBB commented Feb 19, 2026

Copy link
Copy Markdown
Collaborator

@jimchamp upon closer inspection I suspect we could create a site instance (what the setup script does) in the RunAs init. I'd need to research it a bit closer but that would track with how we are moving from threaded dicts to context_vars in general.

@RayBB

RayBB commented Feb 19, 2026

Copy link
Copy Markdown
Collaborator

A quick fix would be to have get_current_user check if web.ctx.site exists and use it if it does, otherwise use the context vars version.

The long term change will require some careful thinking.

@RayBB

RayBB commented Feb 19, 2026

Copy link
Copy Markdown
Collaborator

The three scripts that use RunAs are:

  • update_stale_ocaid_references.py
  • update_legacy_preferences.py
  • write_prefs_to_store.py

@mekarpeles

Copy link
Copy Markdown
Member

@RayBB these 3 scripts are relevant so if we're using RunAs or get current user (in an script), if you can help us make sure those still work / are ported, that would be a huge help.

@github-project-automation github-project-automation Bot moved this to Waiting Review/Merge from Staff in Ray's Project Feb 23, 2026
@jimchamp

jimchamp commented Feb 24, 2026

Copy link
Copy Markdown
Collaborator Author

I believe that Mek misunderstood what I was saying earlier today. Scripts in the /scripts/migrations directory will likely not be used multiple times.

I'm in the process of running write_prefs_to_store.py to migrate some older patron preference objects to a different table. This task should be completed later this week.

Closing this, but will apply these changes to the code in our cron jobs environment after the next deployment (IFF preferences remain to be migrated).

@jimchamp jimchamp closed this Feb 24, 2026
@jimchamp jimchamp deleted the fix-script-setup branch February 24, 2026 01:07
@RayBB RayBB removed this from Ray's Project Mar 4, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants