Repository navigation
Treat an absent storyboard key as no change #203
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
Changes from 9 commits
6ec31b0
b50f5a2
0b68982
0186153
a644239
b240c85
0f7ad82
63e67fa
7e33900
90c5d0d
80d0221
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 | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -65,17 +65,15 @@ | |||||||||||||||||
| from werkzeug.exceptions import RequestEntityTooLarge | ||||||||||||||||||
| from flask_cors import CORS | ||||||||||||||||||
| from google.api_core import exceptions as google_exceptions | ||||||||||||||||||
| from google.auth import compute_engine | ||||||||||||||||||
| from google.auth import default as google_auth_default | ||||||||||||||||||
| from google.auth.transport import requests as google_auth_requests | ||||||||||||||||||
| from google.cloud import firestore | ||||||||||||||||||
| from google.cloud.firestore_v1 import field_path | ||||||||||||||||||
| from google.cloud import storage | ||||||||||||||||||
| from google.oauth2 import id_token as google_id_token | ||||||||||||||||||
| import orchestrator | ||||||||||||||||||
| import transcription | ||||||||||||||||||
| from util import database as util_database | ||||||||||||||||||
| from util import errors as util_errors | ||||||||||||||||||
| from util import gcs_wrapper | ||||||||||||||||||
| from util import model_allowlist | ||||||||||||||||||
| from util import submission_validation | ||||||||||||||||||
| from werkzeug.security import safe_join | ||||||||||||||||||
|
|
@@ -692,9 +690,8 @@ def get_status_handler() -> flask_response: | |||||||||||||||||
| # --------------------------------------------------------------------------- | ||||||||||||||||||
| # Mediated data plane (ROLE='app'): signed-URL minting for GCS plus CRUD on | ||||||||||||||||||
| # the UI Firestore database, so the SPA can run without direct Firestore or | ||||||||||||||||||
| # Storage access. Module-level lazy singletons keep the per-request cost at | ||||||||||||||||||
| # ~1 IAM signBlob RPC per unique path (the per-call construction in | ||||||||||||||||||
| # util.gcs_wrapper.get_signed_url costs ~3 RPCs). | ||||||||||||||||||
| # Storage access. Storage client and IAM signing credentials delegate to the | ||||||||||||||||||
| # cached signing context in util.gcs_wrapper. | ||||||||||||||||||
| # --------------------------------------------------------------------------- | ||||||||||||||||||
|
|
||||||||||||||||||
| _SIGNED_GET_TTL = datetime.timedelta(hours=24) | ||||||||||||||||||
|
|
@@ -732,17 +729,7 @@ def _is_allowed_upload_content_type(content_type: str) -> bool: | |||||||||||||||||
| _ALLOWED_UPLOAD_TYPE_PREFIXES | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
| _storage_client = None | ||||||||||||||||||
| _ui_db = None | ||||||||||||||||||
| _signing_credentials = None | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| def _get_storage_client() -> storage.Client: | ||||||||||||||||||
| """Returns the lazily-built module-level Cloud Storage client.""" | ||||||||||||||||||
| global _storage_client | ||||||||||||||||||
| if _storage_client is None: | ||||||||||||||||||
| _storage_client = storage.Client() | ||||||||||||||||||
| return _storage_client | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| def _get_ui_db() -> firestore.Client | None: | ||||||||||||||||||
|
|
@@ -759,39 +746,19 @@ def _get_ui_db() -> firestore.Client | None: | |||||||||||||||||
| return _ui_db | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| def _get_signing_credentials() -> compute_engine.IDTokenCredentials: | ||||||||||||||||||
| """Returns cached IAM-signing credentials, rebuilding them on expiry. | ||||||||||||||||||
|
|
||||||||||||||||||
| Inlines the flask-context mechanics of util.gcs_wrapper.get_signed_url: | ||||||||||||||||||
| the metadata-server credentials carry no private key, so URL signing | ||||||||||||||||||
| goes through the IAM signBlob API via compute_engine.IDTokenCredentials | ||||||||||||||||||
| (requires roles/iam.serviceAccountTokenCreator on the runtime SA). | ||||||||||||||||||
| """ | ||||||||||||||||||
| global _signing_credentials | ||||||||||||||||||
| if _signing_credentials is None or _signing_credentials.expired: | ||||||||||||||||||
| auth_request = google_auth_requests.Request() | ||||||||||||||||||
| source_credentials, _ = google_auth_default() | ||||||||||||||||||
| source_credentials.refresh(auth_request) | ||||||||||||||||||
| _signing_credentials = compute_engine.IDTokenCredentials( | ||||||||||||||||||
| auth_request, | ||||||||||||||||||
| '', | ||||||||||||||||||
| service_account_email=source_credentials.service_account_email, # pyright: ignore[reportAttributeAccessIssue] | ||||||||||||||||||
| ) | ||||||||||||||||||
| return _signing_credentials | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
| def _signed_url( | ||||||||||||||||||
| blob, method: str, expiration: datetime.timedelta, content_type=None | ||||||||||||||||||
| ) -> str: | ||||||||||||||||||
| """Generates a V4 signed URL for the blob with the cached credentials.""" | ||||||||||||||||||
| """Generates a V4 signed URL for the blob with cached IAM credentials.""" | ||||||||||||||||||
| _, credentials = gcs_wrapper.get_signing_context() | ||||||||||||||||||
| kwargs = {} | ||||||||||||||||||
| if content_type is not None: | ||||||||||||||||||
| kwargs['content_type'] = content_type | ||||||||||||||||||
| return blob.generate_signed_url( | ||||||||||||||||||
| version='v4', | ||||||||||||||||||
| expiration=expiration, | ||||||||||||||||||
| method=method, | ||||||||||||||||||
| credentials=_get_signing_credentials(), | ||||||||||||||||||
| credentials=credentials, | ||||||||||||||||||
| **kwargs, | ||||||||||||||||||
| ) | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -999,7 +966,8 @@ def upload_url_handler() -> flask_response: | |||||||||||||||||
| if not bucket_name: | ||||||||||||||||||
| return _json_error('gcsBucket not configured', 500) | ||||||||||||||||||
| object_path = f'{prefix}/{file_name}' | ||||||||||||||||||
| blob = _get_storage_client().bucket(bucket_name).blob(object_path) | ||||||||||||||||||
| storage_client, _ = gcs_wrapper.get_signing_context() | ||||||||||||||||||
| blob = storage_client.bucket(bucket_name).blob(object_path) | ||||||||||||||||||
| exists = blob.exists() | ||||||||||||||||||
| upload_url = None | ||||||||||||||||||
| if not exists: | ||||||||||||||||||
|
|
@@ -1061,7 +1029,8 @@ def sign_url_handler() -> flask_response: | |||||||||||||||||
| if not ttl_arg.isdecimal() or not 1 <= int(ttl_arg) <= 86400: | ||||||||||||||||||
| return _json_error('ttl must be 1-86400 seconds', 400) | ||||||||||||||||||
| ttl = datetime.timedelta(seconds=int(ttl_arg)) | ||||||||||||||||||
| bucket = _get_storage_client().bucket(bucket_name) | ||||||||||||||||||
| storage_client, _ = gcs_wrapper.get_signing_context() | ||||||||||||||||||
| bucket = storage_client.bucket(bucket_name) | ||||||||||||||||||
| urls = { | ||||||||||||||||||
| path: _signed_url(bucket.blob(path), 'GET', ttl) for path in paths | ||||||||||||||||||
| } | ||||||||||||||||||
|
|
@@ -1206,22 +1175,24 @@ def _write_project_doc( | |||||||||||||||||
| precondition, scene writes retain their existing set behavior, and the | ||||||||||||||||||
| stale-scene prune scan is skipped: a brand-new project has no prior scenes | ||||||||||||||||||
| to prune, and callers cap create at _MAX_CREATE_SCENES so this always fits | ||||||||||||||||||
| one atomic batch. | ||||||||||||||||||
| one atomic batch. When create is false, an omitted storyboard key leaves the | ||||||||||||||||||
| scenes subcollection untouched. | ||||||||||||||||||
| """ | ||||||||||||||||||
| scenes = payload.get('storyboard') | ||||||||||||||||||
| if not isinstance(scenes, list): | ||||||||||||||||||
| scenes = [] | ||||||||||||||||||
| root = dict(payload) | ||||||||||||||||||
| root['storyboard'] = [] | ||||||||||||||||||
| scenes_ref = doc_ref.collection(_SCENES_SUBCOLLECTION) | ||||||||||||||||||
| ops = [('create' if create else 'set', doc_ref, root)] | ||||||||||||||||||
| for index, scene in enumerate(scenes): | ||||||||||||||||||
| ops.append(('set', scenes_ref.document(_scene_doc_id(index)), scene)) | ||||||||||||||||||
| if not create: | ||||||||||||||||||
| keep_ids = {_scene_doc_id(i) for i in range(len(scenes))} | ||||||||||||||||||
| for snapshot in scenes_ref.stream(): | ||||||||||||||||||
| if snapshot.id not in keep_ids: | ||||||||||||||||||
| ops.append(('delete', scenes_ref.document(snapshot.id), None)) | ||||||||||||||||||
| if create or 'storyboard' in payload: | ||||||||||||||||||
| scenes = payload.get('storyboard') | ||||||||||||||||||
| if not isinstance(scenes, list): | ||||||||||||||||||
| scenes = [] | ||||||||||||||||||
| scenes_ref = doc_ref.collection(_SCENES_SUBCOLLECTION) | ||||||||||||||||||
| for index, scene in enumerate(scenes): | ||||||||||||||||||
| ops.append(('set', scenes_ref.document(_scene_doc_id(index)), scene)) | ||||||||||||||||||
| if not create: | ||||||||||||||||||
| keep_ids = {_scene_doc_id(i) for i in range(len(scenes))} | ||||||||||||||||||
| for snapshot in scenes_ref.stream(): | ||||||||||||||||||
| if snapshot.id not in keep_ids: | ||||||||||||||||||
| ops.append(('delete', scenes_ref.document(snapshot.id), None)) | ||||||||||||||||||
| _commit_in_batches(ui_db, ops) | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -1236,29 +1207,32 @@ def _write_editor_project_doc( | |||||||||||||||||
| The root update is deliberately field-based: unlike a read-modify-write | ||||||||||||||||||
| replacement, it cannot copy a stale inputConfig snapshot over a concurrent | ||||||||||||||||||
| Setup save. Fields omitted by the editor payload retain the legacy full-save | ||||||||||||||||||
| replacement behavior through DELETE_FIELD updates. | ||||||||||||||||||
| replacement behavior through DELETE_FIELD updates, except storyboard which | ||||||||||||||||||
| leaves the scenes subcollection untouched when omitted. | ||||||||||||||||||
| """ | ||||||||||||||||||
| root_updates = { | ||||||||||||||||||
| field_path.FieldPath(key).to_api_repr(): firestore.DELETE_FIELD | ||||||||||||||||||
| for key in stored | ||||||||||||||||||
| if key not in payload and key != 'inputConfig' | ||||||||||||||||||
| if key not in payload and key not in ('inputConfig', 'storyboard') | ||||||||||||||||||
| } | ||||||||||||||||||
| root_updates.update({ | ||||||||||||||||||
| field_path.FieldPath(key).to_api_repr(): value | ||||||||||||||||||
| for key, value in payload.items() | ||||||||||||||||||
| }) | ||||||||||||||||||
| scenes = payload.get('storyboard') | ||||||||||||||||||
| if not isinstance(scenes, list): | ||||||||||||||||||
| scenes = [] | ||||||||||||||||||
| root_updates[field_path.FieldPath('storyboard').to_api_repr()] = [] | ||||||||||||||||||
| scenes_ref = doc_ref.collection(_SCENES_SUBCOLLECTION) | ||||||||||||||||||
| ops = [('update', doc_ref, root_updates)] | ||||||||||||||||||
| for index, scene in enumerate(scenes): | ||||||||||||||||||
| ops.append(('set', scenes_ref.document(_scene_doc_id(index)), scene)) | ||||||||||||||||||
| keep_ids = {_scene_doc_id(i) for i in range(len(scenes))} | ||||||||||||||||||
| for snapshot in scenes_ref.stream(): | ||||||||||||||||||
| if snapshot.id not in keep_ids: | ||||||||||||||||||
| ops.append(('delete', scenes_ref.document(snapshot.id), None)) | ||||||||||||||||||
| ops = [] | ||||||||||||||||||
| if 'storyboard' in payload: | ||||||||||||||||||
| scenes = payload.get('storyboard') | ||||||||||||||||||
| if not isinstance(scenes, list): | ||||||||||||||||||
| scenes = [] | ||||||||||||||||||
| root_updates[field_path.FieldPath('storyboard').to_api_repr()] = [] | ||||||||||||||||||
|
Contributor
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. Similar to
Suggested change
Collaborator
Author
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. Agreed, and applied as suggested. The editor path now sets the root Mutation check: dropping that unconditional line makes the editor/ |
||||||||||||||||||
| scenes_ref = doc_ref.collection(_SCENES_SUBCOLLECTION) | ||||||||||||||||||
| for index, scene in enumerate(scenes): | ||||||||||||||||||
| ops.append(('set', scenes_ref.document(_scene_doc_id(index)), scene)) | ||||||||||||||||||
| keep_ids = {_scene_doc_id(i) for i in range(len(scenes))} | ||||||||||||||||||
| for snapshot in scenes_ref.stream(): | ||||||||||||||||||
| if snapshot.id not in keep_ids: | ||||||||||||||||||
| ops.append(('delete', scenes_ref.document(snapshot.id), None)) | ||||||||||||||||||
| ops.insert(0, ('update', doc_ref, root_updates)) | ||||||||||||||||||
| _commit_in_batches(ui_db, ops) | ||||||||||||||||||
|
|
||||||||||||||||||
|
|
||||||||||||||||||
|
|
@@ -1425,7 +1399,9 @@ def project_detail_handler( | |||||||||||||||||
|
|
||||||||||||||||||
| The default PATCH is the faithful port of the UI's whole-document autosave: | ||||||||||||||||||
| a full set() with createdBy stripped from the payload (immutable; the stored | ||||||||||||||||||
| owner is preserved) and lastEdited refreshed server-side. PATCH | ||||||||||||||||||
| owner is preserved) and lastEdited refreshed server-side. An omitted | ||||||||||||||||||
| storyboard preserves scenes; other omitted mutable root fields are removed. | ||||||||||||||||||
| PATCH | ||||||||||||||||||
| ?view=editor is the legacy form of the explicit editor exception. The | ||||||||||||||||||
| dedicated PATCH /api/projects/<id>/editor route is the preferred form; both | ||||||||||||||||||
| use field updates so the omitted Setup inputConfig cannot be overwritten by | ||||||||||||||||||
|
|
||||||||||||||||||
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.
If a caller sends
"storyboard": null(or another non-list value) in aPATCHpayload—which is common in clients that serialize unset optional fields asnull—'storyboard' in payloadevaluates toTrue,scenesis coerced to[],keep_idsbecomes empty, and every scene inprojects/<id>/scenesis still deleted. Additionally, whencreate=Trueandstoryboardis omitted orNone,root['storyboard'] = []is already set on line 1182 and the loop body is a no-op.Checking
isinstance(scenes, list)directly ensures thescenessubcollection is only written or pruned when an explicit list ([...]or[]) is provided: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.
Agreed, and applied. I reproduced it on the PR head:
PATCH {"name": "n", "storyboard": null}deleted all 3 scenes on every route._write_project_docnow writes and prunes scenes only whenisinstance(scenes, list)is true. An explicit[]still clears the storyboard. Forcreate=Truewith the storyboard omitted, the result is the same as before (rootstoryboard: [], no scene writes). I also updated the docstring.New test
test_project_patch_non_list_storyboard_leaves_scenes_intactruns 3 routes × 3 values (null, a string, an object). It fails 9/9 against the previous PR head and passes with the fix. Full Python suite is green.