-
Notifications
You must be signed in to change notification settings - Fork 58
fix(python): add org_id to prompt cache key for cross-org isolation #1268
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1678,13 +1678,14 @@ def compute_metadata(): | |
| eprint(f"Failed to load prompt, attempting to fall back to cache: {server_error}") | ||
| try: | ||
| if id: | ||
| return _state._prompt_cache.get(id=id) | ||
| return _state._prompt_cache.get(id=id, org_id=_state.org_id) | ||
| else: | ||
| return _state._prompt_cache.get( | ||
| slug, | ||
| version=str(version) if version else "latest", | ||
| project_id=project_id, | ||
| project_name=project, | ||
| org_id=_state.org_id, | ||
| ) | ||
| except Exception as cache_error: | ||
| if id: | ||
|
|
@@ -1714,6 +1715,7 @@ def compute_metadata(): | |
| _state._prompt_cache.set( | ||
| prompt, | ||
| id=id, | ||
| org_id=_state.org_id, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here combine with the next statement |
||
| ) | ||
| elif slug: | ||
| _state._prompt_cache.set( | ||
|
|
@@ -1722,6 +1724,7 @@ def compute_metadata(): | |
| version=str(version) if version else "latest", | ||
| project_id=project_id, | ||
| project_name=project, | ||
| org_id=_state.org_id, | ||
| ) | ||
| except Exception as e: | ||
| eprint(f"Failed to store prompt in cache: {e}") | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ | |
| 2. A persistent disk-based cache that serves as a backing store | ||
|
|
||
| This allows for efficient prompt retrieval while maintaining persistence across sessions. | ||
| The cache is keyed by project identifier (ID or name), prompt slug, and version. | ||
| The cache is keyed by organization ID, project identifier (ID or name), prompt slug, and version. | ||
| """ | ||
|
|
||
|
|
||
|
|
@@ -15,22 +15,32 @@ | |
|
|
||
|
|
||
| def _create_cache_key( | ||
| org_id: str | None, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if org_id is required to prevent cross org issues, it should be required (e.g. not none) |
||
| project_id: str | None, | ||
| project_name: str | None, | ||
| slug: str | None, | ||
| version: str = "latest", | ||
| id: str | None = None, | ||
| ) -> str: | ||
| """Creates a unique cache key from project identifier, slug and version, or from ID.""" | ||
| """Creates a unique cache key from org ID, project identifier, slug and version, or from prompt ID. | ||
|
|
||
| The org_id is included to ensure cache isolation between organizations. Without it, | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i think the org_id can always be in the cache key |
||
| two organizations with the same project name and prompt slug could get each other's | ||
| cached prompts, leading to incorrect prompt retrieval. | ||
| """ | ||
| if id: | ||
| # When caching by ID, we don't need project or slug | ||
| # When caching by ID, we don't need project or slug (IDs are globally unique) | ||
| return f"id:{id}" | ||
|
|
||
| prefix = project_id or project_name | ||
| if not prefix: | ||
| raise ValueError("Either project_id or project_name must be provided") | ||
| if not slug: | ||
| raise ValueError("Slug must be provided when not using ID") | ||
|
|
||
| # Include org_id in cache key if available to ensure cross-org isolation | ||
| if org_id: | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. looks like login is called L1696 before updating/setting the prompt. you should be able to assume org_id exists .. perhaps raise if not set |
||
| return f"{org_id}:{prefix}:{slug}:{version}" | ||
| return f"{prefix}:{slug}:{version}" | ||
|
|
||
|
|
||
|
|
@@ -65,6 +75,7 @@ def get( | |
| project_id: str | None = None, | ||
| project_name: str | None = None, | ||
| id: str | None = None, | ||
| org_id: str | None = None, | ||
| ) -> prompt.PromptSchema: | ||
| """ | ||
| Retrieve a prompt from the cache. | ||
|
|
@@ -75,6 +86,7 @@ def get( | |
| project_id: The ID of the project containing the prompt. | ||
| project_name: The name of the project containing the prompt. | ||
| id: The ID of a specific prompt. If provided, slug and project parameters are ignored. | ||
| org_id: The ID of the organization. Used to ensure cache isolation between orgs. | ||
|
|
||
| Returns: | ||
| The cached Prompt object. | ||
|
|
@@ -83,7 +95,7 @@ def get( | |
| ValueError: If neither project_id nor project_name is provided (when not using id). | ||
| KeyError: If the prompt is not found in the cache. | ||
| """ | ||
| cache_key = _create_cache_key(project_id, project_name, slug, version, id) | ||
| cache_key = _create_cache_key(org_id, project_id, project_name, slug, version, id) | ||
|
|
||
| # First check memory cache. | ||
| try: | ||
|
|
@@ -111,6 +123,7 @@ def set( | |
| project_id: str | None = None, | ||
| project_name: str | None = None, | ||
| id: str | None = None, | ||
| org_id: str | None = None, | ||
| ) -> None: | ||
| """ | ||
| Store a prompt in the cache. | ||
|
|
@@ -122,12 +135,13 @@ def set( | |
| project_id: The ID of the project containing the prompt. | ||
| project_name: The name of the project containing the prompt. | ||
| id: The ID of a specific prompt. If provided, slug and project parameters are ignored. | ||
| org_id: The ID of the organization. Used to ensure cache isolation between orgs. | ||
|
|
||
| Raises: | ||
| ValueError: If neither project_id nor project_name is provided (when not using id). | ||
| RuntimeError: If there is an error writing to the disk cache. | ||
| """ | ||
| cache_key = _create_cache_key(project_id, project_name, slug, version, id) | ||
| cache_key = _create_cache_key(org_id, project_id, project_name, slug, version, id) | ||
|
|
||
| # Update memory cache. | ||
| self.memory_cache.set(cache_key, value) | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
you can revert this line since id will always win maybe even combine the next statement for the same reason into one