Skip to content
Merged
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
4 changes: 3 additions & 1 deletion DEVELOPING.md
Original file line number Diff line number Diff line change
Expand Up @@ -251,7 +251,9 @@ Editor saves without inputs use `PATCH /api/projects/:id/editor`. Its root
field updates leave stored `inputConfig` untouched, including a concurrent
Setup update. Do not copy an earlier Setup snapshot into a replacement write.
Full GET/PATCH remains the Setup and legacy detail contract; full PATCH keeps
its replacement semantics. All editor candidates remain available for counts,
its replacement semantics, except that an omitted `storyboard` leaves stored
scenes unchanged. Other omitted root fields, including `inputConfig`, are removed.
All editor candidates remain available for counts,
selection, generation and composition.

The dedicated editor PATCH path is also a rollback safety boundary: a
Expand Down
61 changes: 34 additions & 27 deletions orch.py
Original file line number Diff line number Diff line change
Expand Up @@ -1175,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 = []

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.

If a caller sends "storyboard": null (or another non-list value) in a PATCH payload—which is common in clients that serialize unset optional fields as null—'storyboard' in payload evaluates to True, scenes is coerced to [], keep_ids becomes empty, and every scene in projects/<id>/scenes is still deleted. Additionally, when create=True and storyboard is omitted or None, root['storyboard'] = [] is already set on line 1182 and the loop body is a no-op.

Checking isinstance(scenes, list) directly ensures the scenes subcollection is only written or pruned when an explicit list ([...] or []) is provided:

Suggested change
if create or 'storyboard' in payload:
scenes = payload.get('storyboard')
if not isinstance(scenes, list):
scenes = []
scenes = payload.get('storyboard')
if isinstance(scenes, list):

Copy link
Copy Markdown
Collaborator Author

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_doc now writes and prunes scenes only when isinstance(scenes, list) is true. An explicit [] still clears the storyboard. For create=True with the storyboard omitted, the result is the same as before (root storyboard: [], no scene writes). I also updated the docstring.

New test test_project_patch_non_list_storyboard_leaves_scenes_intact runs 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.

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)


Expand All @@ -1205,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()] = []

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.

Similar to _write_project_doc, if a PATCH payload to the editor endpoint includes "storyboard": null (or a non-list value), 'storyboard' in payload is True and scenes = [] deletes every scene in the subcollection. Furthermore, because lines 1218–1221 copy all payload keys into root_updates, setting root_updates['storyboard'] = [] unconditionally before checking isinstance(scenes, list) ensures a non-list storyboard value in payload can never overwrite the root document's storyboard: [] placeholder:

Suggested change
if 'storyboard' in payload:
scenes = payload.get('storyboard')
if not isinstance(scenes, list):
scenes = []
root_updates[field_path.FieldPath('storyboard').to_api_repr()] = []
root_updates[field_path.FieldPath('storyboard').to_api_repr()] = []
scenes = payload.get('storyboard')
if isinstance(scenes, list):

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Agreed, and applied as suggested. The editor path now sets the root storyboard placeholder to [] unconditionally, before the isinstance(scenes, list) check. As a result, a non-list value copied from payload can never land on the root document, and scenes are written or pruned only for an explicit list.

Mutation check: dropping that unconditional line makes the editor/?view=editor cases of both the new non-list test and the shrink test fail (8 failures).

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)


Expand Down Expand Up @@ -1394,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
Expand Down
187 changes: 187 additions & 0 deletions test/test_frontdoor_data.py
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@
import threading
import uuid

import pytest

from google.api_core import exceptions as google_exceptions
from google.cloud import firestore

Expand Down Expand Up @@ -1662,6 +1664,191 @@ def test_project_patch_prunes_removed_scenes(monkeypatch, orchestrator_module):
body = client.get('/api/projects/shrink').get_json()
assert [s['d'] for s in body['storyboard']] == ['0']

@pytest.mark.parametrize(
'path_template',
[
'/api/projects/{id}',
'/api/projects/{id}/editor',
'/api/projects/{id}?view=editor',
],
)
def test_project_patch_omitting_storyboard_leaves_scenes_intact(
monkeypatch, orchestrator_module, path_template
):
del orchestrator_module
orch, fake_db, _ = _load_app(monkeypatch)
client = orch.app.test_client()
project_id = f'keep-scenes-{uuid.uuid4().hex[:8]}'

assert (
client.post(
'/api/projects',
json={
'id': project_id,
'name': 'Original',
'inputConfig': {'productDescription': 'Keep for editor saves'},
'storyboard': [{'d': '0'}, {'d': '1'}, {'d': '2'}],
},
).status_code
== 200
)
assert len(_scenes_docs(fake_db, project_id)) == 3

# PATCH {"name": "Renamed"} must leave all 3 scenes intact (SM-6 fix).
url = path_template.format(id=project_id)
response = client.patch(url, json={'name': 'Renamed'})
assert response.status_code == 200

scenes = _scenes_docs(fake_db, project_id)
assert len(scenes) == 3
assert [scenes[f'{i:06d}']['d'] for i in range(3)] == ['0', '1', '2']
Comment on lines +1705 to +1707

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.

Because _read_project_doc (GET /api/projects/<id>) unconditionally overwrites data['storyboard'] from scenes_ref.stream(), the GET assertion on line 1708 would still pass even if _write_editor_project_doc marked the root document's storyboard field with DELETE_FIELD. Asserting on the stored root document in fake_db directly verifies the root storyboard == [] preservation invariant across all three routes:

