Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
57 changes: 31 additions & 26 deletions orch.py
Original file line number Diff line number Diff line change
Expand Up @@ -1175,22 +1175,23 @@ 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 or non-list storyboard
(e.g. null) leaves the scenes subcollection untouched; only an explicit list
(including []) writes and prunes scenes.
"""
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))
scenes = payload.get('storyboard')
if isinstance(scenes, list):
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 +1206,31 @@ 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: the
root always keeps the [] placeholder, and an omitted or non-list storyboard
(e.g. null) leaves the scenes subcollection untouched.
"""
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 = []
scenes = payload.get('storyboard')
if isinstance(scenes, list):
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 +1397,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
231 changes: 231 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,235 @@ 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

# The GET below rebuilds storyboard from the subcollection, so check the
# stored root placeholder directly.
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']
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('storyboard', [None, 'not-a-list', {'d': '0'}])
@pytest.mark.parametrize(
'path_template',
[
'/api/projects/{id}',
'/api/projects/{id}/editor',
'/api/projects/{id}?view=editor',
],
)
def test_project_patch_non_list_storyboard_leaves_scenes_intact(
monkeypatch, orchestrator_module, path_template, storyboard
):
del orchestrator_module
orch, fake_db, _ = _load_app(monkeypatch)
client = orch.app.test_client()
project_id = f'null-storyboard-{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
)

# Clients that serialize unset optional fields as null must not wipe scenes.
url = path_template.format(id=project_id)
response = client.patch(url, json={'name': 'n', 'storyboard': storyboard})
assert response.status_code == 200

assert fake_db.collection('projects').docs[project_id]['storyboard'] == []
scenes = _scenes_docs(fake_db, project_id)
assert [scenes[f'{i:06d}']['d'] for i in range(3)] == ['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 live only in the subcollection, never inline on the root doc.
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'}
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
Loading