Suggested change
scenes = _scenes_docs(fake_db, project_id)
assert len(scenes) == 3
assert [scenes[f'{i:06d}']['d'] for i in range(3)] == ['0', '1', '2']
assert fake_db.collection('projects').docs[project_id]['storyboard'] == []
scenes = _scenes_docs(fake_db, project_id)
assert len(scenes) == 3
assert [scenes[f'{i:06d}']['d'] for i in range(3)] == ['0', '1', '2']

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Good point: GET rebuilds storyboard from the subcollection, so it would hide this regression. I added the direct fake_db root assertion here (with a short comment explaining why). The new non-list test uses the same check.


body = client.get(f'/api/projects/{project_id}').get_json()
assert body['name'] == 'Renamed'
assert [s['d'] for s in body['storyboard']] == ['0', '1', '2']

if path_template == '/api/projects/{id}':
# Full PATCH replaces root fields; only an omitted storyboard is preserved.
assert 'inputConfig' not in body
else:
assert body['inputConfig'] == {
'productDescription': 'Keep for editor saves'
}


@pytest.mark.parametrize(
'path_template',
[
'/api/projects/{id}',
'/api/projects/{id}/editor',
'/api/projects/{id}?view=editor',
],
)
def test_project_patch_empty_payload_leaves_scenes_intact(
monkeypatch, orchestrator_module, path_template
):
del orchestrator_module
orch, fake_db, _ = _load_app(monkeypatch)
client = orch.app.test_client()
project_id = f'empty-patch-{uuid.uuid4().hex[:8]}'

assert (
client.post(
'/api/projects',
json={
'id': project_id,
'name': 'Original',
'storyboard': [{'d': '0'}, {'d': '1'}, {'d': '2'}],
},
).status_code
== 200
)
assert len(_scenes_docs(fake_db, project_id)) == 3

url = path_template.format(id=project_id)
response = client.patch(url, json={})
assert response.status_code == 200

scenes = _scenes_docs(fake_db, project_id)
assert len(scenes) == 3
body = client.get(f'/api/projects/{project_id}').get_json()
assert [s['d'] for s in body['storyboard']] == ['0', '1', '2']


@pytest.mark.parametrize(
'path_template',
[
'/api/projects/{id}',
'/api/projects/{id}/editor',
'/api/projects/{id}?view=editor',
],
)
def test_project_patch_explicit_empty_storyboard_deletes_all_scenes(
monkeypatch, orchestrator_module, path_template
):
del orchestrator_module
orch, fake_db, _ = _load_app(monkeypatch)
client = orch.app.test_client()
project_id = f'clear-scenes-{uuid.uuid4().hex[:8]}'

assert (
client.post(
'/api/projects',
json={
'id': project_id,
'name': 'Original',
'storyboard': [{'d': '0'}, {'d': '1'}, {'d': '2'}],
},
).status_code
== 200
)
assert len(_scenes_docs(fake_db, project_id)) == 3

url = path_template.format(id=project_id)
response = client.patch(url, json={'storyboard': []})
assert response.status_code == 200

assert len(_scenes_docs(fake_db, project_id)) == 0
body = client.get(f'/api/projects/{project_id}').get_json()
assert body['storyboard'] == []


@pytest.mark.parametrize(
'path_template',
[
'/api/projects/{id}',
'/api/projects/{id}/editor',
'/api/projects/{id}?view=editor',
],
)
def test_project_patch_shrinking_storyboard_deletes_trailing_scenes(
monkeypatch, orchestrator_module, path_template
):
del orchestrator_module
orch, fake_db, _ = _load_app(monkeypatch)
client = orch.app.test_client()
project_id = f'shrink-5-to-3-{uuid.uuid4().hex[:8]}'

assert (
client.post(
'/api/projects',
json={
'id': project_id,
'name': 'Original',
'storyboard': [{'d': str(i)} for i in range(5)],
},
).status_code
== 200
)
assert len(_scenes_docs(fake_db, project_id)) == 5

url = path_template.format(id=project_id)
response = client.patch(
url,
json={
'storyboard': [{'d': f'{i}-updated'} for i in range(3)],
},
)
assert response.status_code == 200

scenes = _scenes_docs(fake_db, project_id)
assert len(scenes) == 3
assert set(scenes.keys()) == {'000000', '000001', '000002'}
Comment on lines +1878 to +1880

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.

In _write_editor_project_doc, root_updates.update(...) initially copies the full payload['storyboard'] list into root_updates before root_updates['storyboard'] = [] resets it to an empty list. Asserting that the stored root document in fake_db still has storyboard == [] after a PATCH that supplies scenes verifies that scenes are never persisted inline on the root document:

Suggested change
scenes = _scenes_docs(fake_db, project_id)
assert len(scenes) == 3
assert set(scenes.keys()) == {'000000', '000001', '000002'}
assert fake_db.collection('projects').docs[project_id]['storyboard'] == []
scenes = _scenes_docs(fake_db, project_id)
assert len(scenes) == 3
assert set(scenes.keys()) == {'000000', '000001', '000002'}

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Applied. The shrink test now asserts the stored root storyboard == [] after a PATCH that supplies scenes. It catches the regression: removing the editor path's root_updates['storyboard'] = [] makes this test fail for /editor and ?view=editor, because the scenes are then stored inline on the root.

assert [scenes[f'{i:06d}']['d'] for i in range(3)] == [
'0-updated',
'1-updated',
'2-updated',
]
assert '000003' not in scenes
assert '000004' not in scenes

body = client.get(f'/api/projects/{project_id}').get_json()
assert [s['d'] for s in body['storyboard']] == [
'0-updated',
'1-updated',
'2-updated',
]


def test_project_delete_clears_scenes_subcollection(
monkeypatch, orchestrator_module
Expand Down