From d9f168c317f107c45d090e6c436d6fe759bdb0fe Mon Sep 17 00:00:00 2001 From: Vito Date: Wed, 28 Oct 2026 18:09:16 +0000 Subject: [PATCH 1/3] feat(nodes): per-node Save to library switch (#90) (#427) * feat(nodes): per-node Save to library switch (#90) Every PromptManager node (Classic, Text, Focused, Split, Pair) gets an optional save_to_library BOOLEAN, default on and declared last. Off, the node still returns its normal outputs but PromptManager ignores the run: - the node saves, counts, tracks and records nothing; - the queue hook reads the switch from the queued graph (literal, or a linked PrimitiveBoolean), so a cached node is not counted either; a switch wired from an unreadable node is left to the node itself; - an opted-out node is no run prompt, so no tracker entry, run key, embedded-metadata match or history attribution links its images, and a run with any opted-out positive node links none of its files; - the scan reports such files as not_saved (never stored or queued for Needs review), and a file whose only opted-out node is its negative is stored positive-only, without the pair; - execution history records no prompt text for an opted-out role. PromptSearchList only reads the library and gets no switch. The switch takes the widget index older workflows used for the editor's own DOM slot (''), so a configure step turns any non-boolean value on: workflows saved before the switch load with it on and no earlier widget value moves. The Focused, Split and Pair editors draw it as a "Save to library" checkbox in their toolbar; Classic and Text show the native toggle. * fix(nodes): Save to library review fixes (#90) - A switch wired from a node the graph cannot read and off at run time marks the node's run key, so an earlier run's tracker entry never links this run's images. - The scan reports such files as not_saved_wired ("only the run itself can link it") instead of not_saved. - Workflow-only metadata ignores muted and bypassed PromptManager nodes and reads nodes inside subgraph definitions. - The Focused, Split and Pair footer says when the switch comes from a link. * fix(usage): count cached re-runs of runtime-linked prompt text (#347) A PromptManager-family node whose text comes from a runtime link (PreviewAny, an LLM, a list node) counted only the job that executed it; later jobs served from ComfyUI's output cache counted nothing. Each execution now records a receipt (digests of its STRING outputs and hashes of the text it stores) per run key and queue item. On execution_cached the receipt whose digests equal the cache entry's text is replayed once for the job, with the node's role and what the queue hook already saw recomputed from the job's own graph. The executing node and the replay count through one shared own_counts. Unmatched cache entries count nothing, as before; a node that executes after its replay in the same job skips its own count. No schema change, no backfill. * fix(usage): never replay a node twice in one job; dedupe receipts once complete (#347) A repeated execution_cached id or message for the same job now counts the node once. Receipts replace equal older ones only when their job finishes (or the next job starts), so a list still recording [A] of [A, B] no longer drops an older [A] receipt. Node-level tests check the receipt each execution records. * test(coverage): baseline for the Save to library module (#90) * test(usage): disabled node then cached jobs counts 0 (#90, #347) A PromptManager node with Save to library off records no cached-run receipt and is no run prompt in the replayed job's graph, so ComfyUI serving it from its cache on later jobs counts nothing. The prompt is saved first by an enabled run, so a replayed count would show. Covers PromptManager, Focused, Split, Text and the Pair. * fix(nodes): hide the editor's native widgets through one helper (#90) The Save to library switch copied the text fallbacks' zero-height computeSize arrow, and neither copy was ever called, so focused-node.js function coverage fell to 87.5 (floor 90). One hideNativeWidget helper now hides both, and the spec checks that the hidden switch and a text fallback take no node height. --- README.md | 6 + prompt_manager.py | 19 +- prompt_manager_base.py | 40 +- prompt_manager_pair.py | 27 +- prompt_manager_text.py | 19 +- py/api/admin.py | 31 ++ tests/browser/classic-node.spec.mjs | 10 +- tests/browser/focused-node.spec.mjs | 30 +- tests/browser/pair-node.spec.mjs | 14 +- tests/comfyui/execution-text-link.spec.mjs | 48 +- tests/comfyui/text-link.spec.mjs | 11 +- tests/js/save-to-library.test.js | 68 +++ tests/test_cached_runs.py | 46 +- tests/test_save_to_library.py | 508 +++++++++++++++++++ tests/vendor/comfyui-config.test.mjs | 7 +- tools/vendor/frontend-coverage-baseline.json | 6 + utils/execution_context_capture.py | 3 + utils/execution_context_graph.py | 10 + utils/image_monitor.py | 39 +- utils/prompt_graph.py | 202 +++++++- utils/usage_tracking.py | 14 +- web/comfy/prompt_manager.js | 3 + web/js/focused-node.js | 35 +- web/js/prompt-editor.css | 2 + web/js/save-to-library.js | 47 ++ 25 files changed, 1187 insertions(+), 58 deletions(-) create mode 100644 tests/js/save-to-library.test.js create mode 100644 tests/test_save_to_library.py create mode 100644 web/js/save-to-library.js diff --git a/README.md b/README.md index 6142e6fc..946967d1 100644 --- a/README.md +++ b/README.md @@ -312,6 +312,12 @@ Both nodes support the same metadata fields: Both nodes will automatically save prompts to the database and link any generated images to the prompt. +- **Save to library** (`save_to_library`, on by default; "Save to library" in the Focused, Split and Pair editors): + turn it off while iterating and PromptManager ignores that node's runs. The prompt is not saved, the run is not + counted and its images are not linked (a scan skips them too), while the node's output is unchanged. Workflows + saved before this switch existed load with it on. If any PromptManager node feeding a sampler's positive input + has it off, none of that run's images are linked, because the image cannot show which sampler produced it. + ### 🖼️ Using the Output-Wide Gallery The comprehensive gallery system provides complete access to all your ComfyUI output: diff --git a/prompt_manager.py b/prompt_manager.py index 2dcf0031..b7bcdc1a 100644 --- a/prompt_manager.py +++ b/prompt_manager.py @@ -26,13 +26,13 @@ except ImportError: InputTypeDict = dict try: - from .prompt_manager_base import PromptManagerBase + from .prompt_manager_base import SAVE_TO_LIBRARY_INPUT, PromptManagerBase except ImportError: import os import sys sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) - from prompt_manager_base import PromptManagerBase + from prompt_manager_base import SAVE_TO_LIBRARY_INPUT, PromptManagerBase class PromptManager(PromptManagerBase, ComfyNodeABC): @@ -115,6 +115,7 @@ class PromptManager(PromptManagerBase, ComfyNodeABC): ) }, ), + "save_to_library": SAVE_TO_LIBRARY_INPUT, }, "hidden": { # The queued graph and this node's id: used to tell the positive @@ -152,6 +153,7 @@ class PromptManager(PromptManagerBase, ComfyNodeABC): append_text: str = "", prompt: Optional[Dict[str, Any]] = None, unique_id: Optional[str] = None, + save_to_library: bool = True, ) -> Tuple[Any]: """ Encode the text prompt and save it to the database. @@ -164,6 +166,7 @@ class PromptManager(PromptManagerBase, ComfyNodeABC): search_text: Text to search for in past prompts prepend_text: Text to prepend to the main prompt append_text: Text to append to the main prompt + save_to_library: Off: nothing is saved, counted or tracked (#90) Returns: Tuple containing the conditioning for the diffusion model and the final @@ -205,7 +208,9 @@ class PromptManager(PromptManagerBase, ComfyNodeABC): # Save prompt to database and set execution context for gallery tracking prompt_id = None extended_tags = [] - if storage_text and storage_text.strip(): + if not save_to_library: + self._skip_not_saved(prompt, unique_id) + elif storage_text and storage_text.strip(): self.logger.debug(f"Processing prompt text: {storage_text[:100]}...") # Add prepend/append info to tags if they exist @@ -249,6 +254,11 @@ class PromptManager(PromptManagerBase, ComfyNodeABC): tokens = clip.tokenize(encoding_text) conditioning = clip.encode_from_tokens_scheduled(tokens) + self.logger.info(f"CLIP encoding completed, text: {repr(encoding_text)[:80]}") + result = (conditioning, encoding_text) + if not save_to_library: + return result # #90: no metadata registration, no history text + # Register with ComfyUI integration for standard metadata compatibility node_id = f"promptmanager_{int(time.time() * 1000)}" self.comfyui_integration.register_prompt( @@ -262,9 +272,6 @@ class PromptManager(PromptManagerBase, ComfyNodeABC): "append_text": append_text.strip() if append_text else None, }, ) - - self.logger.info(f"CLIP encoding completed, text: {repr(encoding_text)[:80]}") - result = (conditioning, encoding_text) record_runtime_context( type(self).__name__, text, prepend_text, append_text, encoding_text ) diff --git a/prompt_manager_base.py b/prompt_manager_base.py index 62977b1f..921e1e2a 100644 --- a/prompt_manager_base.py +++ b/prompt_manager_base.py @@ -23,7 +23,7 @@ try: from .utils.image_monitor import get_image_monitor from .utils.prompt_tracker import get_prompt_tracker from .utils.hashing import generate_prompt_hash - from .utils.prompt_graph import resolve_text, run_prompt_nodes + from .utils.prompt_graph import resolve_text, run_prompt_nodes, save_setting from .utils.run_keys import executing_run_id, prompt_node_key from .utils.usage_tracking import PENDING_FIRST_USE from .utils.cached_runs import ( @@ -42,7 +42,7 @@ except ImportError: from utils.image_monitor import get_image_monitor from utils.prompt_tracker import get_prompt_tracker from utils.hashing import generate_prompt_hash - from utils.prompt_graph import resolve_text, run_prompt_nodes + from utils.prompt_graph import resolve_text, run_prompt_nodes, save_setting from utils.run_keys import executing_run_id, prompt_node_key from utils.usage_tracking import PENDING_FIRST_USE from utils.cached_runs import ( @@ -54,6 +54,22 @@ except ImportError: ) +# The optional input every PromptManager node declares last (#90): declared +# last so no widget value of a workflow saved before it shifts. +SAVE_TO_LIBRARY_INPUT = ( + "BOOLEAN", + { + "default": True, + "label_on": "save", + "label_off": "skip", + "tooltip": ( + "Save this node's prompt to the library, count its runs and link its " + "images. Off: PromptManager ignores the run; the output is unchanged." + ), + }, +) + + class PromptManagerBase: """Mixin providing shared prompt management logic for ComfyUI nodes. @@ -156,7 +172,9 @@ class PromptManagerBase: resolved = resolve_text(prompt_graph, node_id) return { "is_run_prompt": node_id in run_prompt_nodes(prompt_graph), + # The hook skips a switch it cannot read (#90): the node counts "seen_at_queue": resolved is not None + and save_setting(prompt_graph, node_id) is True and generate_prompt_hash(resolved) == generate_prompt_hash(text), "known": True, } @@ -245,6 +263,24 @@ class PromptManagerBase: "run_id": executing_run_id(), } + def _skip_not_saved(self, prompt_graph: Any = None, unique_id: Any = None) -> None: + """``save_to_library`` is off: nothing is saved, counted or tracked (#90). + + A literal off makes the node no run prompt (``utils.prompt_graph``), so + no image looks up its run key. A switch wired from a node the graph + cannot read keeps the node a run prompt: its run key is marked like an + empty editor's, so an earlier run's entry of that key never links this + run's images. + """ + message = ( + "Save to library is off for this node: its prompt is not saved, the " + "run is not counted and its images are not linked" + ) + if isinstance(prompt_graph, dict) and unique_id is not None: + self._mark_untracked(prompt_graph, unique_id, message) + else: + self.logger.info(message) + def _skip_empty_prompt(self, prompt_graph: Any, unique_id: Any) -> None: """The ``text`` editor is empty: save and track nothing (#342). diff --git a/prompt_manager_pair.py b/prompt_manager_pair.py index df62d633..d406e23e 100644 --- a/prompt_manager_pair.py +++ b/prompt_manager_pair.py @@ -22,7 +22,11 @@ try: ) from .utils.hashing import generate_prompt_hash from .utils.pair_execution import record_pair_execution - from .utils.prompt_graph import intrinsic_pair_nodes, run_prompt_components + from .utils.prompt_graph import ( + intrinsic_pair_nodes, + run_prompt_components, + save_setting, + ) from .utils.prompt_pair_resolver import resolve_text_input from .utils.usage_tracking import ( PENDING_FIRST_USE, @@ -41,7 +45,11 @@ except ImportError: ) from utils.hashing import generate_prompt_hash from utils.pair_execution import record_pair_execution - from utils.prompt_graph import intrinsic_pair_nodes, run_prompt_components + from utils.prompt_graph import ( + intrinsic_pair_nodes, + run_prompt_components, + save_setting, + ) from utils.prompt_pair_resolver import resolve_text_input from utils.usage_tracking import ( PENDING_FIRST_USE, @@ -90,8 +98,13 @@ class PromptManagerPair(PromptManagerFocused): prompt: Optional[Dict[str, Any]] = None, unique_id: Optional[str] = None, extra_pnginfo: Optional[Dict[str, Any]] = None, + save_to_library: bool = True, ) -> Tuple[Any, Any, str, str]: - """Encode both sides once; database and tracking failures never block CLIP.""" + """Encode both sides once; database and tracking failures never block CLIP. + + With ``save_to_library`` off nothing is saved, counted, tracked or + recorded (#90); the four outputs are the same. + """ if clip is None: raise RuntimeError("ERROR: clip input is invalid: None") if not isinstance(text, str) or not isinstance(negative_text, str): @@ -109,6 +122,10 @@ class PromptManagerPair(PromptManagerFocused): negative_conditioning = clip.encode_from_tokens_scheduled( clip.tokenize(negative_text) ) + result = positive_conditioning, negative_conditioning, positive, negative_text + if not save_to_library: + self._skip_not_saved(prompt, unique_id) + return result record_pair_execution( extra_pnginfo, unique_id, @@ -169,7 +186,6 @@ class PromptManagerPair(PromptManagerFocused): ) except Exception as error: self.logger.warning(f"Could not register paired metadata: {error}") - result = positive_conditioning, negative_conditioning, positive, negative_text record_runtime_context( type(self).__name__, text, @@ -219,6 +235,9 @@ class PromptManagerPair(PromptManagerFocused): resolved_negative = ( resolve_text_input(prompt, node_id, "negative_text") if known else None ) + if known and save_setting(prompt, node_id) is not True: + # The hook skips a switch it cannot read (#90): the node counts + resolved_positive = resolved_negative = None positive_seen = resolved_positive is not None and generate_prompt_hash( resolved_positive ) == generate_prompt_hash(text) diff --git a/prompt_manager_text.py b/prompt_manager_text.py index 6e316f98..c6e3f070 100644 --- a/prompt_manager_text.py +++ b/prompt_manager_text.py @@ -26,13 +26,13 @@ except ImportError: InputTypeDict = dict try: - from .prompt_manager_base import PromptManagerBase + from .prompt_manager_base import SAVE_TO_LIBRARY_INPUT, PromptManagerBase except ImportError: import os import sys sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) - from prompt_manager_base import PromptManagerBase + from prompt_manager_base import SAVE_TO_LIBRARY_INPUT, PromptManagerBase class PromptManagerText(PromptManagerBase, ComfyNodeABC): @@ -115,6 +115,7 @@ class PromptManagerText(PromptManagerBase, ComfyNodeABC): ), }, ), + "save_to_library": SAVE_TO_LIBRARY_INPUT, }, "hidden": { # The queued graph and this node's id: used to tell the positive @@ -149,6 +150,7 @@ class PromptManagerText(PromptManagerBase, ComfyNodeABC): append_text: str = "", prompt: Optional[Dict[str, Any]] = None, unique_id: Optional[str] = None, + save_to_library: bool = True, ) -> Tuple[str]: """ Process the text prompt and save it to the database. @@ -160,6 +162,7 @@ class PromptManagerText(PromptManagerBase, ComfyNodeABC): search_text: Text to search for in past prompts prepend_text: Text to prepend to the main prompt append_text: Text to append to the main prompt + save_to_library: Off: nothing is saved, counted or tracked (#90) Returns: Tuple containing the final processed text string @@ -184,7 +187,9 @@ class PromptManagerText(PromptManagerBase, ComfyNodeABC): # Save prompt to database and set execution context for gallery tracking prompt_id = None extended_tags = [] - if storage_text and storage_text.strip(): + if not save_to_library: + self._skip_not_saved(prompt, unique_id) + elif storage_text and storage_text.strip(): self.logger.debug(f"Processing prompt text: {storage_text[:100]}...") # Add prepend/append info to tags if they exist @@ -220,6 +225,11 @@ class PromptManagerText(PromptManagerBase, ComfyNodeABC): else: self._skip_empty_prompt(prompt, unique_id) + self.logger.debug(f"Text processing completed: {final_text[:100]}...") + result = (final_text,) + if not save_to_library: + return result # #90: no metadata registration, no history text + # Register with ComfyUI integration for standard metadata compatibility node_id = f"promptmanagertext_{int(time.time() * 1000)}" self.comfyui_integration.register_prompt( @@ -233,9 +243,6 @@ class PromptManagerText(PromptManagerBase, ComfyNodeABC): "append_text": append_text.strip() if append_text else None, }, ) - - self.logger.debug(f"Text processing completed: {final_text[:100]}...") - result = (final_text,) record_runtime_context( type(self).__name__, text, prepend_text, append_text, final_text ) diff --git a/py/api/admin.py b/py/api/admin.py index a96d07d2..e010e3fb 100644 --- a/py/api/admin.py +++ b/py/api/admin.py @@ -52,6 +52,7 @@ try: extract_generation_params, parse_page_json, ) + from ...utils.prompt_graph import metadata_opted_out, side_opted_out from ...utils.prompt_pair_resolver import resolve_prompt_pair, sampler_less_prompt from ...utils.review_capture import reviewable except ImportError: @@ -67,6 +68,7 @@ except ImportError: extract_generation_params, parse_page_json, ) + from utils.prompt_graph import metadata_opted_out, side_opted_out from utils.prompt_pair_resolver import resolve_prompt_pair, sampler_less_prompt from utils.review_capture import reviewable @@ -260,6 +262,11 @@ SCAN_DETAILS = { "failed": "Error: the prompt could not be stored", "error": "Error: the file could not be processed (see the server log)", "found": "Metadata found, but no prompt text to store", + "not_saved": "Skipped · its PromptManager node does not save to the library", + "not_saved_wired": ( + "Skipped · its Save to library switch is wired from another node, so only " + "the run itself can link it" + ), None: "No prompt metadata in the file", } @@ -1720,10 +1727,34 @@ class AdminRoutesMixin: except (ValueError, TypeError): # Preserve malformed metadata for conservative resolver rejection. pass + if metadata_opted_out(canonical): + # Save to library was off for the run's prompt: no prompt, link, + # recorded-run fallback or Needs review item (#90) + return "not_saved" + if metadata_opted_out(canonical, unreadable=True): + # The switch is wired from a node the file cannot show: only the + # live run (the node saw the value) can link it, never a scan + return "not_saved_wired" if not canonical.get("prompt") and not canonical.get("parameters"): return self._link_recorded_run(media_file, canonical, detailed, history) _with_file_size(canonical, media_file) resolution = resolve_prompt_pair(canonical) + if resolution["state"] == "resolved" and side_opted_out( + canonical.get("prompt"), resolution["negative"] + ): + # Only the negative's node is off: store the positive alone (#90) + prompt_text = ( + resolution["positive"] + .get("storage_text", resolution["positive"]["text"]) + .strip() + ) + status = self._store_scanned_prompt(media_file, canonical, prompt_text) + if not detailed or status not in SCAN_TITLED_DETAILS: + return status + return { + "status": status, + "title": self._scan_title(media_file, prompt_text), + } if blank_positive(resolution): # Only prepend/append text: no stored prompt can own it (#342) resolution = {**resolution, "state": "unresolved"} diff --git a/tests/browser/classic-node.spec.mjs b/tests/browser/classic-node.spec.mjs index bd627e22..0f6cdead 100644 --- a/tests/browser/classic-node.spec.mjs +++ b/tests/browser/classic-node.spec.mjs @@ -19,7 +19,7 @@ test.beforeEach(async ({page, browserName}) => { await import('/extensions/promptmanager/prompt_manager.js'); class Node { constructor(){ - this.widgets=['text','category','tags','search_text'].map(name=>({name,value:'',inputEl:document.createElement('textarea')})); + this.widgets=[...['text','category','tags','search_text'].map(name=>({name,value:'',inputEl:document.createElement('textarea')})),{name:'save_to_library',type:'toggle',value:true}]; this.widgets_values=[]; this.inputs=[]; } addDOMWidget(name,type,element){document.body.append(element);return {element};} @@ -36,6 +36,14 @@ test.afterEach(async ({page,browserName}) => { expect(page.classicErrors).toEqual([]); }); +test('Classic loads an older workflow with Save to library on and keeps a saved off (#90)',async({page})=>{ + // A workflow saved before the switch held the search UI's slot ('') at the switch's index + const loaded=await page.evaluate(()=>{const widget=node.widgets.find(w=>w.name==='save_to_library');widget.value='';node.configure({});return widget.value;}); + expect(loaded).toBe(true); + const off=await page.evaluate(()=>{const widget=node.widgets.find(w=>w.name==='save_to_library');widget.value=false;node.configure({});return [widget.value,node.serialize().widgets_values[4]];}); + expect(off).toEqual([false,false]); +}); + test('Classic keeps immediate selection, search query and metadata serialization',async({page})=>{ await page.route('**/prompt_manager/search?**', route=>route.fulfill({json:{success:true,results:[{text:'A new portrait',category:'Portrait',tags:['film grain'],rating:5}]}})); await page.evaluate(()=>{node.widgets.find(w=>w.name==='search_text').value='tag:"film grain" portrait';}); diff --git a/tests/browser/focused-node.spec.mjs b/tests/browser/focused-node.spec.mjs index 4f2026bb..6ab2818c 100644 --- a/tests/browser/focused-node.spec.mjs +++ b/tests/browser/focused-node.spec.mjs @@ -26,7 +26,7 @@ test.beforeEach(async({page,browserName})=>{ window.fixtureApp={registerExtension(extension){this.extension=extension;},ui:{settings:new EventTarget()},graph:{setDirtyCanvas(){}},canvas:{}}; await import('/extensions/promptmanager/prompt_manager.js'); class Node { - constructor(){this.widgets=['text','category','tags','search_text'].map(name=>({name,type:'text',value:'',options:{},inputEl:document.createElement('textarea')}));this.inputs=[...['clip','prepend_text','append_text'].map(name=>({name,type:name==='clip'?'CLIP':'STRING',link:null})),...['text','category','tags','search_text'].map(name=>({name,type:'STRING',link:null,widget:{name}}))];this.properties={};this.size=[550,700];} + constructor(){this.widgets=[...['text','category','tags','search_text'].map(name=>({name,type:'text',value:'',options:{},inputEl:document.createElement('textarea')})),{name:'save_to_library',type:'toggle',value:true,options:{}}];this.inputs=[...['clip','prepend_text','append_text'].map(name=>({name,type:name==='clip'?'CLIP':'STRING',link:null})),...['text','category','tags','search_text'].map(name=>({name,type:'STRING',link:null,widget:{name}})),{name:'save_to_library',type:'BOOLEAN',link:null,widget:{name:'save_to_library'}}];this.properties={};this.size=[550,700];} findInputByType(type){const index=this.inputs.findIndex(input=>input.type===type&&input.link==null);return index<0?undefined:{index,slot:this.inputs[index]};} addWidget(type,name,value,callback,options){const widget={type,name,value,callback,options};this.widgets.push(widget);return widget;} addDOMWidget(name,type,element){const host=document.createElement('div');host.style.height='650px';host.className='fixture-dom-widget';host.append(element);document.body.append(host);return {element};} @@ -41,6 +41,32 @@ test.beforeEach(async({page,browserName})=>{ }); test.afterEach(async({page,browserName})=>{await saveCoverage(page,browserName);expect(page.focusedErrors).toEqual([]);}); +test(`${variant} Save to library switch saves its value and loads older workflows on (#90)`, async ({page}) => { + const toggle=page.getByRole('checkbox',{name:'Save to library',exact:true}); + await expect(toggle).toBeChecked(); + expect(await page.evaluate(()=>node.widgets.map(widget=>widget.name))).toEqual(['text','category','tags','search_text','prepend_text','append_text','save_to_library']); + // The native switch is drawn in the toolbar only: the host keeps it hidden and zero-height, like the text fallbacks + expect(await page.evaluate(()=>{const widget=node.widgets.find(item=>item.name==='save_to_library');return {type:widget.type,hidden:widget.hidden,size:widget.computeSize()};})).toEqual({type:'hidden',hidden:true,size:[0,-4]}); + expect(await page.evaluate(()=>node.widgets.find(item=>item.name==='text').computeSize())).toEqual([0,-4]); + await toggle.uncheck(); + await expect(page.locator('.pm-focused > .pm-editor-status[role=status]')).toHaveText('Save to library is off: runs of this node are not added to the library.'); + const saved=await page.evaluate(()=>node.serialize()); + expect(saved.widgets_values[6]).toBe(false); + await page.evaluate(saved=>node.configure(saved),saved); + await expect(toggle).not.toBeChecked(); + await page.getByLabel('Main prompt',{exact:true}).fill('Typed while off'); + await expect(page.locator('.pm-focused > .pm-editor-status[role=status]')).toHaveText('Save to library is off: runs of this node are not added to the library.'); + // A workflow saved before the switch held the editor's own slot ('') at its index + await page.evaluate(()=>node.configure({widgets_values:['Old text','','','','','',''],properties:{}})); + await expect(toggle).toBeChecked(); + expect(await page.evaluate(()=>node.widgets.find(widget=>widget.name==='save_to_library').value)).toBe(true); + await toggle.check(); + await page.getByLabel('Main prompt',{exact:true}).fill('Typed while on'); + await expect(page.locator('.pm-focused > .pm-editor-status[role=status]')).toHaveText('Main prompt is saved when this node runs.'); + await page.evaluate(()=>{node.inputs.find(input=>input.name==='save_to_library').link=9;node.onConnectionsChange?.();}); + await expect(toggle).toBeDisabled(); +}); + test(`${variant} library autocompletes filters and searches without changing editor metadata`, async ({page}) => { const requests=[]; await page.route('**/prompt_manager/categories',route=>route.fulfill({json:{success:true,categories:['Portrait']}})); @@ -257,7 +283,7 @@ test(`${variant} restores workflow values and disclosures and protects connected test(`${variant} offers text as a link socket, keeps typed text while linked and through a reload (#343)`,async({page})=>{ const sockets=()=>page.evaluate(()=>Object.fromEntries(node.inputs.map(input=>[input.name,input.widget?.name??null]))); // The hidden text widget no longer owns the input, so the host draws it as a socket; the other widget inputs are unchanged. - expect(await sockets()).toEqual({clip:null,prepend_text:null,append_text:null,text:null,category:'category',tags:'tags',search_text:'search_text'}); + expect(await sockets()).toEqual({clip:null,prepend_text:null,append_text:null,text:null,category:'category',tags:'tags',search_text:'search_text',save_to_library:'save_to_library'}); expect(await page.evaluate(()=>node.findInputByType('STRING').slot.name)).toBe('text'); expect(await page.evaluate(()=>node.findInputByType('CLIP').slot.name)).toBe('clip'); const main=page.getByRole('textbox',{name:'Main prompt',exact:true}); diff --git a/tests/browser/pair-node.spec.mjs b/tests/browser/pair-node.spec.mjs index 2d554a84..e1ccda8a 100644 --- a/tests/browser/pair-node.spec.mjs +++ b/tests/browser/pair-node.spec.mjs @@ -17,7 +17,7 @@ test.beforeEach(async({page,browserName})=>{ window.fixtureApp={registerExtension(extension){this.extension=extension;},ui:{settings},graph:{_nodes:[],setDirtyCanvas(){}},canvas:{},graphToPrompt:async()=>({})}; await import('/extensions/promptmanager/prompt_manager.js'); class Node { - constructor(){this.id=1;this.type='PromptManagerPair';this.widgets=['text','negative_text','category','tags','search_text'].map(name=>({name,type:'text',value:'',options:{},inputEl:document.createElement('textarea')}));this.inputs=[...['clip','prepend_text','append_text'].map(name=>({name,type:name==='clip'?'CLIP':'STRING',link:null})),...['text','negative_text','category','tags','search_text'].map(name=>({name,type:'STRING',link:null,widget:{name}}))];this.properties={};this.size=[1100,700];} + constructor(){this.id=1;this.type='PromptManagerPair';this.widgets=[...['text','negative_text','category','tags','search_text'].map(name=>({name,type:'text',value:'',options:{},inputEl:document.createElement('textarea')})),{name:'save_to_library',type:'toggle',value:true,options:{}}];this.inputs=[...['clip','prepend_text','append_text'].map(name=>({name,type:name==='clip'?'CLIP':'STRING',link:null})),...['text','negative_text','category','tags','search_text'].map(name=>({name,type:'STRING',link:null,widget:{name}})),{name:'save_to_library',type:'BOOLEAN',link:null,widget:{name:'save_to_library'}}];this.properties={};this.size=[1100,700];} findInputByType(type){const index=this.inputs.findIndex(input=>input.type===type&&input.link==null);return index<0?undefined:{index,slot:this.inputs[index]};} addWidget(type,name,value,callback,options){const widget={type,name,value,callback,options};this.widgets.push(widget);return widget;} addDOMWidget(name,type,element){const host=document.createElement('div');host.style.cssText='height:700px;width:1100px;max-width:100%';host.className='fixture-dom-widget';host.append(element);document.body.append(host);return {element};} @@ -34,6 +34,18 @@ test.afterEach(async({page,browserName})=>{await saveCoverage(page,browserName); async function selectPair(page){await page.getByRole('button',{name:'Recent',exact:true}).click();await page.locator('.pm-editor-result').click();} async function draft(page,positive='Draft positive',negative='Draft negative'){await page.getByRole('textbox',{name:'Positive prompt',exact:true}).fill(positive);await page.getByRole('textbox',{name:'Negative prompt',exact:true}).fill(negative);} +test('paired editor Save to library switch keeps the widget order and loads older workflows on (#90)',async({page})=>{ + await expect(page.locator('.pm-pair')).toBeVisible(); + const toggle=page.getByRole('checkbox',{name:'Save to library',exact:true}); + await expect(toggle).toBeChecked(); + expect(await page.evaluate(()=>node.widgets.map(widget=>widget.name))).toEqual(['text','negative_text','category','tags','search_text','prepend_text','append_text','save_to_library']); + await toggle.uncheck(); + expect(await page.evaluate(()=>node.serialize().widgets_values[7])).toBe(false); + await expect(page.locator('.pm-pair > .pm-editor-status[role=status]').last()).toHaveText('Save to library is off: runs of this node are not added to the library.'); + await page.evaluate(()=>node.configure({widgets_values:['P','N','','','','','',''],properties:{}})); + await expect(toggle).toBeChecked(); +}); + test('paired editor previews both texts before explicitly replacing both',async({page})=>{ await expect(page.locator('.pm-pair')).toBeVisible(); await draft(page); diff --git a/tests/comfyui/execution-text-link.spec.mjs b/tests/comfyui/execution-text-link.spec.mjs index 20e60de6..813a0345 100644 --- a/tests/comfyui/execution-text-link.spec.mjs +++ b/tests/comfyui/execution-text-link.spec.mjs @@ -16,8 +16,8 @@ installHostHooks(test, ['PromptManager', 'PromptManagerText', 'PromptManagerFocu // `primitive`: a core string node the queue-time resolver follows. `legacy`: the frontend's virtual Primitive node, // whose value is queued as literal text. `preview`: PrimitiveString -> PreviewAny, the shape of the maintainer's // TextGenerate -> PreviewAny chain, whose text is only known when the node runs. -function build(page, { kind, positive, negative, prefix }) { - return page.evaluate(async ({ kind, positive, negative, prefix }) => { +function build(page, { kind, positive, negative, prefix, saveToLibrary }) { + return page.evaluate(async ({ kind, positive, negative, prefix, saveToLibrary }) => { const { app } = await import('/scripts/app.js'); app.graph.clear(); const add = (type, x, y) => { const node = LiteGraph.createNode(type); app.graph.add(node); node.pos = [x, y]; return node; }; @@ -28,6 +28,7 @@ function build(page, { kind, positive, negative, prefix }) { const set = (name, value) => { const widget = prompt.widgets.find(item => item.name === name); widget.value = value; if (widget.inputEl) widget.inputEl.value = value; }; set('text', positive.typed); if (negative) set('negative_text', negative.typed); + if (saveToLibrary !== undefined) prompt.widgets.find(item => item.name === 'save_to_library').value = saveToLibrary; const drive = (name, side, y) => { if (!side.via) return; if (side.via === 'legacy') { @@ -73,7 +74,7 @@ function build(page, { kind, positive, negative, prefix }) { await prompt._pmTextSerializerReady; const { output, workflow } = await app.graphToPrompt(); return { output, workflow, promptNode: String(prompt.id) }; - }, { kind, positive, negative, prefix }); + }, { kind, positive, negative, prefix, saveToLibrary }); } async function run(execution, page, testCase, built) { @@ -195,6 +196,47 @@ for (const [label, linked, negativeVia] of [['positive', ['text'], 'primitive'], }); } +// #90: with Save to library off the run still produces its image, but PromptManager saves, counts and links nothing, +// and the run's history records no prompt text for it. +for (const kind of ['PromptManager', 'PromptManagerText', 'PromptManagerFocused', 'PromptManagerSplit', 'PromptManagerPair']) { + test(`${kind} with Save to library off saves, counts and links nothing (#90)`, async ({ execution, page }) => { + test.setTimeout(90000); + const text = unique(`Not saved ${kind}`); + const negative = kind === 'PromptManagerPair' ? { typed: unique('Not saved negative') } : undefined; + const prefix = `notsaved/${randomUUID()}`; + const { output, workflow, promptNode } = await build(page, { kind, positive: { typed: text }, negative, prefix, saveToLibrary: false }); + expect(output[promptNode].inputs.save_to_library).toBe(false); + const id = randomUUID(); + const response = await execution.request.post('/prompt', { data: { prompt: output, prompt_id: id, extra_data: { extra_pnginfo: { workflow } } } }); + expect(response.ok(), await response.text()).toBe(true); + let item; + await expect.poll(async () => { + item = (await (await execution.request.get(`/history/${id}`)).json())[id]; + return item?.status?.status_str ?? null; + }, { timeout: 20000 }).toBe('success'); + let result; + await expect.poll(async () => { + result = await readExecutionRuns(execution.databasePath, id); + const run = result.rows[0]; + return Boolean(run && result.captures[run.run_id]); + }, { timeout: 10000, message: 'the run reaches SQLite' }).toBe(true); + const run = result.rows[0]; + for (const context of result.captures[run.run_id].contexts || []) { + expect(context.positive_prompt_id ?? null).toBeNull(); + expect(context.pair_id ?? null).toBeNull(); + } + expect((result.usage[run.run_id] || []).every(entry => entry.run_count === 0)).toBe(true); + const files = Object.values(item.outputs).flatMap(value => value.images || []).map(image => image.filename); + expect(files.length).toBeGreaterThan(0); + await scan(execution); + expect(Object.values(await linkedPrompts(execution, files))).toEqual(files.map(() => null)); + for (const query of [text, negative?.typed].filter(Boolean)) { + const found = await (await execution.request.get(`/prompt_manager/search?text=${encodeURIComponent(query)}`)).json(); + expect(found.results ?? [], query).toEqual([]); + } + }); +} + // #347: a cached PromptManager node counts every job, as an executing one does. Jobs after the first change only // the sampler seed, so ComfyUI serves the PromptManager node (and its PreviewAny source) from its output cache. const COUNTS = `import json, sqlite3, sys, pathlib diff --git a/tests/comfyui/text-link.spec.mjs b/tests/comfyui/text-link.spec.mjs index 6bfaf01a..612d34b4 100644 --- a/tests/comfyui/text-link.spec.mjs +++ b/tests/comfyui/text-link.spec.mjs @@ -370,6 +370,7 @@ test('workflows saved before #343 load unchanged and their linked text keeps wor const nodes = Object.fromEntries(app.graph._nodes.map(node => [node.id, node])); const reloaded = app.graph.serialize(); return { + saves: Object.fromEntries(Object.entries(output).filter(([, node]) => node.class_type.startsWith('PromptManager')).map(([id, node]) => [id, node.inputs.save_to_library])), values: Object.fromEntries(reloaded.nodes.map(node => [node.id, node.widgets_values])), links: Object.fromEntries(Object.entries(output).map(([id, node]) => [id, { text: node.inputs.text, negative_text: node.inputs.negative_text }])), widgetBound: Object.fromEntries(Object.values(nodes).filter(node => node.type.startsWith('PromptManager')).map(node => [node.id, node.inputs.filter(input => ['text', 'negative_text'].includes(input.name) && input.widget).map(input => input.name)])), @@ -377,8 +378,16 @@ test('workflows saved before #343 load unchanged and their linked text keeps wor }, workflow); for (const node of workflow.nodes) { // Every saved widget value comes back as it was saved, except the typed text main lost under a link (null). - if (node.widgets_values?.[0] !== null) expect(result.values[node.id], `${node.type} ${node.id}`).toEqual(node.widgets_values); + // #90: the last saved value is the editor's own slot (''), where the Save to library switch is now: it loads on. + if (node.widgets_values?.[0] === null) continue; + if (!node.type.startsWith('PromptManager')) { expect(result.values[node.id], `${node.type} ${node.id}`).toEqual(node.widgets_values); continue; } + const saved = node.widgets_values.length - 1; + expect(result.values[node.id].slice(0, saved), `${node.type} ${node.id}`).toEqual(node.widgets_values.slice(0, saved)); + expect(result.values[node.id][saved], `${node.type} ${node.id} save_to_library`).toBe(true); } + // Every PromptManager node of the old workflow queues with Save to library on. + for (const save of Object.values(result.saves)) expect(save).toBe(true); + expect(Object.keys(result.saves).length).toBeGreaterThan(0); expect(result.links).toMatchObject({ 1: { text: 'Saved PromptManager text' }, 2: { text: 'Saved PromptManagerText text' }, 3: { text: 'Saved PromptManagerFocused text' }, 4: { text: 'Saved PromptManagerSplit text' }, 5: { text: 'Saved PromptManagerPair text', negative_text: 'Saved pair negative' }, diff --git a/tests/js/save-to-library.test.js b/tests/js/save-to-library.test.js new file mode 100644 index 00000000..7d64aa0e --- /dev/null +++ b/tests/js/save-to-library.test.js @@ -0,0 +1,68 @@ +// #90: the node's save_to_library switch (web/js/save-to-library.js). +const test = require('node:test'); +const assert = require('node:assert/strict'); +const source = import('../../web/js/save-to-library.js'); + +function classicNode(saved) { + const names = ['text', 'category', 'tags', 'search_text', 'prepend_text', 'append_text', 'save_to_library', 'prompt_manager_search_ui']; + const widgets = names.map(name => ({name, value: name === 'save_to_library' ? true : ''})); + const node = {widgets, widgets_values: saved ? [...saved] : undefined}; + // What a host configure does: each saved value goes to the widget at its index. + (saved || []).forEach((value, index) => { if (widgets[index]) widgets[index].value = value; }); + return node; +} + +test('an older workflow stored the editor slot where the switch is now: it loads on', async () => { + const {normalizeSaveWidget, savesToLibrary} = await source; + const node = classicNode(['a cat', 'c', 't', '', '', '', '']); + assert.equal(node.widgets[6].value, '', 'the host put the old DOM slot value into the switch'); + assert.equal(normalizeSaveWidget(node), true); + assert.equal(node.widgets[6].value, true); + assert.equal(node.widgets_values[6], true); + assert.equal(savesToLibrary(node), true); + assert.deepEqual(node.widgets_values.slice(0, 6), ['a cat', 'c', 't', '', '', ''], 'no earlier value moved'); +}); + +test('null, undefined and missing slots load on; saved booleans are kept', async () => { + const {normalizeSaveWidget, savesToLibrary} = await source; + for (const value of [null, undefined, 0, 'false']) { + const node = classicNode(['a', '', '', '', '', '', value]); + normalizeSaveWidget(node); + assert.equal(savesToLibrary(node), true, String(value)); + } + const short = classicNode(['a', '', '', '', '', '']); + assert.equal(normalizeSaveWidget(short), false); + assert.equal(savesToLibrary(short), true); + const off = classicNode(['a', '', '', '', '', '', false]); + assert.equal(normalizeSaveWidget(off), false); + assert.equal(savesToLibrary(off), false); +}); + +test('a node without the widget (an older backend) saves', async () => { + const {normalizeSaveWidget, savesToLibrary} = await source; + assert.equal(savesToLibrary({widgets: [{name: 'text', value: ''}]}), true); + assert.equal(normalizeSaveWidget({widgets: []}), false); + assert.equal(savesToLibrary(undefined), true); +}); + +test('the switch moves after the editor fallbacks so every frontend has one order', async () => { + const {placeSaveWidgetLast} = await source; + const widgets = ['text', 'category', 'tags', 'search_text', 'save_to_library', 'prepend_text', 'append_text'].map(name => ({name})); + const node = {widgets}; + assert.equal(placeSaveWidgetLast(node), true); + assert.deepEqual(node.widgets.map(widget => widget.name), ['text', 'category', 'tags', 'search_text', 'prepend_text', 'append_text', 'save_to_library']); + assert.equal(placeSaveWidgetLast(node), false, 'already last'); + assert.equal(placeSaveWidgetLast({widgets: [{name: 'text'}]}), false); +}); + +test('setting the switch updates the widget, its saved value and notifies the host', async () => { + const {setSaveToLibrary, savesToLibrary} = await source; + const calls = []; + const node = classicNode(['a', '', '', '', '', '', true]); + node.widgets[6].callback = value => calls.push(value); + setSaveToLibrary(node, false); + assert.equal(savesToLibrary(node), false); + assert.equal(node.widgets_values[6], false); + assert.deepEqual(calls, [false]); + setSaveToLibrary({widgets: []}, false); +}); diff --git a/tests/test_cached_runs.py b/tests/test_cached_runs.py index d6ea4954..b337b6a6 100644 --- a/tests/test_cached_runs.py +++ b/tests/test_cached_runs.py @@ -210,13 +210,24 @@ class JobCase(unittest.TestCase): node = self.node(kind) self.context = (self.context[0], index) clip = mock.Mock() + # The node receives its switch as the queued graph holds it (#90) + save = g[node_id]["inputs"].get("save_to_library", True) if kind == "PromptManagerText": - return node.process_text(text, prompt=g, unique_id=node_id) + return node.process_text( + text, prompt=g, unique_id=node_id, save_to_library=save + ) if kind == "PromptManagerPair": return node.encode_prompt( - clip, text, negative_text=negative, prompt=g, unique_id=node_id + clip, + text, + negative_text=negative, + prompt=g, + unique_id=node_id, + save_to_library=save, ) - return node.encode_prompt(clip, text, prompt=g, unique_id=node_id) + return node.encode_prompt( + clip, text, prompt=g, unique_id=node_id, save_to_library=save + ) def cache(self, node_id, *results): """Store executed results (one per list index) as ComfyUI's cache entry.""" @@ -313,6 +324,35 @@ class TestCachedJobsCount(JobCase): self.job(g, cached=["pm"]) self.assertEqual(self.db.search_prompts(), []) + def test_disabled_node_then_cached_jobs_counts_0(self): + # #90 x #347: a node with Save to library off records no receipt and is + # no run prompt, so its cached jobs replay nothing. The prompt exists + # (an enabled run saved it), so any replayed count would show. + for kind in ( + "PromptManager", + "PromptManagerFocused", + "PromptManagerSplit", + "PromptManagerText", + "PromptManagerPair", + ): + with self.subTest(kind=kind): + text = f"{LLM} not saved by {kind}" + negative = "blurry" if kind == "PromptManagerPair" else "" + self.first(graph(kind, negative_text=negative), text, negative) + before = (self.count(text), self.pair_count(text, negative)) + + def off(seed): + g = graph(kind, negative_text=negative, seed=seed) + g["pm"]["inputs"]["save_to_library"] = False + return g + + self.first(off(1), text, negative) + self.job(off(2), cached=["pm"]) + self.job(off(3), cached=["pm"]) + after = (self.count(text), self.pair_count(text, negative)) + self.assertEqual(before[0], 1) + self.assertEqual(after, before) + def test_alternating_upstream_values_each_count_their_own_prompt(self): g = graph() self.first(g, text="value A") diff --git a/tests/test_save_to_library.py b/tests/test_save_to_library.py new file mode 100644 index 00000000..3d8bee47 --- /dev/null +++ b/tests/test_save_to_library.py @@ -0,0 +1,508 @@ +"""#90 item 3: a node's ``save_to_library`` switch. + +Off: PromptManager ignores the run completely (no prompt saved, no run count, +no image link), while the node still returns its normal outputs. On (the +default, and every workflow or image made before the switch existed): as before. +""" + +import logging +import os +import sys +import unittest +from pathlib import Path +from unittest import mock + +sys.path.insert(0, os.path.dirname(os.path.dirname(os.path.abspath(__file__)))) + +try: + from tests.test_prompt_attribution import MonitorCase, NodeCase + from tests.test_execution_contexts import load_module +except ImportError: # discovered with tests/ as the top-level directory + from test_prompt_attribution import MonitorCase, NodeCase + from test_execution_contexts import load_module + +from prompt_manager import PromptManager, PromptManagerFocused, PromptManagerSplit +from prompt_manager_pair import PromptManagerPair +from prompt_manager_text import PromptManagerText +from prompt_search_list import PromptSearchList +from utils.hashing import generate_prompt_hash +from utils.prompt_graph import ( + intrinsic_pair_nodes, + positive_opt_outs, + run_prompt_components, + run_prompt_nodes, + save_setting, +) +from utils.run_keys import prompt_node_key +from utils.usage_tracking import ( + PendingFirstUse, + QueuedRuns, + handle_queued_prompt, + on_dequeue, + queued_pair_runs, + queued_prompt_runs, +) + +POSITIVE = "a lighthouse at dusk" +NEGATIVE = "blurry" +NODE_CLASSES = ( + PromptManager, + PromptManagerText, + PromptManagerFocused, + PromptManagerSplit, + PromptManagerPair, +) + + +def run_graph(save=None, kind="PromptManager", negative_save=None): + """A PromptManager positive feeding a KSampler; ``save`` None leaves it out.""" + inputs = {"text": POSITIVE, "clip": ["4", 1]} + if save is not None: + inputs["save_to_library"] = save + negative_inputs = {"text": NEGATIVE, "clip": ["4", 1]} + if negative_save is not None: + negative_inputs["save_to_library"] = negative_save + return { + "1": {"class_type": kind, "inputs": inputs}, + "2": {"class_type": "PromptManager", "inputs": negative_inputs}, + "4": {"class_type": "CheckpointLoaderSimple", "inputs": {"ckpt_name": "m"}}, + "3": { + "class_type": "KSampler", + "inputs": { + "model": ["4", 0], + "positive": ["1", 0], + "negative": ["2", 0], + "latent_image": ["6", 0], + }, + }, + "6": {"class_type": "EmptyLatentImage", "inputs": {"batch_size": 1}}, + } + + +def pair_graph(save=None): + inputs = {"text": POSITIVE, "negative_text": NEGATIVE, "clip": ["4", 1]} + if save is not None: + inputs["save_to_library"] = save + return { + "1": {"class_type": "PromptManagerPair", "inputs": inputs}, + "4": {"class_type": "CheckpointLoaderSimple", "inputs": {"ckpt_name": "m"}}, + "3": { + "class_type": "KSampler", + "inputs": { + "model": ["4", 0], + "positive": ["1", 0], + "negative": ["1", 1], + "latent_image": ["6", 0], + }, + }, + "6": {"class_type": "EmptyLatentImage", "inputs": {"batch_size": 1}}, + } + + +class TestInputDeclaration(unittest.TestCase): + def test_every_prompt_manager_node_declares_a_boolean_defaulting_on(self): + for cls in NODE_CLASSES: + optional = cls.INPUT_TYPES()["optional"] + kind, options = optional["save_to_library"] + self.assertEqual(kind, "BOOLEAN", cls.__name__) + self.assertIs(options["default"], True, cls.__name__) + # Declared last: no saved widget value of an older workflow shifts + self.assertEqual(list(optional)[-1], "save_to_library", cls.__name__) + + def test_search_list_has_no_switch(self): + inputs = PromptSearchList.INPUT_TYPES() + self.assertNotIn("save_to_library", inputs.get("optional", {})) + self.assertNotIn("save_to_library", inputs.get("required", {})) + + +class TestSaveSetting(unittest.TestCase): + def test_missing_input_is_on(self): + self.assertIs(save_setting(run_graph(), "1"), True) + + def test_literals_follow_comfyui_boolean_coercion(self): + for value, expected in ( + (True, True), + (False, False), + ("", False), + (0, False), + ({"__value__": False}, False), + ({"__value__": True}, True), + ): + with self.subTest(value=value): + self.assertIs(save_setting(run_graph(save=value), "1"), expected) + + def test_link_to_primitive_boolean_is_read(self): + for value in (True, False): + graph = run_graph(save=["9", 0]) + graph["9"] = {"class_type": "PrimitiveBoolean", "inputs": {"value": value}} + self.assertIs(save_setting(graph, "1"), value) + + def test_link_to_an_unknown_node_is_not_knowable(self): + graph = run_graph(save=["9", 0]) + graph["9"] = {"class_type": "SomeCustomSwitch", "inputs": {}} + self.assertIsNone(save_setting(graph, "1")) + self.assertIsNone(save_setting(run_graph(save=["missing", 0]), "1")) + + +class TestGraphRoles(unittest.TestCase): + def test_opted_out_positive_is_not_a_run_prompt(self): + graph = run_graph(save=False) + self.assertEqual(run_prompt_nodes(graph), []) + self.assertEqual(run_prompt_components(graph), []) + self.assertEqual(positive_opt_outs(graph), ["1"]) + + def test_enabled_positive_is_unchanged(self): + for save in (None, True): + graph = run_graph(save=save) + self.assertEqual(run_prompt_nodes(graph), ["1"]) + self.assertEqual(positive_opt_outs(graph), []) + + def test_opted_out_negative_is_no_positive_opt_out(self): + graph = run_graph(negative_save=False) + self.assertEqual(run_prompt_nodes(graph), ["1"]) + self.assertEqual(positive_opt_outs(graph), []) + + def test_walk_stops_at_an_opted_out_node(self): + graph = run_graph(save=False) + graph["1"]["inputs"]["text"] = ["7", 0] + graph["7"] = {"class_type": "PromptManagerText", "inputs": {"text": "x"}} + self.assertEqual(run_prompt_nodes(graph), []) + + def test_sampler_less_fallback_skips_opted_out_nodes(self): + graph = { + "1": { + "class_type": "PromptManagerText", + "inputs": {"text": "a", "save_to_library": False}, + }, + "2": {"class_type": "PromptManagerText", "inputs": {"text": "b"}}, + } + self.assertEqual(run_prompt_nodes(graph), ["2"]) + self.assertEqual(run_prompt_components(graph), [("2", "text")]) + self.assertEqual(positive_opt_outs(graph), ["1"]) + + def test_opted_out_pair_is_not_an_intrinsic_pair(self): + self.assertEqual(intrinsic_pair_nodes(pair_graph()), ["1"]) + self.assertEqual(intrinsic_pair_nodes(pair_graph(save=False)), []) + + +class TestQueueHook(unittest.TestCase): + def test_opted_out_node_is_never_stashed(self): + self.assertEqual(queued_prompt_runs(run_graph(save=False)), []) + self.assertEqual(queued_pair_runs(pair_graph(save=False)), []) + + def test_unknown_setting_is_left_to_the_node(self): + graph = run_graph(save=["9", 0]) + graph["9"] = {"class_type": "SomeCustomSwitch", "inputs": {}} + self.assertEqual(queued_prompt_runs(graph), []) + pair = pair_graph(save=["9", 0]) + pair["9"] = {"class_type": "SomeCustomSwitch", "inputs": {}} + self.assertEqual(queued_pair_runs(pair), []) + + def test_primitive_boolean_on_still_counts(self): + graph = run_graph(save=["9", 0]) + graph["9"] = {"class_type": "PrimitiveBoolean", "inputs": {"value": True}} + self.assertEqual( + queued_prompt_runs(graph), [(generate_prompt_hash(POSITIVE), POSITIVE)] + ) + + def test_dequeue_counts_nothing_for_a_cached_opted_out_node(self): + """A cached node never executes: only the queued graph can say it is off.""" + db = mock.Mock() + db.get_prompt_by_hash.return_value = {"id": 5} + queued, pending = QueuedRuns(), PendingFirstUse() + request = {"prompt": run_graph(save=False), "prompt_id": "job-1"} + handle_queued_prompt(request, queued, pair_queued=QueuedRuns()) + on_dequeue("job-1", queued, lambda: db, pending, pair_queued=QueuedRuns()) + db.record_prompt_use.assert_not_called() + self.assertEqual(pending.consume(generate_prompt_hash(POSITIVE)), 0) + + +class TestNodesSaveNothing(NodeCase): + def run_node(self, cls, graph, save): + node = self.node(cls) + node.record_pair = mock.Mock() + kwargs = {"prompt": graph, "unique_id": "1", "save_to_library": save} + if cls is PromptManagerText: + return node, node.process_text(POSITIVE, **kwargs) + if cls is PromptManagerPair: + return node, node.encode_prompt( + self.clip(), POSITIVE, negative_text=NEGATIVE, **kwargs + ) + return node, node.encode_prompt(self.clip(), POSITIVE, **kwargs) + + def test_off_saves_counts_and_tracks_nothing_but_returns_outputs(self): + for cls in NODE_CLASSES: + with self.subTest(node=cls.__name__): + graph = ( + pair_graph(save=False) + if cls is PromptManagerPair + else run_graph(save=False, kind=cls.__name__) + ) + node, result = self.run_node(cls, graph, False) + self.assertEqual(self.count_rows("prompts"), 0) + self.assertEqual(self.count_rows("prompt_pairs"), 0) + self.assertEqual(self.tracker.keyed_snapshot(), {}) + self.assertEqual(self.tracker._prompt_queue, []) + node.comfyui_integration.register_prompt.assert_not_called() + if cls is PromptManagerText: + self.assertEqual(result, (POSITIVE,)) + elif cls is PromptManagerPair: + self.assertEqual(result[0], ("cond", POSITIVE)) + self.assertEqual(result[1], ("cond", NEGATIVE)) + self.assertEqual(result[2:], (POSITIVE, NEGATIVE)) + else: + self.assertEqual(result, (("cond", POSITIVE), POSITIVE)) + + def test_off_without_graph_context_still_saves_nothing(self): + node = self.node(PromptManager) + node.encode_prompt(self.clip(), POSITIVE, save_to_library=False) + self.assertEqual(self.count_rows("prompts"), 0) + self.assertEqual(self.tracker._prompt_queue, []) + + def test_off_leaves_an_existing_prompt_uncounted(self): + prompt_id = self.save(POSITIVE) + self.run_node(PromptManager, run_graph(save=False), False) + row = self.db.get_prompt_by_id(prompt_id) + self.assertFalse(row.get("run_count") or 0) + + def test_on_is_unchanged(self): + self.run_node(PromptManager, run_graph(save=True), True) + self.assertIsNotNone(self.db.get_prompt_by_hash(generate_prompt_hash(POSITIVE))) + self.assertIn( + prompt_node_key(run_graph(save=True), "1"), self.tracker.keyed_snapshot() + ) + + def test_unknown_setting_on_at_run_time_counts_at_execution(self): + """The hook could not read a wired switch, so the node counts its run.""" + graph = run_graph(save=["9", 0]) + graph["9"] = {"class_type": "SomeCustomSwitch", "inputs": {}} + prompt_id = self.save(POSITIVE) + self.run_node(PromptManager, graph, True) + self.assertEqual(self.db.get_prompt_by_id(prompt_id)["run_count"], 1) + + +class TestWiredSwitchOffMarksItsRun(NodeCase): + """Review finding: a wired switch off at run time keeps the node a run prompt.""" + + def wired_graph(self): + graph = run_graph(save=["9", 0]) + graph["9"] = {"class_type": "SomeCustomSwitch", "inputs": {}} + return graph + + def test_an_earlier_entry_of_the_key_never_stands_in(self): + graph = self.wired_graph() + key = prompt_node_key(graph, "1") + node = self.node(PromptManager) + node.encode_prompt( + self.clip(), POSITIVE, prompt=graph, unique_id="1", save_to_library=True + ) + node.encode_prompt( + self.clip(), POSITIVE, prompt=graph, unique_id="1", save_to_library=False + ) + latest, _run, _count = self.tracker.keyed_snapshot()[key] + self.assertTrue(latest.get("placeholder")) + + def test_literal_off_marks_nothing(self): + node = self.node(PromptManager) + node.encode_prompt( + self.clip(), + POSITIVE, + prompt=run_graph(save=False), + unique_id="1", + save_to_library=False, + ) + self.assertEqual(self.tracker.keyed_snapshot(), {}) + + +class TestMonitorLeavesOptedOutFilesUnlinked(MonitorCase): + def test_existing_prompt_is_not_linked_from_metadata(self): + self.save(POSITIVE) + self.assertIsNone(self.process({"prompt": run_graph(save=False)})) + + def test_find_prompt_reports_opted_out(self): + self.save(POSITIVE) + state, prompt = self.handler._find_prompt_from_metadata( + {"prompt": run_graph(save=False)} + ) + self.assertEqual((state, prompt), ("opted_out", None)) + + def test_mixed_positive_settings_leave_the_file_unlinked(self): + self.save(POSITIVE) + graph = run_graph(save=True) + graph["8"] = { + "class_type": "PromptManager", + "inputs": {"text": "other", "save_to_library": False}, + } + graph["5"] = { + "class_type": "KSampler", + "inputs": { + "model": ["4", 0], + "positive": ["8", 0], + "negative": ["2", 0], + "latent_image": ["6", 0], + }, + } + self.assertIsNone(self.process({"prompt": graph})) + + def test_workflow_only_metadata_with_switch_off_is_opted_out(self): + self.save(POSITIVE) + metadata = { + "text_encoder_nodes": [ + { + "type": "PromptManager", + "inputs": [], + "widgets_values": [POSITIVE, "", "", "", "", "", False], + } + ] + } + state, _prompt = self.handler._find_prompt_from_metadata(metadata) + self.assertEqual(state, "opted_out") + + def test_workflow_only_ignores_muted_nodes_and_reads_subgraphs(self): + from utils.prompt_graph import metadata_opted_out + + node = { + "type": "PromptManager", + "widgets_values": [POSITIVE, "", "", "", "", "", False], + } + self.assertFalse( + metadata_opted_out({"workflow": {"nodes": [{**node, "mode": 4}]}}) + ) + self.assertTrue( + metadata_opted_out( + { + "workflow": { + "nodes": [], + "definitions": {"subgraphs": [{"nodes": [node]}]}, + } + } + ) + ) + + def test_unknown_setting_is_left_to_the_run(self): + self.save(POSITIVE) + graph = run_graph(save=["9", 0]) + graph["9"] = {"class_type": "SomeCustomSwitch", "inputs": {}} + state, _prompt = self.handler._find_prompt_from_metadata({"prompt": graph}) + self.assertEqual(state, "unknown") + + def test_enabled_positive_still_links(self): + prompt_id = self.save(POSITIVE) + self.assertEqual(self.process({"prompt": run_graph(save=True)}), prompt_id) + + +class TestLinkKeepsOptedOutNegativeOut(MonitorCase): + def setUp(self): + super().setUp() + # The real link, not the recording stub of MonitorCase + del self.handler.link_image_to_prompt + + def test_opted_out_negative_is_never_stored_as_a_pair(self): + prompt_id = self.save(POSITIVE) + metadata = {"prompt": run_graph(save=True, negative_save=False)} + self.handler.link_image_to_prompt(self.image, {"id": prompt_id}, metadata) + self.assertIsNone(self.db.get_prompt_by_hash(generate_prompt_hash(NEGATIVE))) + self.assertEqual(self.count_pairs(), 0) + linked = ( + self.db.model.get_connection() + .execute("SELECT prompt_id FROM generated_images") + .fetchall() + ) + self.assertEqual([row[0] for row in linked], [prompt_id]) + + def test_opted_out_positive_links_nothing(self): + prompt_id = self.save(POSITIVE) + metadata = {"prompt": run_graph(save=False)} + self.handler.link_image_to_prompt(self.image, {"id": prompt_id}, metadata) + count = ( + self.db.model.get_connection() + .execute("SELECT COUNT(*) FROM generated_images") + .fetchone()[0] + ) + self.assertEqual(count, 0) + + def count_pairs(self): + conn = self.db.model.get_connection() + return conn.execute("SELECT COUNT(*) FROM prompt_pairs").fetchone()[0] + + +class TestScanSkipsOptedOutFiles(NodeCase): + def setUp(self): + super().setUp() + from py.api import admin + + self.mixin = admin.AdminRoutesMixin.__new__(admin.AdminRoutesMixin) + self.mixin.db = self.db + self.mixin.logger = logging.getLogger("test.save_to_library.scan") + self.file = Path(self.tmp.name) / "out.png" + + def test_opted_out_file_stores_nothing(self): + status = self.mixin._ingest_scanned_file( + self.file, {"prompt": run_graph(save=False)} + ) + self.assertEqual(status, "not_saved") + self.assertEqual(self.count_rows("prompts"), 0) + self.assertEqual(self.count_rows("generated_images"), 0) + + def test_opted_out_file_is_never_queued_for_review(self): + ambiguous = [] + graph = run_graph(save=False) + graph["5"] = dict(graph["3"]) # a second sampler: ambiguous otherwise + graph["5"]["inputs"] = {**graph["3"]["inputs"], "positive": ["2", 0]} + status = self.mixin._ingest_scanned_file( + self.file, {"prompt": graph}, ambiguous=ambiguous + ) + self.assertEqual(status, "not_saved") + self.assertEqual(ambiguous, []) + + def test_opted_out_negative_stores_the_positive_only(self): + status = self.mixin._ingest_scanned_file( + self.file, {"prompt": run_graph(save=True, negative_save=False)} + ) + self.assertEqual(status, "added") + self.assertIsNotNone(self.db.get_prompt_by_hash(generate_prompt_hash(POSITIVE))) + self.assertIsNone(self.db.get_prompt_by_hash(generate_prompt_hash(NEGATIVE))) + self.assertEqual(self.count_rows("prompt_pairs"), 0) + + def test_wired_switch_is_left_to_the_live_run(self): + graph = run_graph(save=["9", 0]) + graph["9"] = {"class_type": "SomeCustomSwitch", "inputs": {}} + status = self.mixin._ingest_scanned_file(self.file, {"prompt": graph}) + self.assertEqual(status, "not_saved_wired") + self.assertEqual(self.count_rows("prompts"), 0) + + def test_status_has_a_scan_detail(self): + from py.api import admin + + group, row, _line = admin._scan_outcome(self.file, [], "not_saved") + self.assertEqual(group, "no_metadata") + self.assertIn("does not save", row["detail"]) + + +class TestHistoryRecordsNoText(unittest.TestCase): + def setUp(self): + self.m = load_module("execution_context_graph") + + def test_opted_out_role_has_no_sources_and_no_text(self): + recipe = self.m.project_contexts(run_graph(save=False), None).recipes[0] + self.assertEqual(recipe.positive.sources, ()) + self.assertIsNone(recipe.positive.inferred) + self.assertEqual(recipe.positive.reason, "not_saved") + # The enabled negative is recorded as before + self.assertEqual(recipe.negative.inferred.raw.value, NEGATIVE) + + def test_unknown_setting_records_no_text_either(self): + graph = run_graph(save=["9", 0]) + graph["9"] = {"class_type": "SomeCustomSwitch", "inputs": {}} + recipe = self.m.project_contexts(graph, None).recipes[0] + self.assertIsNone(recipe.positive.inferred) + self.assertEqual(recipe.positive.reason, "not_saved") + + def test_enabled_role_is_unchanged(self): + recipe = self.m.project_contexts(run_graph(save=True), None).recipes[0] + self.assertEqual(recipe.positive.inferred.raw.value, POSITIVE) + self.assertEqual(len(recipe.positive.sources), 1) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/vendor/comfyui-config.test.mjs b/tests/vendor/comfyui-config.test.mjs index 2cb9587c..bf5b1c0d 100644 --- a/tests/vendor/comfyui-config.test.mjs +++ b/tests/vendor/comfyui-config.test.mjs @@ -35,15 +35,16 @@ test('execution script discovers only the new matrix and preserves all 78 origin assert.match(executions, /\[nodes2-start\]/); // + the fresh-host library location case, + #342 image attribution, + #343 linked text, // + #332 image category routes on a generated output (image-attribution.spec.mjs, 2 projects), - // + #347 cached runtime-linked runs: five node kinds and eight one-renderer cases (execution-text-link.spec.mjs, 2 projects) - assert.match(executions, /Total: 118 tests in 5 files/); + // + #347 cached runtime-linked runs: five node kinds and eight one-renderer cases (execution-text-link.spec.mjs, 2 projects), + // + #90 Save to library off (five node types) + assert.match(executions, /Total: 128 tests in 5 files/); for (const project of ['classic-start', 'nodes2-start']) { const cases = executions.split('\n').filter(line => line.includes(`[${project}]`)); assert.equal(cases.filter(line => line.includes('execution-history.spec.mjs')).length, 15); assert.equal(cases.filter(line => line.includes('execution-outputs.spec.mjs')).length, 8); assert.equal(cases.filter(line => line.includes('library-location.spec.mjs')).length, 1); assert.equal(cases.filter(line => line.includes('image-attribution.spec.mjs')).length, 3); - assert.equal(cases.filter(line => line.includes('execution-text-link.spec.mjs')).length, 32); + assert.equal(cases.filter(line => line.includes('execution-text-link.spec.mjs')).length, 37); } assert.doesNotMatch(executions, /prompt-manager.spec/); }); diff --git a/tools/vendor/frontend-coverage-baseline.json b/tools/vendor/frontend-coverage-baseline.json index de6c2416..dd070d4b 100644 --- a/tools/vendor/frontend-coverage-baseline.json +++ b/tools/vendor/frontend-coverage-baseline.json @@ -107,6 +107,12 @@ "functions": 98, "statements": 98 }, + "save-to-library.js": { + "lines": 98, + "branches": 98, + "functions": 98, + "statements": 98 + }, "scan-progress.js": { "lines": 98, "branches": 98, diff --git a/utils/execution_context_capture.py b/utils/execution_context_capture.py index 3a07985f..0fd22d24 100644 --- a/utils/execution_context_capture.py +++ b/utils/execution_context_capture.py @@ -11,6 +11,7 @@ import weakref from .execution_context_budget import ContextBudget, PayloadLease, SealedContext from .execution_context_graph import ( + NOT_SAVED, GraphProjection, GraphRole, SourceRef, @@ -325,6 +326,8 @@ class ContextCapture: nonlocal ambiguous if role.reason == "source-transformed": return _unknown("unsupported") + if role.reason == NOT_SAVED: + return _unknown("unavailable") if not role.sources: return role.inferred or _unknown("unsupported") values = [ diff --git a/utils/execution_context_graph.py b/utils/execution_context_graph.py index 740cfe43..ee884a34 100644 --- a/utils/execution_context_graph.py +++ b/utils/execution_context_graph.py @@ -3,8 +3,13 @@ import re from dataclasses import dataclass from .execution_contexts import PromptSnapshot, TextEvidence, text_size +from .prompt_graph import save_setting from .prompt_pair_resolver import _DEFAULT_ADAPTERS +# A role whose PromptManager source has save_to_library off, or wired from a +# node the graph cannot read: no text is recorded for it (#90) +NOT_SAVED = "not_saved" + @dataclass(frozen=True) class SourceRef: @@ -332,6 +337,11 @@ class _Projector: self.transformed = False # Only the capture-source-to-consumer path determines transforms. sources = self.discover_sources(value, "conditioning") + if any( + save_setting(self.graph, source.node_id) is not True + for source in sources + ): + return GraphRole((), None, NOT_SAVED) snapshot, _ = self.text(value, "conditioning") return GraphRole( sources, snapshot, "source-transformed" if self.transformed else "" diff --git a/utils/image_monitor.py b/utils/image_monitor.py index 7c70d876..78cfcb10 100644 --- a/utils/image_monitor.py +++ b/utils/image_monitor.py @@ -37,7 +37,13 @@ from .hashing import generate_prompt_hash from .metadata_extractor import ComfyUIMetadataExtractor from .parallel import note_refusal, refused_at_shutdown from .logging_config import get_logger -from .prompt_graph import resolve_text, run_prompt_nodes +from .prompt_graph import ( + metadata_opted_out, + resolve_text, + run_prompt_nodes, + save_setting, + side_opted_out, +) from .prompt_pair_resolver import resolve_prompt_pair from .run_keys import image_run_keys from .usage_tracking import survive_switch @@ -450,6 +456,12 @@ class ImageGenerationHandler(FileSystemEventHandler): # that produced this image, independent of node caching and of any # negative-prompt PromptManager nodes in the workflow. state, current_prompt = self._find_prompt_from_metadata(metadata) + if state == "opted_out": + self.logger.info( + "Save to library is off for this run's prompt, skipping image: " + f"{image_path}" + ) + return if state == "foreign": # Another workflow saved this image: it belongs to no prompt of # ours, and must not consume a batch queue entry. @@ -715,16 +727,27 @@ class ImageGenerationHandler(FileSystemEventHandler): ``("linked", prompt)`` with a context dict holding 'id' and 'text'; ``("foreign", None)`` when the workflow has no positive PromptManager node, so the image belongs to no prompt of ours; + ``("opted_out", None)`` when a PromptManager node that would be the + run's prompt has its ``save_to_library`` switch off (#90): the image + is linked to no prompt, by no fallback; ``("unknown", None)`` when metadata can't decide (missing, a batch item whose text came from another node, or a lookup failure) and the caller's queue and tracker fallbacks apply. """ if not metadata: return "unknown", None + if metadata_opted_out(metadata): + return "opted_out", None + graph = metadata.get("prompt") + if isinstance(graph, dict) and any( + save_setting(graph, node_id) is None for node_id in run_prompt_nodes(graph) + ): + # A switch wired from a node the graph cannot read: only the node's + # own run (it saw the real value) can link the image (#90) + return "unknown", None prompt_text = None resolution = resolve_prompt_pair(metadata) - graph = metadata.get("prompt") if resolution["state"] == "resolved" and isinstance(graph, dict): positive = resolution["positive"] path = positive["provenance"].get("path", []) @@ -865,13 +888,23 @@ class ImageGenerationHandler(FileSystemEventHandler): metadata: Extracted metadata from the image file (workflow, parameters, etc.) """ + if metadata_opted_out(metadata): + # Reached from a run or history: the file's own graph says no (#90) + self.logger.info( + f"Save to library is off for {os.path.basename(image_path)}; " + "left unlinked" + ) + return try: resolution = resolve_prompt_pair(metadata) + graph = metadata.get("prompt") if isinstance(metadata, dict) else None + # An opted-out negative is never stored: link the positive alone + pair_allowed = not side_opted_out(graph, resolution.get("negative")) def link(): # One write; run again on the moved library when a Move # library switched while it waited (#249 S6b). - if resolution["state"] == "resolved": + if resolution["state"] == "resolved" and pair_allowed: positive = resolution["positive"] expected = self.db_manager.get_prompt_by_hash( generate_prompt_hash( diff --git a/utils/prompt_graph.py b/utils/prompt_graph.py index fc1ee362..b21021af 100644 --- a/utils/prompt_graph.py +++ b/utils/prompt_graph.py @@ -21,6 +21,11 @@ PROMPT_MANAGER_TYPES = frozenset( } ) +# A node's switch (#90): False means PromptManager ignores that node's runs +SAVE_INPUT = "save_to_library" +# Core nodes whose output is a known boolean: (class_type) -> input holding it +_BOOLEAN_SOURCES = {"PrimitiveBoolean": "value"} + # Upper bound on nodes expanded per graph; queued and embedded graphs are untrusted MAX_VISITS = 500 # Longest text resolve_text will build; StringConcatenate chains can double @@ -59,6 +64,116 @@ def _inputs(node: Any) -> Dict[str, Any]: return inputs if isinstance(inputs, dict) else {} +def _as_boolean(value: Any) -> bool: + """ComfyUI's coercion of a BOOLEAN input before the node runs.""" + if isinstance(value, dict) and "__value__" in value: + value = value["__value__"] + return bool(value) + + +def save_setting(graph: Any, node_id: Any) -> Optional[bool]: + """A PromptManager node's ``save_to_library`` as the node will receive it. + + True when the input is missing (every workflow and image from before the + switch), the literal coerced like ComfyUI does, the value of a linked core + ``PrimitiveBoolean``, and None when the switch is wired from a node whose + output cannot be known from the graph. + """ + node = graph.get(str(node_id)) if isinstance(graph, dict) else None + inputs = _inputs(node) + if SAVE_INPUT not in inputs: + return True + value = inputs[SAVE_INPUT] + if not _is_link(value): + return _as_boolean(value) + source = graph.get(str(value[0])) + if not isinstance(source, dict) or value[1] != 0: + return None + key = _BOOLEAN_SOURCES.get(str(source.get("class_type", ""))) + source_inputs = _inputs(source) + if key is None or key not in source_inputs or _is_link(source_inputs[key]): + return None + return _as_boolean(source_inputs[key]) + + +def opted_out(graph: Any, node_id: Any) -> bool: + """Whether a PromptManager node's switch is off: PromptManager ignores it.""" + return save_setting(graph, node_id) is False + + +def side_opted_out(graph: Any, side: Any) -> bool: + """Whether a resolved side's path passes a PromptManager node that is off. + + ``side`` is a resolver side (``positive`` or ``negative`` of a resolved + pair): its text must not be stored when any PromptManager node on its + provenance path has its switch off or wired from an unreadable node (#90). + """ + if not isinstance(graph, dict) or not isinstance(side, dict): + return False + provenance = side.get("provenance") + path = provenance.get("path") if isinstance(provenance, dict) else None + for step in path if isinstance(path, list) else []: + node_id = step.get("node_id") if isinstance(step, dict) else None + node = graph.get(str(node_id)) if node_id is not None else None + if ( + isinstance(node, dict) + and node.get("class_type") in PROMPT_MANAGER_TYPES + and save_setting(graph, node_id) is not True + ): + return True + return False + + +def _workflow_node_opted_out(node: Any) -> bool: + """A saved-workflow PromptManager node whose widget values hold ``False``. + + No other PromptManager widget holds a boolean, so the value is the switch. + """ + if not isinstance(node, dict): + return False + if (node.get("type") or node.get("class_type")) not in PROMPT_MANAGER_TYPES: + return False + if node.get("mode") in (2, 4): + return False # muted or bypassed: it did not run + values = node.get("widgets_values") + return isinstance(values, list) and any(value is False for value in values) + + +def metadata_opted_out(metadata: Any, *, unreadable: bool = False) -> bool: + """Whether a file's metadata says its run's prompt does not save (#90). + + The executed graph (``prompt``) decides when present: a node of + ``positive_opt_outs``, or with ``unreadable`` also a run prompt whose + switch is wired from a node the graph cannot read. Without one, the + saved workflow (``workflow`` nodes or the extractor's + ``text_encoder_nodes``) is opted out when a PromptManager node stores + ``False``. + """ + if not isinstance(metadata, dict): + return False + graph = metadata.get("prompt") + if isinstance(graph, dict): + if positive_opt_outs(graph): + return True + return unreadable and any( + save_setting(graph, node_id) is None for node_id in run_prompt_nodes(graph) + ) + workflow = metadata.get("workflow") + nodes = list(metadata.get("text_encoder_nodes") or []) + if isinstance(workflow, dict) and isinstance(workflow.get("nodes"), list): + nodes.extend(workflow["nodes"]) + definitions = workflow.get("definitions") + subgraphs = ( + definitions.get("subgraphs") if isinstance(definitions, dict) else None + ) + for subgraph in subgraphs if isinstance(subgraphs, list) else []: + inner = subgraph.get("nodes") if isinstance(subgraph, dict) else None + nodes.extend(inner if isinstance(inner, list) else []) + # Without the executed graph the node's role is unknown: any opted-out + # PromptManager node opts the file out (never a guessed link). + return any(_workflow_node_opted_out(node) for node in nodes[:MAX_VISITS]) + + def _positive_roots(graph: Dict[str, Any]) -> List[Any]: """Links into samplers' positive inputs (KSampler, CFGGuider, BasicGuider, ...). @@ -100,11 +215,21 @@ def has_sampler(graph: Any) -> bool: def positive_prompt_components(graph: Any) -> List[Tuple[str, str]]: - """Node/input identities reaching actual sampler positive wires.""" + """Node/input identities reaching actual sampler positive wires. + + A node whose switch is off is not one, and nothing upstream of it is + followed (#90): see ``positive_opt_outs``. + """ + return _walk_positive(graph)[0] + + +def _walk_positive(graph: Any) -> Tuple[List[Tuple[str, str]], List[str]]: + """(components of enabled nodes, opted-out nodes) on sampler positive wires.""" if not isinstance(graph, dict): - return [] + return [], [] found: List[Tuple[str, str]] = [] + skipped: List[str] = [] seen = set() stack = list(reversed(_positive_roots(graph))) while stack and len(seen) < MAX_VISITS: @@ -117,6 +242,10 @@ def positive_prompt_components(graph: Any) -> List[Tuple[str, str]]: if not isinstance(node, dict): continue if node.get("class_type") in PROMPT_MANAGER_TYPES: + if opted_out(graph, node_id): + if node_id not in skipped: + skipped.append(node_id) + continue field = "text" if node.get("class_type") == "PromptManagerPair": field = { @@ -132,7 +261,33 @@ def positive_prompt_components(graph: Any) -> List[Tuple[str, str]]: found.append(identity) continue stack.extend(reversed(_upstream_links(_inputs(node)))) - return found + return found, skipped + + +def _sampler_less_nodes(graph: Dict[str, Any]) -> List[str]: + """Every PromptManager node of a graph without a recognisable sampler.""" + return [ + str(node_id) + for node_id, node in graph.items() + if isinstance(node, dict) and node.get("class_type") in PROMPT_MANAGER_TYPES + ] + + +def positive_opt_outs(graph: Any) -> List[str]: + """PromptManager nodes with their switch off that would be a run's prompt. + + Those on a sampler's positive wires, or, in a graph without a recognisable + sampler, any. A run with one of them links no file to any prompt (#90): + the file's graph cannot always say which sampler wrote it. + """ + if not isinstance(graph, dict): + return [] + found, skipped = _walk_positive(graph) + if found or skipped or has_sampler(graph): + return skipped + return [ + node_id for node_id in _sampler_less_nodes(graph) if opted_out(graph, node_id) + ] def positive_prompt_nodes(graph: Any) -> List[str]: @@ -148,13 +303,13 @@ def positive_prompt_nodes(graph: Any) -> List[str]: def run_prompt_components(graph: Any) -> List[Tuple[str, str]]: """Actual positive components, retaining standalone main-editor fallback.""" - components = positive_prompt_components(graph) - if components or not isinstance(graph, dict) or has_sampler(graph): + components, skipped = _walk_positive(graph) + if components or skipped or not isinstance(graph, dict) or has_sampler(graph): return components return [ - (str(node_id), "text") - for node_id, node in graph.items() - if isinstance(node, dict) and node.get("class_type") in PROMPT_MANAGER_TYPES + (node_id, "text") + for node_id in _sampler_less_nodes(graph) + if not opted_out(graph, node_id) ] @@ -165,16 +320,16 @@ def run_prompt_nodes(graph: Any) -> List[str]: say), every PromptManager node counts, as before 3.2.4, so an exotic workflow never silently loses usage counting or image linking. When a sampler exists and no PromptManager node feeds its positive input, nothing counts: the only - PromptManager nodes are negative prompts. + PromptManager nodes are negative prompts. A node whose switch is off never + counts (#90). """ - positive = positive_prompt_nodes(graph) - if positive or not isinstance(graph, dict) or has_sampler(graph): - return positive - return [ - str(node_id) - for node_id, node in graph.items() - if isinstance(node, dict) and node.get("class_type") in PROMPT_MANAGER_TYPES - ] + return list( + dict.fromkeys( + node_id + for node_id, field in run_prompt_components(graph) + if field == "text" + ) + ) def _join_text_inputs( @@ -270,7 +425,18 @@ def resolve_text(graph: Any, node_id: str) -> Optional[str]: def intrinsic_pair_nodes(graph: Any) -> List[str]: - """Paired nodes whose matching outputs feed the same sampler, or standalone.""" + """Paired nodes whose matching outputs feed the same sampler, or standalone. + + A pair whose switch is off is never one (#90). + """ + return [ + node_id + for node_id in _intrinsic_pair_nodes(graph) + if not opted_out(graph, node_id) + ] + + +def _intrinsic_pair_nodes(graph: Any) -> List[str]: if not isinstance(graph, dict) or len(graph) > MAX_VISITS: return [] if not has_sampler(graph): diff --git a/utils/usage_tracking.py b/utils/usage_tracking.py index 6fb6d39c..761019da 100644 --- a/utils/usage_tracking.py +++ b/utils/usage_tracking.py @@ -30,7 +30,12 @@ from typing import Any, Callable, List, Optional, Tuple try: from .hashing import generate_prompt_hash from .logging_config import get_logger - from .prompt_graph import intrinsic_pair_nodes, resolve_text, run_prompt_components + from .prompt_graph import ( + intrinsic_pair_nodes, + resolve_text, + run_prompt_components, + save_setting, + ) from .prompt_pair_resolver import resolve_text_input from .pair_execution import clear_pair_execution from .queue_observers import subscribe_dequeue @@ -41,6 +46,7 @@ except ImportError: intrinsic_pair_nodes, resolve_text, run_prompt_components, + save_setting, ) from utils.prompt_pair_resolver import resolve_text_input from utils.pair_execution import clear_pair_execution @@ -127,10 +133,14 @@ def queued_prompt_runs(graph: Any) -> List[Run]: """(hash, text) of each positive PromptManager node whose text is knowable. Each hash once: the same text in two positive nodes is one run of the prompt. + A node whose switch is off is never counted; one whose switch is wired from + a node the graph cannot read counts itself when it executes (#90). """ runs: List[Run] = [] seen = set() for node_id, field in run_prompt_components(graph): + if save_setting(graph, node_id) is not True: + continue node = graph.get(node_id, {}) if isinstance(graph, dict) else {} text = ( resolve_text_input(graph, node_id, field) @@ -156,6 +166,8 @@ def queued_pair_runs(graph: Any) -> List[Tuple[str, str, str]]: """Resolve raw paired recipes only when both outputs form the used pair.""" found, seen = [], set() for node_id in intrinsic_pair_nodes(graph): + if save_setting(graph, node_id) is not True: + continue # wired switch: the node counts itself (#90) positive = resolve_text_input(graph, node_id, "text") negative = resolve_text_input(graph, node_id, "negative_text") if positive is None or not positive.strip() or negative is None: diff --git a/web/comfy/prompt_manager.js b/web/comfy/prompt_manager.js index 51c1bdca..706c9d78 100644 --- a/web/comfy/prompt_manager.js +++ b/web/comfy/prompt_manager.js @@ -6,6 +6,7 @@ const { installClassicControls } = await import(api.fileURL('/prompt_manager/js/ const { installFocusedNode, installSplitNode, installPairNode } = await import(api.fileURL('/prompt_manager/js/focused-node.js')); const { installPromptBridge } = await import(api.fileURL('/prompt_manager/js/prompt-bridge-host.js')); const { collapseLinkedWidget, isLinked, linkNoteText, primitiveTextValue, storedWidgetValue } = await import(api.fileURL('/prompt_manager/js/text-links.js')); +const { normalizeSaveWidget } = await import(api.fileURL('/prompt_manager/js/save-to-library.js')); const promptManagerTypes = new Set(['PromptManager', 'PromptManagerText', 'PromptManagerFocused', 'PromptManagerSplit', 'PromptManagerPair']); // ============================================================================ @@ -525,6 +526,8 @@ app.registerExtension({ if (originalConfigure) { originalConfigure.call(this, data); } + // #90: an older workflow loads with Save to library on + normalizeSaveWidget(this); if (data._userHasResized) { this._userHasResized = data._userHasResized; } diff --git a/web/js/focused-node.js b/web/js/focused-node.js index 55727284..f4aae5a1 100644 --- a/web/js/focused-node.js +++ b/web/js/focused-node.js @@ -2,9 +2,13 @@ import {createScope, createButton, createField, mountTheme} from './shared/toolk import {createPromptLibrary} from './prompt-library.js'; import {attachMetadataAutocomplete} from './metadata-autocomplete.js'; import {exposeTextSockets, linkNoteText, preferTextInputs, textInputAtPoint} from './text-links.js'; +import {SAVE_WIDGET, normalizeSaveWidget, placeSaveWidgetLast, saveWidget, savesToLibrary, setSaveToLibrary} from './save-to-library.js'; const names = ['text', 'category', 'tags', 'search_text', 'prepend_text', 'append_text']; let nextEditorId = 0; +// The editor draws these widgets itself; the host keeps them as hidden, zero-height values. +function hiddenWidgetSize() { return [0, -4]; } +function hideNativeWidget(widget) { widget.hidden = true; widget.type = 'hidden'; widget.computeSize = hiddenWidgetSize; } function loadStyles() { const href = new URL('./prompt-editor.css', import.meta.url).href; @@ -43,10 +47,15 @@ export function installFocusedNode(nodeType, app, api, parseQuery, {layout = 'fo for (const name of pair ? [...names,'negative_text'] : names) { let widget = this.widgets?.find(item => item.name === name); if (!widget) widget = this.addWidget('text', name, '', () => {}, {}); - widget.hidden = true; widget.type = 'hidden'; widget.computeSize = () => [0, -4]; + hideNativeWidget(widget); widgets.set(name, widget); } exposeTextSockets(this, textInputs); + // #90: the switch follows the hidden fallbacks above (one widget order on every frontend) and is drawn in the + // toolbar, like the other hidden widgets are drawn by the editor. + placeSaveWidgetLast(this); + const nativeSave = saveWidget(this); + if (nativeSave) hideNativeWidget(nativeSave); const element = document.createElement('div'); element.className = `ca-tools pm-focused${split ? ' pm-split' : ''}${pair ? ' pm-pair' : ''}`; element.setAttribute('aria-label',pair ? 'Paired prompt editor' : split ? 'Split prompt editor' : 'Focused prompt editor'); loadStyles(); mountTheme(element, app.ui?.settings, scope); @@ -58,6 +67,11 @@ export function installFocusedNode(nodeType, app, api, parseQuery, {layout = 'fo toolbar.append(web); const save = pair ? createButton('Save pair','ca-primary') : null; if (save) toolbar.append(save); + const saveSwitch = document.createElement('input'); saveSwitch.type = 'checkbox'; saveSwitch.dataset.saveToLibrary = ''; + const saveLabel = document.createElement('label'); saveLabel.className = 'pm-editor-save'; + saveLabel.title = 'Off: runs of this node are not saved, counted or linked to their images. The output is unchanged.'; + saveLabel.append(saveSwitch, document.createTextNode('Save to library')); + if (nativeSave) toolbar.append(saveLabel); let saving = false; const workspace = document.createElement('div'); workspace.className = 'pm-editor-workspace'; const editor = document.createElement('div'); editor.className = 'pm-editor-compose'; @@ -138,7 +152,11 @@ export function installFocusedNode(nodeType, app, api, parseQuery, {layout = 'fo } const composed = disclosure('preview','Composed text preview'); const preview = document.createElement('p'); preview.className = 'pm-editor-preview'; preview.dataset.composedPreview = ''; composed.append(preview); - const footer = document.createElement('p'); footer.className = 'pm-editor-status'; footer.setAttribute('role','status'); footer.textContent = pair ? 'The prompt pair is saved when this node runs, or use Save pair.' : 'Main prompt is saved when this node runs.'; + const footer = document.createElement('p'); footer.className = 'pm-editor-status'; footer.setAttribute('role','status'); + const SAVE_OFF = 'Save to library is off: runs of this node are not added to the library.'; + const SAVE_LINKED = 'Save to library comes from a link: runs are added to the library only while it is on.'; + function runNote(saved) { return isConnected(SAVE_WIDGET) ? SAVE_LINKED : savesToLibrary(thisNode) ? saved : SAVE_OFF; } + footer.textContent = runNote(pair ? 'The prompt pair is saved when this node runs, or use Save pair.' : 'Main prompt is saved when this node runs.'); const saveConnectionNotice = pair ? document.createElement('p') : null; if (saveConnectionNotice) { saveConnectionNotice.className = 'pm-editor-status'; saveConnectionNotice.setAttribute('role','status'); @@ -166,6 +184,7 @@ export function installFocusedNode(nodeType, app, api, parseQuery, {layout = 'fo mainField.hidden = Boolean(isConnected('text')); connected.hidden = !isConnected('text'); if (negative) { negative.field.hidden = Boolean(isConnected('negative_text')); negative.note.hidden = !isConnected('negative_text'); } if (save) { save.disabled = saving || hasConnectedPairValues(); saveConnectionNotice.hidden = !hasConnectedPairValues(); } + saveSwitch.checked = savesToLibrary(thisNode); saveSwitch.disabled = isConnected(SAVE_WIDGET); controls.forEach((input,name) => { input.value = value(name); if (name !== 'text' && name !== 'negative_text') { @@ -185,7 +204,7 @@ export function installFocusedNode(nodeType, app, api, parseQuery, {layout = 'fo write('category',prompt.category || ''); write('tags',Array.isArray(prompt.tags) ? prompt.tags.join(', ') : prompt.tags || ''); } - render(); footer.setAttribute('role','status'); footer.textContent = pair ? 'Loaded pair. Saved when this node runs.' : 'Loaded prompt. Saved when this node runs.'; main.focus(); + render(); footer.setAttribute('role','status'); footer.textContent = runNote(pair ? 'Loaded pair. Saved when this node runs.' : 'Loaded prompt. Saved when this node runs.'); main.focus(); }}); workspace.append(library.element); element.append(footer); function snapshot() { @@ -197,7 +216,13 @@ export function installFocusedNode(nodeType, app, api, parseQuery, {layout = 'fo library.element.hidden = !split && !state.libraryOpen; browse.textContent = state.libraryOpen ? 'Hide library' : 'Browse library'; browse.setAttribute('aria-expanded', String(Boolean(state.libraryOpen))); library.setQuery(value('search_text')); library.restoreFilters(state.libraryFilters); render(); } - controls.forEach((input,name) => scope.listen(input,'input',() => {write(name,input.value);render();footer.setAttribute('role','status');footer.textContent=pair?'The pair is saved when this node runs.':'Main prompt is saved when this node runs.';})); + controls.forEach((input,name) => scope.listen(input,'input',() => {write(name,input.value);render();footer.setAttribute('role','status');footer.textContent=runNote(pair?'The pair is saved when this node runs.':'Main prompt is saved when this node runs.');})); + scope.listen(saveSwitch,'change',() => { + if (isConnected(SAVE_WIDGET)) return; + setSaveToLibrary(thisNode, saveSwitch.checked); render(); + footer.setAttribute('role','status'); footer.textContent = runNote(pair ? 'The pair is saved when this node runs.' : 'Main prompt is saved when this node runs.'); + thisNode.setDirtyCanvas(true,true); app.graph?.setDirtyCanvas(true,true); + }); scope.listen(browse,'click',() => { library.element.hidden = !library.element.hidden; library.closeCompletions(); @@ -248,6 +273,8 @@ export function installFocusedNode(nodeType, app, api, parseQuery, {layout = 'fo const result = configure?.apply(this,args); // A saved workflow restores its inputs with their widget bindings; keep the text inputs as sockets. exposeTextSockets(this, textInputs); + // #90: an older workflow loads with Save to library on + normalizeSaveWidget(this); this._focusedView?.restore(); return result; }; diff --git a/web/js/prompt-editor.css b/web/js/prompt-editor.css index ba6fbf4a..27c78330 100644 --- a/web/js/prompt-editor.css +++ b/web/js/prompt-editor.css @@ -2,6 +2,8 @@ .pm-focused [hidden] { display: none !important; } .pm-editor-row { display: flex; align-items: center; gap: 8px; flex-wrap: wrap; } .pm-editor-toolbar > strong { margin-inline-end: auto; font-weight: 500; } +.pm-editor-save { display: inline-flex; align-items: center; gap: 6px; color: var(--ca-muted); cursor: pointer; } +.pm-editor-save:has(input:checked) { color: inherit; } .pm-focused .ca-field-label { align-self: flex-start; margin-inline: 0; text-align: start; } .pm-editor-workspace, .pm-editor-compose { display: contents; } .pm-focused .pm-editor-main { display: flex; flex: 1; min-block-size: 180px; flex-direction: column; align-items: stretch; gap: 5px; text-align: start; } diff --git a/web/js/save-to-library.js b/web/js/save-to-library.js new file mode 100644 index 00000000..fdd39fe3 --- /dev/null +++ b/web/js/save-to-library.js @@ -0,0 +1,47 @@ +// #90: a PromptManager node's `save_to_library` switch. Off: PromptManager ignores the node's runs (nothing saved, +// counted or linked); the node's output is unchanged. +// +// The input is declared last, so it takes the widget index an older workflow used for the editor's own +// (non-serialized) DOM widget, where it stored `''` or nothing. ComfyUI coerces a BOOLEAN with `bool(value)`, so that +// `''` would turn saving off: after a workflow loads, anything that is not a boolean is the default, on. + +export const SAVE_WIDGET = 'save_to_library'; + +export function saveWidget(node) { + return node?.widgets?.find(widget => widget?.name === SAVE_WIDGET); +} + +/** Whether the node saves its runs (a missing widget, as on an older backend, is on). */ +export function savesToLibrary(node) { + return saveWidget(node)?.value !== false; +} + +/** Set a non-boolean switch value (from an older workflow) to on. Returns whether it changed. */ +export function normalizeSaveWidget(node) { + const widget = saveWidget(node); + if (!widget || typeof widget.value === 'boolean') return false; + widget.value = true; + const index = node.widgets.indexOf(widget); + if (Array.isArray(node.widgets_values) && index >= 0) node.widgets_values[index] = true; + return true; +} + +/** Move the switch after every other widget the node has now, so its index is the same on every frontend. */ +export function placeSaveWidgetLast(node) { + const widget = saveWidget(node); + const widgets = node?.widgets; + if (!widget || widgets.at(-1) === widget) return false; + widgets.splice(widgets.indexOf(widget), 1); + widgets.push(widget); + return true; +} + +/** Turn the switch on or off the way a native widget edit does. */ +export function setSaveToLibrary(node, value) { + const widget = saveWidget(node); + if (!widget) return; + widget.value = Boolean(value); + const index = node.widgets.indexOf(widget); + if (Array.isArray(node.widgets_values) && index >= 0) node.widgets_values[index] = widget.value; + widget.callback?.(widget.value, undefined, node); +} From 3ebd5e41665b9f712be48112838b19aa90b56e60 Mon Sep 17 00:00:00 2001 From: Vito Date: Wed, 28 Oct 2026 18:38:33 +0000 Subject: [PATCH 2/3] ci(browser): a hung browser step fails with its running tests named and its traces kept (#425) (#429) * ci(browser): stop a hung browser run before the job limit and name it A browser job killed at its 20-minute limit keeps no log and skips its upload steps (merge-queue run 40723857746, firefox shard 2/2), so nothing showed which test hung. - Each browser test step gets timeout-minutes 18, below the job's 20, so the step fails and the always() uploads still run. - Under CI, Playwright's globalTimeout stops the run first: PM_TEST_GLOBAL_TIMEOUT_MINUTES 17 for page workflows, 15 for coverage workflows, a fixed 3 for the viewer suite. Locally there is no limit unless the variable is set. Each test keeps the 30 s default. - running-tests-reporter.mjs prints the tests still running when the run ends; Playwright's own reporters cannot, since those tests never end. - A blob report goes to tools/vendor/test-results/blob/, inside the uploaded traces path. - ci-workflow.test.mjs pins globalTimeout < step limit < job limit and the always() uploads; no existing limit is raised. Closes #425 * ci(browser): rely on the step limit, report the running tests on SIGTERM Maintainer ruling on #425: no Playwright globalTimeout. The 18-minute step limit under the unchanged 20-minute job limit stays, as do the always() uploads and the CI blob report. - Drop globalTimeout and PM_TEST_GLOBAL_TIMEOUT_MINUTES with its helper and tests. - A step that reaches its limit gets SIGTERM, which ended Playwright with no report and no blob. The running-tests reporter now catches the first SIGTERM, prints the tests still running and sends Playwright its own SIGINT, so the run ends interrupted and the blob report is written. A second SIGTERM kills as usual. - Test paths are relative to the suite's rootDir instead of the cwd, so `npm run --prefix` no longer prints ../../ prefixes. - The workflow comment and TESTING.md say the guarantee assumes an npm cache hit and that coverage shard 1 is tight in its 18-minute step. * ci(browser): forward SIGTERM and SIGINT from frontend-coverage.mjs to its child In the coverage job npm signals only frontend-coverage.mjs. It blocked in spawnSync with no listener, so Playwright never saw a step timeout. It now runs each step's child with async spawn, in its own process group, and forwards SIGTERM and SIGINT to the running child. Step order, exit codes and the "Command failed (N)" message are unchanged. Locally, in the pinned image, `timeout -s TERM` on test:coverage printed the still-running tests and wrote both blob reports. TESTING.md records that a SIGTERM to npm alone stops at npm's `sh -c`, and that delivery through `docker exec` in CI is not yet observed. Refs #425 * ci(browser): load the running-tests reporter by absolute path A config imported from another directory (the workflow-artifacts test copies it to a temp root) resolved './running-tests-reporter.mjs' against that root under CI and failed to load it. Resolve it from this module instead. --- .github/workflows/viewerjs-check.yml | 13 +++++ TESTING.md | 25 ++++++++ tests/vendor/browser-server-fixture.test.mjs | 60 +++++++++++++++++++- tests/vendor/ci-workflow.test.mjs | 20 +++++++ tests/vendor/frontend-coverage-cli.test.mjs | 49 +++++++++++++++- tools/vendor/frontend-coverage.mjs | 35 +++++++++--- tools/vendor/playwright.config.mjs | 3 +- tools/vendor/playwright.workflows.config.mjs | 4 +- tools/vendor/running-tests-reporter.mjs | 58 +++++++++++++++++++ tools/vendor/test-workers.mjs | 11 ++++ 10 files changed, 262 insertions(+), 16 deletions(-) create mode 100644 tools/vendor/running-tests-reporter.mjs diff --git a/.github/workflows/viewerjs-check.yml b/.github/workflows/viewerjs-check.yml index 783e1b92..4a87231d 100644 --- a/.github/workflows/viewerjs-check.yml +++ b/.github/workflows/viewerjs-check.yml @@ -27,6 +27,15 @@ concurrency: # coverage shards 7m34s/6m04s/5m22s (tests 6m38s/4m53s/4m04s), merge 54s, firefox 7m57s/5m21s (tests # 7m08s/4m34s), webkit 8m16s/5m03s (tests 7m26s/3m58s); a cache miss adds up to 5m to npm ci, which # the 20-minute limits absorb. +# Hangs (#425): a job killed at its limit keeps no log and skips its upload steps, so each browser +# test step has its own timeout-minutes (18) below the job's 20. At the step limit, SIGTERM makes +# tools/vendor/running-tests-reporter.mjs print the tests still running and hand over to Playwright's +# interrupt, which writes the blob report; the always() uploads then run. +# Measured on main (runs 40725289094, 40719375442, 40700439399): setup before the test step <= 52s, +# uploads <= 35s; test steps up to 14m42s (webkit shard 1/2) and 13m05s for coverage (shard 1/3: +# 12m53s, node units and the viewer suite before 11.8m of workflows), so coverage is the tight one +# in its 18 minutes. This holds only when the npm cache hits: a cold cache adds up to 5m to npm ci, +# and then the job limit can still end the job before the step limit does. jobs: frontend-coverage: name: frontend coverage (shard ${{ matrix.shard }}/3) @@ -79,6 +88,7 @@ jobs: - if: matrix.shard == 3 run: npm run --prefix tools/vendor test - name: Page workflows with coverage, shard ${{ matrix.shard }}/3 + timeout-minutes: 18 run: npm run --prefix tools/vendor test:coverage:shard -- ${{ matrix.shard }}/3 - name: Upload raw coverage uses: actions/upload-artifact@v7 @@ -87,6 +97,7 @@ jobs: path: coverage/js/raw/ if-no-files-found: error retention-days: 3 + # Traces and the blob report (test-results/blob/) of a failed, timed-out or cancelled step. - name: Upload Chromium and touch traces if: always() uses: actions/upload-artifact@v7 @@ -211,7 +222,9 @@ jobs: key: npm-${{ runner.os }}-playwright-v1.64.1-${{ hashFiles('tools/vendor/package-lock.json') }} - run: npm ci --prefix tools/vendor --ignore-scripts - name: Page workflows in ${{ matrix.browser }}, shard ${{ matrix.shard }}/2 + timeout-minutes: 18 run: npm run --prefix tools/vendor test:workflows -- --project ${{ matrix.browser }} --shard ${{ matrix.shard }}/2 + # Traces and the blob report (test-results/blob/) of a failed, timed-out or cancelled step. - name: Upload browser traces if: always() uses: actions/upload-artifact@v7 diff --git a/TESTING.md b/TESTING.md index 43118eed..7e00c70f 100644 --- a/TESTING.md +++ b/TESTING.md @@ -262,6 +262,31 @@ watchdog dump, is a failure like any other: read the dump, find the blocking call, fix it. Never raise a limit or rerun to get green; a limit changes only with new measurements written next to it. +Browser jobs (#425): GitHub keeps no log of a job killed at its limit and skips +its upload steps, so the test step in each browser job in `viewerjs-check.yml` +has its own `timeout-minutes` (18, below the job's 20). At the step limit +GitHub sends SIGTERM, which would end Playwright without any report; under `CI` +`tools/vendor/running-tests-reporter.mjs` catches the first one, prints +`Still running when the run ended (SIGTERM): N` and one line per unfinished +test (path relative to the suite's `rootDir`), then sends Playwright its own +SIGINT, so the workers stop, the run ends interrupted and the blob report is +written to `tools/vendor/test-results/blob/`. In the coverage job npm signals +only `frontend-coverage.mjs`, which forwards SIGTERM and SIGINT to the child it +is running (started in its own process group, so a group-wide signal still +arrives once). Delivery is proven locally in the pinned image for a SIGTERM to +the whole process group (`timeout -s TERM`). A SIGTERM to npm alone +(`timeout --foreground`) reaches nothing: npm forwards it to the `sh -c` that +runs the script, and that shell exits without passing it on. Delivery in CI, +where the step runs through `docker exec`, is not yet observed; the next real +step timeout will show it. That directory and the traces +upload with `if: always()`; `npx playwright merge-reports` opens the blob. +Each test keeps the 30 s default and there is no `globalTimeout`. Two limits +of this: it assumes the npm cache hits (a cold cache adds up to 5 minutes to +`npm ci`, and the job limit can then still win), and coverage shard 1 (12m53s +measured) is the tight one in its 18-minute step. Local runs keep the `list` +reporter only; reproduce with `CI=1 timeout -s TERM 40 npm run --prefix +tools/vendor test:workflows -- --project=firefox`. + ### After a push Follow the run for the commit you pushed, never an older one: diff --git a/tests/vendor/browser-server-fixture.test.mjs b/tests/vendor/browser-server-fixture.test.mjs index e0e20bed..91ec59fd 100644 --- a/tests/vendor/browser-server-fixture.test.mjs +++ b/tests/vendor/browser-server-fixture.test.mjs @@ -2,12 +2,14 @@ import test from 'node:test'; import assert from 'node:assert/strict'; import { createServer } from 'node:net'; import { spawn } from 'node:child_process'; -import { once } from 'node:events'; +import { EventEmitter, once } from 'node:events'; import { readFileSync } from 'node:fs'; +import { isAbsolute } from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; import { setTimeout as delay } from 'node:timers/promises'; import { startBrowserServer, createServerSupervisor, attachServerStderr, forwardPrefixedLines, SLOW_RESET_PREFIX } from '../../tools/vendor/start-browser-server.mjs'; -import { workflowWorkers } from '../../tools/vendor/test-workers.mjs'; +import { reporters, workflowWorkers } from '../../tools/vendor/test-workers.mjs'; +import RunningTestsReporter from '../../tools/vendor/running-tests-reporter.mjs'; const fakeServer = fileURLToPath(new URL('./fake-browser-server.mjs', import.meta.url)); const fake = (...extra) => ({ command: process.execPath, args: port => [fakeServer, '--port', String(port), ...extra] }); @@ -255,3 +257,57 @@ test('worker count follows PM_TEST_WORKERS, then one in CI, then a quarter of th assert.throws(() => workflowWorkers({ env: { PM_TEST_WORKERS: 'many' }, cpus: 8 }), /PM_TEST_WORKERS must be a positive integer/); assert.throws(() => workflowWorkers({ env: { PM_TEST_WORKERS: '0' }, cpus: 8 }), /PM_TEST_WORKERS must be a positive integer/); }); + +test('CI adds the running-tests reporter and a blob report under test-results/blob/, which the trace upload covers; local runs keep list', () => { + assert.equal(reporters({ env: {}, name: 'workflows' }), 'list'); + const [list, running, blob] = reporters({ env: { CI: '1' }, name: 'workflows' }); + assert.deepEqual(list, ['list']); + // Absolute: a config imported from another directory (workflow-artifacts.test.mjs) still finds it. + assert.ok(isAbsolute(running[0]) && running[0].endsWith('running-tests-reporter.mjs'), running[0]); + assert.deepEqual(blob, ['blob', { outputDir: './test-results/blob/workflows' }]); +}); + +function fakeHost() { + const host = new EventEmitter(); + Object.assign(host, { pid: 4242, signals: [], cwd: () => '/repo/tools/vendor' }); + host.kill = (pid, signal) => host.signals.push([pid, signal]); + return host; +} + +const spec = (id, title, line) => ({ id, title, location: { file: '/repo/tests/browser/a.spec.mjs', line }, + parent: { project: () => ({ name: 'firefox' }) } }); + +test('the running-tests reporter names only the tests that never ended, relative to rootDir', () => { + let clock = 1_000; + const output = []; + const host = fakeHost(); + const reporter = new RunningTestsReporter({ now: () => clock, write: text => output.push(text), host }); + reporter.onBegin({ rootDir: '/repo/tests/browser' }); + const done = spec('1', 'finishes', 10); + const stuck = spec('2', 'hangs on a fetch', 20); + reporter.onTestBegin(done, { retry: 0 }); + reporter.onTestBegin(stuck, { retry: 0 }); + clock += 500; + reporter.onTestEnd(done, { retry: 0 }); + clock += 41_500; + reporter.onEnd({ status: 'interrupted' }); + assert.equal(output.join(''), '\nStill running when the run ended (interrupted): 1\n' + + ' [firefox] › a.spec.mjs:20 › hangs on a fetch (running 42.0s)\n'); + assert.equal(host.listenerCount('SIGTERM'), 0, 'the run is over: SIGTERM gets its default again'); + output.length = 0; + reporter.onTestEnd(stuck, { retry: 0 }); + reporter.onEnd({ status: 'passed' }); + assert.deepEqual(output, [], 'a run where every test ended prints nothing'); +}); + +test('on the first SIGTERM the reporter prints the running tests and hands over to Playwright\'s SIGINT', () => { + const output = []; + const host = fakeHost(); + const reporter = new RunningTestsReporter({ now: () => 0, write: text => output.push(text), host }); + reporter.onBegin({ rootDir: '/repo/tests/browser' }); + reporter.onTestBegin(spec('2', 'hangs on a fetch', 20), { retry: 0 }); + host.emit('SIGTERM'); + assert.match(output.join(''), /Still running when the run ended \(SIGTERM\): 1\n {2}\[firefox\] › a\.spec\.mjs:20 › hangs on a fetch/); + assert.deepEqual(host.signals, [[4242, 'SIGINT']], 'Playwright\'s own interrupt ends the run and writes the blob'); + assert.equal(host.listenerCount('SIGTERM'), 0, 'a second SIGTERM kills the process as usual'); +}); diff --git a/tests/vendor/ci-workflow.test.mjs b/tests/vendor/ci-workflow.test.mjs index d8ed881a..0e3d0c62 100644 --- a/tests/vendor/ci-workflow.test.mjs +++ b/tests/vendor/ci-workflow.test.mjs @@ -247,6 +247,26 @@ test('browser job timeouts keep a real margin over measured runs', () => { } }); +// A hang (#425) must end inside the test step, before the job limit, so the log and the always() +// uploads survive: the step's timeout-minutes < the job's. +const BROWSER_TEST_STEP = /npm run --prefix tools\/vendor test:(workflows|coverage)/; + +test('each browser test step stops itself before its job limit and its traces upload anyway', () => { + for (const id of browserJobIds) { + const body = allJobs.get(id); + const jobMinutes = Number(/^ {4}timeout-minutes: (\d+)$/m.exec(body)[1]); + const jobSteps = steps(body); + const testAt = jobSteps.findIndex(step => BROWSER_TEST_STEP.test(step)); + const testStep = jobSteps[testAt]; + const stepMinutes = Number(/^ {8}timeout-minutes: (\d+)$/m.exec(testStep)?.[1]); + assert.ok(stepMinutes > 0, `${id}: the test step sets its own timeout-minutes`); + assert.ok(jobMinutes - stepMinutes >= 2, `${id}: the step limit leaves time for setup and the uploads`); + const upload = jobSteps.slice(testAt + 1).find(step => /^ {10}path: tools\/vendor\/test-results\/$/m.test(step)); + assert.ok(upload, `${id} uploads tools/vendor/test-results/ (traces and blob report) after the tests`); + assert.match(upload, /^ {8}if: always\(\)$/m, `${id} uploads them when the step failed or timed out`); + } +}); + // Every job is bounded (#350): a hang ends at a measured limit instead of GitHub's 6-hour default. const GATING_WORKFLOWS = ['test.yml', 'code-quality.yml', 'viewerjs-check.yml']; const BOUNDED_WORKFLOWS = [...GATING_WORKFLOWS, 'portability.yml']; diff --git a/tests/vendor/frontend-coverage-cli.test.mjs b/tests/vendor/frontend-coverage-cli.test.mjs index 8243181b..d926f72c 100644 --- a/tests/vendor/frontend-coverage-cli.test.mjs +++ b/tests/vendor/frontend-coverage-cli.test.mjs @@ -1,9 +1,11 @@ import test from 'node:test'; import assert from 'node:assert/strict'; -import { existsSync, mkdtempSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; +import { existsSync, mkdtempSync, mkdirSync, readFileSync, rmSync, writeFileSync } from 'node:fs'; +import { EventEmitter } from 'node:events'; +import { setTimeout as delay } from 'node:timers/promises'; import { tmpdir } from 'node:os'; import { join } from 'node:path'; -import { plan, runCoverage } from '../../tools/vendor/frontend-coverage.mjs'; +import { plan, runCoverage, runNode } from '../../tools/vendor/frontend-coverage.mjs'; const steps = argv => plan(argv).map(({ step, ...options }) => (Object.keys(options).length ? { step, ...options } : step)); @@ -91,3 +93,46 @@ test('shard 1 runs the node units into the shared raw dir, then the viewer suite assert.ok(calls[1].args.includes(join(paths.tools, 'playwright.config.mjs'))); assert.deepEqual(calls[2].args.slice(-2), ['--shard', '1/3']); }); + +// #425: npm and a CI step timeout signal only frontend-coverage.mjs; its running child must get it. +function hostWithSignals() { + return new EventEmitter(); +} + +test('runNode resolves on exit 0, rejects with the exit code otherwise and leaves no signal listener', async () => { + const host = hostWithSignals(); + await runNode(['-e', 'process.exit(0)'], {}, { host }); + await assert.rejects(runNode(['-e', 'process.exit(3)'], {}, { host }), /Command failed \(3\): -e process\.exit\(3\)/); + assert.equal(host.listenerCount('SIGTERM') + host.listenerCount('SIGINT'), 0); +}); + +test('runNode forwards SIGTERM to the running child, which can report before it exits', async t => { + const root = mkdtempSync(join(tmpdir(), 'pm-coverage-signal-')); + t.after(() => rmSync(root, { recursive: true, force: true })); + const ready = join(root, 'ready'); + const seen = join(root, 'seen'); + const child = `const fs = require('node:fs'); +process.on('SIGTERM', () => { fs.writeFileSync(${JSON.stringify(seen)}, 'SIGTERM'); process.exit(7); }); +fs.writeFileSync(${JSON.stringify(ready)}, ''); setInterval(() => {}, 1000);`; + const host = hostWithSignals(); + const running = runNode(['-e', child], {}, { host }); + while (!existsSync(ready)) await delay(20); + host.emit('SIGTERM'); + await assert.rejects(running, /Command failed \(7\)/); + assert.equal(readFileSync(seen, 'utf8'), 'SIGTERM', 'the child handled the forwarded signal'); + assert.equal(host.listenerCount('SIGTERM'), 0); +}); + +test('runNode forwards SIGINT and starts the child in its own process group', async () => { + const host = hostWithSignals(); + const kills = []; + let options; + const fake = new EventEmitter(); + fake.kill = signal => { kills.push(signal); queueMicrotask(() => fake.emit('exit', null, signal)); }; + const running = runNode(['x.mjs'], {}, { host, spawnChild: (command, args, spawnOptions) => { options = spawnOptions; return fake; } }); + assert.equal(options.detached, true, 'a group-wide signal reaches the child only through forwarding, once'); + host.emit('SIGINT'); + await assert.rejects(running, /Command failed \(null\): x\.mjs/); + assert.deepEqual(kills, ['SIGINT']); + assert.equal(host.listenerCount('SIGINT'), 0); +}); diff --git a/tools/vendor/frontend-coverage.mjs b/tools/vendor/frontend-coverage.mjs index 30de4152..3ade3bb5 100644 --- a/tools/vendor/frontend-coverage.mjs +++ b/tools/vendor/frontend-coverage.mjs @@ -1,7 +1,7 @@ import { existsSync, readFileSync, readdirSync, mkdirSync, rmSync, writeFileSync } from 'node:fs'; import { dirname, join, relative } from 'node:path'; import { fileURLToPath, pathToFileURL } from 'node:url'; -import { spawnSync } from 'node:child_process'; +import { spawn } from 'node:child_process'; import { repositoryRoot } from './vendor-viewerjs.mjs'; import { discoverSources, checkCoverage } from '@comfyassets/toolkit/testing/coverage'; @@ -10,10 +10,27 @@ const metrics = ['lines', 'branches', 'functions', 'statements']; const SHARD = /^([1-9]\d*)\/([1-9]\d*)$/; const USAGE = 'usage: frontend-coverage.mjs [--update-baseline] | --shard I/N | --merge-only [--update-baseline]'; -function runNode(args, env = {}) { - const result = spawnSync(process.execPath, args, { cwd: repositoryRoot, stdio: 'inherit', - env: { ...process.env, ...env } }); - if (result.status !== 0) throw new Error(`Command failed (${result.status}): ${args.join(' ')}`); +const FORWARDED_SIGNALS = ['SIGTERM', 'SIGINT']; + +// Runs one step's node child and waits for it. npm and a CI step timeout signal only this process, +// so SIGTERM and SIGINT are forwarded to the running child (#425): Playwright then reports the tests +// still running and writes its blob. The child gets its own process group, so a signal sent to the +// whole group (Ctrl-C, `timeout`) still reaches it exactly once, through this forwarding. +export function runNode(args, env = {}, { spawnChild = spawn, host = process } = {}) { + return new Promise((resolve, reject) => { + const child = spawnChild(process.execPath, args, { cwd: repositoryRoot, stdio: 'inherit', + env: { ...process.env, ...env }, detached: true }); + const forward = signal => () => child.kill(signal); + const handlers = FORWARDED_SIGNALS.map(signal => [signal, forward(signal)]); + for (const [signal, handler] of handlers) host.on(signal, handler); + const done = () => { for (const [signal, handler] of handlers) host.removeListener(signal, handler); }; + child.once('error', error => { done(); reject(error); }); + child.once('exit', code => { + done(); + if (code === 0) resolve(); + else reject(new Error(`Command failed (${code}): ${args.join(' ')}`)); + }); + }); } function defaultPaths() { @@ -112,17 +129,17 @@ export async function runCoverage({ argv = process.argv.slice(2), run = runNode, 'node-units'() { const tests = readdirSync(join(root, 'tests/js')).filter(file => file.endsWith('.test.js')) .map(file => `tests/js/${file}`); - run(['--test', ...tests], { NODE_V8_COVERAGE: raw }); + return run(['--test', ...tests], { NODE_V8_COVERAGE: raw }); }, viewer() { - run([playwright, 'test', '--config', join(tools, 'playwright.config.mjs')]); + return run([playwright, 'test', '--config', join(tools, 'playwright.config.mjs')]); }, workflows({ shard }) { - run([playwright, 'test', '--config', join(tools, 'playwright.workflows.config.mjs'), + return run([playwright, 'test', '--config', join(tools, 'playwright.workflows.config.mjs'), '--project', 'chromium', '--project', 'touch', ...(shard ? ['--shard', shard] : [])], { PM_JS_COVERAGE: raw }); }, report() { - run([join(tools, 'node_modules/c8/bin/c8.js'), 'report', '--all', '--src', sourceRoot, + return run([join(tools, 'node_modules/c8/bin/c8.js'), 'report', '--all', '--src', sourceRoot, '--include', 'web/js/**/*.js', '--include', 'web/js/**/*.mjs', '--temp-directory', raw, '--reports-dir', reportRoot, '--reporter', 'text', '--reporter', 'json-summary', '--reporter', 'html']); }, diff --git a/tools/vendor/playwright.config.mjs b/tools/vendor/playwright.config.mjs index 5d291a79..81bc799d 100644 --- a/tools/vendor/playwright.config.mjs +++ b/tools/vendor/playwright.config.mjs @@ -1,4 +1,5 @@ import { defineConfig } from '@playwright/test'; +import { reporters } from './test-workers.mjs'; export default defineConfig({ testDir: './tests', @@ -8,7 +9,7 @@ export default defineConfig({ workers: 2, forbidOnly: Boolean(process.env.CI), retries: process.env.CI ? 1 : 0, - reporter: 'list', + reporter: reporters({ name: 'viewer' }), use: { browserName: 'chromium', headless: true, trace: 'retain-on-failure' }, projects: [ { name: 'desktop', use: { viewport: { width: 1280, height: 800 } } }, diff --git a/tools/vendor/playwright.workflows.config.mjs b/tools/vendor/playwright.workflows.config.mjs index 67e7c21b..d076ba92 100644 --- a/tools/vendor/playwright.workflows.config.mjs +++ b/tools/vendor/playwright.workflows.config.mjs @@ -1,5 +1,5 @@ import { defineConfig, devices } from '@playwright/test'; -import { workflowWorkers } from './test-workers.mjs'; +import { reporters, workflowWorkers } from './test-workers.mjs'; // No global webServer: tests/browser/fixtures.mjs starts one server per worker // on an OS-chosen port. Files run in parallel; tests inside a file keep their order. @@ -11,7 +11,7 @@ export default defineConfig({ fullyParallel: false, forbidOnly: Boolean(process.env.CI), retries: 0, - reporter: 'list', + reporter: reporters({ name: 'workflows' }), use: { trace: 'retain-on-failure', screenshot: 'only-on-failure' }, projects: [ { name: 'chromium', outputDir: './test-results/workflows/chromium', use: { browserName: 'chromium' } }, diff --git a/tools/vendor/running-tests-reporter.mjs b/tools/vendor/running-tests-reporter.mjs new file mode 100644 index 00000000..4a918955 --- /dev/null +++ b/tools/vendor/running-tests-reporter.mjs @@ -0,0 +1,58 @@ +import { writeSync } from 'node:fs'; +import { relative } from 'node:path'; + +// Names the tests still running when a run is cut off (#425). A browser test step that reaches its +// timeout-minutes gets SIGTERM, which ends Playwright at once: no reporter's onEnd runs, so neither +// `list` nor the blob report says what was running. On the first SIGTERM this reporter prints every +// started test that has not ended, then turns the signal into SIGINT, Playwright's own interrupt, +// which stops the workers, ends the run as interrupted (non-zero exit) and writes the blob report. +// A second SIGTERM gets the default behaviour: the process dies. +export default class RunningTestsReporter { + constructor({ now = () => Date.now(), write = text => writeSync(1, text), host = process } = {}) { + this.now = now; + this.write = write; + this.host = host; + this.rootDir = host.cwd(); + this.running = new Map(); + this.onSigterm = () => { + this.host.removeListener('SIGTERM', this.onSigterm); + this.report('SIGTERM'); + this.host.kill(this.host.pid, 'SIGINT'); + }; + } + + printsToStdio() { + return false; + } + + onBegin(config) { + this.rootDir = config.rootDir; + this.host.on('SIGTERM', this.onSigterm); + } + + onTestBegin(test, result) { + this.running.set(`${test.id}#${result.retry}`, { test, startedAt: this.now() }); + } + + onTestEnd(test, result) { + this.running.delete(`${test.id}#${result.retry}`); + } + + describe({ test, startedAt }) { + const project = test.parent?.project()?.name; + const where = `${relative(this.rootDir, test.location.file)}:${test.location.line}`; + const seconds = ((this.now() - startedAt) / 1000).toFixed(1); + return `${project ? `[${project}] › ` : ''}${where} › ${test.title} (running ${seconds}s)`; + } + + report(reason) { + if (!this.running.size) return; + const lines = [...this.running.values()].map(entry => ` ${this.describe(entry)}`); + this.write(`\nStill running when the run ended (${reason}): ${this.running.size}\n${lines.join('\n')}\n`); + } + + onEnd(result) { + this.host.removeListener('SIGTERM', this.onSigterm); + this.report(result.status); + } +} diff --git a/tools/vendor/test-workers.mjs b/tools/vendor/test-workers.mjs index 92dff984..03adf724 100644 --- a/tools/vendor/test-workers.mjs +++ b/tools/vendor/test-workers.mjs @@ -1,4 +1,8 @@ import { availableParallelism } from 'node:os'; +import { fileURLToPath } from 'node:url'; + +// Absolute, so a config that imports this one from another directory still finds it. +const RUNNING_TESTS_REPORTER = fileURLToPath(new URL('./running-tests-reporter.mjs', import.meta.url)); const MAX_LOCAL_WORKERS = 8; const CPUS_PER_WORKER = 4; @@ -16,3 +20,10 @@ export function workflowWorkers({ env = process.env, cpus = availableParallelism if (env.CI) return 1; return Math.max(1, Math.min(MAX_LOCAL_WORKERS, Math.floor(cpus / CPUS_PER_WORKER))); } + +// Under CI, running-tests-reporter.mjs names the tests a step timeout's signal cut off (`list` +// cannot: they never end), and a blob report beside the traces is uploaded with them. +export function reporters({ env = process.env, name } = {}) { + if (!env.CI) return 'list'; + return [['list'], [RUNNING_TESTS_REPORTER], ['blob', { outputDir: `./test-results/blob/${name}` }]]; +} From 4f37af209c7cc6c86525fcf52704fe1d2c97ac36 Mon Sep 17 00:00:00 2001 From: Vito Date: Wed, 28 Oct 2026 18:49:06 +0000 Subject: [PATCH 3/3] feat(collections): store collections and add their routes and Images filter (#331, PR 1) (#400) * feat(collections): store collections in the library database * feat(collections): add, remove and rename collections and keep members through duplicate cleanup * feat(collections): add collection routes and the Images collection filter * feat(images): store images' own categories in the library database (#332) One new table, image_categories, and its category index, created empty by a forward-only migration that reads and writes only its own objects. Startup checks the structure; restore also checks the values. The store sets, clears and reads own categories by file key (the deepest output source holding the file and the path below it), and copies a removed duplicate's category to a kept copy that has none. * feat(images): duplicate cleanup keeps images' own categories (#332) The kept copy's own category wins; a kept copy without one receives the removed copy's, keyed by the deepest output source holding each file. The removed copy's row is never deleted, so a give-back or a failed delete loses nothing. The job log reports how many were kept. * feat(images): set, remove and list images' own categories (#332) PUT /prompt_manager/images/category sets the own category of up to 500 listed files, DELETE removes it (listed or not), and GET /prompt_manager/images/categories lists the values whose file is listed. Targets are the Images listing identity of an output source, checked by media_target (shared with Collections). The routes never touch prompts, recipes or links; POST /prompt_manager/bulk/category is unchanged. * fix(images): key symlinked output folders and keep key work off the event loop (#332) Review fixes: a source matches by its configured spelling too, so a detected output folder that is a symlink keys its files (routes and duplicate cleanup). File keys are computed in the executor. A path no source holds is a plain 403, a lone surrogate a 400. The install repairs a table that lacks its category index instead of refusing to open. * fix(collections): address review findings * feat(images): set or remove a category on all N matching images (#332) The category routes also accept the Images listing filter of "Select all N matching" (#392) instead of explicit items, resolved now and capped at 500 images like an explicit selection. * test(vendor): count the #332 host case in the execution matrix image-attribution.spec.mjs gains the image category route case, so the execution config lists 92 tests (3 attribution cases per project). * test(browser): install the slideshow clock before wrapping setTimeout The slideshow fixture wrapped window.setTimeout before page.clock.install(), so the viewer's timer ran on real time and clock.runFor() did not drive it; a late WebKit timer failed 'slideshow exposes exact state labels' on CI. * test(legacy): record the image category routes in the legacy contract snapshot (#332) #332 adds PUT/DELETE /prompt_manager/images/category and GET /prompt_manager/images/categories; the legacy pages' routes are unchanged. * test(duplicates): a removed copy with a collection and a category moves both (#331, #332) --- database/duplicate_copies.py | 40 +- database/image_collections.py | 490 +++++++++++++++ database/models.py | 14 +- database/operations.py | 2 + database/restore_stage.py | 12 +- py/api/__init__.py | 3 + py/api/collections.py | 350 +++++++++++ py/api/duplicate_files.py | 9 +- py/api/duplicate_spellings.py | 3 +- py/api/image_tag_jobs.py | 4 + py/api/images.py | 153 +++-- py/api/prompts.py | 4 + tests/browser/full-inspector.spec.mjs | 5 +- tests/browser/server.py | 2 + tests/fixtures/legacy_contract_snapshot.json | 6 + tests/js/workspace-system-backups.test.js | 4 +- tests/test_api_collections.py | 601 +++++++++++++++++++ tests/test_api_duplicate_files.py | 36 ++ tests/test_api_workspace_images.py | 204 +++++++ tests/test_database_backup.py | 44 ++ tests/test_duplicate_image_categories.py | 33 + tests/test_image_collections.py | 268 +++++++++ tests/test_image_collections_schema.py | 521 ++++++++++++++++ tests/test_image_tag_jobs.py | 10 + tests/test_restore_plans.py | 1 + tests/test_restore_stage.py | 43 ++ 26 files changed, 2801 insertions(+), 61 deletions(-) create mode 100644 database/image_collections.py create mode 100644 py/api/collections.py create mode 100644 tests/test_api_collections.py create mode 100644 tests/test_image_collections.py create mode 100644 tests/test_image_collections_schema.py diff --git a/database/duplicate_copies.py b/database/duplicate_copies.py index 22129786..5399b7e1 100644 --- a/database/duplicate_copies.py +++ b/database/duplicate_copies.py @@ -9,6 +9,8 @@ IMMEDIATE`` transaction, so no row ever names a deleted file: already links the kept copy: then its saved-pair links join that record and it is deleted; - its favorite; +- its collection memberships (#331), joined onto the kept copy's as their + union under the same keys a favorite moves by; - its image tags (``media_image_tags`` of its byte record), joined onto the kept copy's byte record as their union, in order; - its own image category (#332), copied to the kept copy when that copy has @@ -54,6 +56,15 @@ INSERT_FAVORITE_SQL = "INSERT OR IGNORE INTO image_favorites VALUES (?, ?, ?)" DELETE_FAVORITE_SQL = ( "DELETE FROM image_favorites WHERE root_key = ? AND relative_path = ?" ) +COPY_MEMBERSHIPS_SQL = ( + "INSERT OR IGNORE INTO image_collection_items " + "(collection_id, root_key, relative_path, added_at) " + "SELECT collection_id, ?, ?, added_at FROM image_collection_items " + "WHERE root_key = ? AND relative_path = ?" +) +DELETE_MEMBERSHIPS_SQL = ( + "DELETE FROM image_collection_items WHERE root_key = ? AND relative_path = ?" +) COPY_BYTE_SQL = ( f"SELECT b.id FROM {FILES_TABLE} f JOIN {BYTES_TABLE} b " # nosec B608 "ON b.file_id=f.id WHERE f.locator=? AND b.sha256=?" @@ -77,6 +88,8 @@ FILE_LINK_SQL = ( DELETE_LINK_SQL, FAVORITE_SQL, DELETE_FAVORITE_SQL, + COPY_MEMBERSHIPS_SQL, + DELETE_MEMBERSHIPS_SQL, COPY_BYTE_SQL, COPY_TAG_COUNT_SQL, PENDING_COUNT_SQL, @@ -140,14 +153,17 @@ class DuplicateCopyOperations: by), ``targets`` (``{removed spelling: kept spelling}``, the spelling a moved record takes; else ``kept_path``), ``favorites`` (``(root key, removed key, kept root key, kept key)`` per listed root holding the - file), ``removed_locator``, - ``kept_locator``, ``sha256`` and the kept copy's ``kept_size`` and - ``kept_mtime_ns``; ``categories``: ``(removed file key, kept file - key)`` pairs of the image categories (#332). Returns ``{moved, - merged, favorites, tags, review, categories}`` (``categories``: the - image categories copied to the kept copy). + file; they move its favorite and its collection memberships), + ``removed_locator``, ``kept_locator``, ``sha256`` and the kept copy's + ``kept_size`` and ``kept_mtime_ns``; ``categories``: ``(removed file + key, kept file key)`` pairs of the image categories (#332). Returns + ``{moved, merged, favorites, tags, review, collections, categories}`` + (``collections``: the memberships inserted on the kept copy; + ``categories``: the image categories copied to the kept copy). """ - counts = dict.fromkeys(("moved", "merged", "favorites", "tags", "review"), 0) + counts = dict.fromkeys( + ("moved", "merged", "favorites", "tags", "review", "collections"), 0 + ) kept_keys = [normalize_image_path(path) for path in move["kept"]] with self._pair_transaction() as conn: for spelled in dict.fromkeys(move["removed"]): @@ -180,6 +196,7 @@ class DuplicateCopyOperations: counts["moved"] += 1 for favorite in move.get("favorites", ()): counts["favorites"] += _move_favorite(conn, *favorite) + counts["collections"] += _move_memberships(conn, *favorite) counts["tags"] = self._move_copy_tags(conn, move) counts["categories"] = sum( copy_image_category(conn, removed, kept) @@ -224,3 +241,12 @@ def _move_favorite(conn, root_key, removed_key, kept_root, kept_key): conn.execute(INSERT_FAVORITE_SQL, (kept_root, kept_key, row[0])) conn.execute(DELETE_FAVORITE_SQL, (root_key, removed_key)) return 1 + + +def _move_memberships(conn, root_key, removed_key, kept_root, kept_key): + """Copy a removed copy's memberships to the kept copy; the rows inserted.""" + inserted = conn.execute( + COPY_MEMBERSHIPS_SQL, (kept_root, kept_key, root_key, removed_key) + ).rowcount + conn.execute(DELETE_MEMBERSHIPS_SQL, (root_key, removed_key)) + return inserted diff --git a/database/image_collections.py b/database/image_collections.py new file mode 100644 index 00000000..9eeb4494 --- /dev/null +++ b/database/image_collections.py @@ -0,0 +1,490 @@ +"""Collections (#331): named, hand-picked groups of image files. + +A member names its file exactly as a favorite does (``image_favorites.py``): +the source folder's real, case-normalized path (``root_key``) and the path +below it (``relative_path``), both server-side only. A member whose file is +moved, renamed or deleted outside PromptManager is kept and simply not listed; +nothing follows a file to a new place. + +Names are trimmed with each run of whitespace collapsed to one space, 1 to 100 +characters without control characters; ``name_key`` (NFC of the case-folded +NFC name) is UNIQUE, so names are unique ignoring case and Unicode form. + +The two tables install like ``image_favorites`` and ``saved_views``: optional +in old backups, required once a database is current, created empty with no +backfill. Startup checks only the structure, so a value saved under older +rules never stops the library from opening; restore checks the values too, +but never recomputes ``name_key`` (case tables differ between Python +versions). +""" + +import re +import sqlite3 +import unicodedata +from typing import Optional + +from .image_favorites import MAX_KEY_LENGTH + +COLLECTIONS_TABLE = "image_collections" +ITEMS_TABLE = "image_collection_items" +FILE_INDEX = "idx_image_collection_items_file" +TABLES = (COLLECTIONS_TABLE, ITEMS_TABLE) +MAX_NAME_LENGTH = 100 +_TIMESTAMP_BYTES = 64 +# A name key's byte bound: case folding expands a character to at most +# three, each at most 4 UTF-8 bytes. +_MAX_KEY_BYTES = MAX_NAME_LENGTH * 3 * 4 +_WHITESPACE = re.compile(r"\s+") + +_TABLE_DDL = { + COLLECTIONS_TABLE: ( + f"CREATE TABLE {COLLECTIONS_TABLE} (id INTEGER PRIMARY KEY AUTOINCREMENT, " + "name TEXT NOT NULL, name_key TEXT NOT NULL UNIQUE, " + "created_at TEXT NOT NULL)" + ), + ITEMS_TABLE: ( + f"CREATE TABLE {ITEMS_TABLE} (collection_id INTEGER NOT NULL REFERENCES " + f"{COLLECTIONS_TABLE}(id) ON DELETE CASCADE, root_key TEXT NOT NULL, " + "relative_path TEXT NOT NULL, added_at TEXT NOT NULL, " + "PRIMARY KEY (collection_id, root_key, relative_path))" + ), +} +_INDEX_DDL = f"CREATE INDEX {FILE_INDEX} ON {ITEMS_TABLE}(root_key, relative_path)" +# (name, declared type, not null, default, primary key position) +_COLUMNS = { + COLLECTIONS_TABLE: ( + ("id", "INTEGER", 0, None, 1), + ("name", "TEXT", 1, None, 0), + ("name_key", "TEXT", 1, None, 0), + ("created_at", "TEXT", 1, None, 0), + ), + ITEMS_TABLE: ( + ("collection_id", "INTEGER", 1, None, 1), + ("root_key", "TEXT", 1, None, 2), + ("relative_path", "TEXT", 1, None, 3), + ("added_at", "TEXT", 1, None, 0), + ), +} +# (column, parent table, parent column, on delete) +_FOREIGN_KEYS = { + COLLECTIONS_TABLE: set(), + ITEMS_TABLE: {("collection_id", COLLECTIONS_TABLE, "id", "CASCADE")}, +} + + +class CollectionInvalid(ValueError): + """A name, id or target breaks the write rules (a 400).""" + + +def _has_control(text: str) -> bool: + """Any Unicode control character (category Cc: C0, DEL and C1).""" + return any(unicodedata.category(char) == "Cc" for char in text) + + +def normalize_collection_name(name: object) -> str: + """The stored spelling of *name*: trimmed, whitespace collapsed, checked. + + Raises ``CollectionInvalid`` unless the result is 1 to 100 characters + without control characters. + """ + if not isinstance(name, str): + raise CollectionInvalid("Name must be a string") + collapsed = _WHITESPACE.sub(" ", name).strip() + if not 1 <= len(collapsed) <= MAX_NAME_LENGTH: + raise CollectionInvalid(f"Name must be 1 to {MAX_NAME_LENGTH} characters") + if _has_control(collapsed): + raise CollectionInvalid("Name contains control characters") + return collapsed + + +def collection_name_key(name: str) -> str: + """The uniqueness key of a name: NFC of the case-folded NFC spelling.""" + return unicodedata.normalize("NFC", unicodedata.normalize("NFC", name).casefold()) + + +def _compact(sql: str) -> str: + return re.sub(r"\s+", "", sql).lower() + + +def _expected_catalog() -> set: + objects = {("table", name, _compact(sql)) for name, sql in _TABLE_DDL.items()} + objects.add(("index", FILE_INDEX, _compact(_INDEX_DDL))) + objects |= {("index", f"sqlite_autoindex_{table}_1", "") for table in TABLES} + return objects + + +def _catalog(conn: sqlite3.Connection) -> list: + # sqlite_sequence (AUTOINCREMENT) is shared with other tables and ignored. + return conn.execute( + "SELECT type,name,sql FROM sqlite_master " + "WHERE name IN (?,?,?) OR tbl_name IN (?,?)", + (*TABLES, FILE_INDEX, *TABLES), + ).fetchall() + + +def _structure_error(conn: sqlite3.Connection) -> Optional[str]: + for table, expected in _COLUMNS.items(): + columns = tuple( + (row[1], row[2].upper(), row[3], row[4], row[5]) + for row in conn.execute(f"PRAGMA table_info({table})") + ) + if columns != expected: + return f"Image collections table {table} has invalid columns" + foreign_keys = { + (row[3], row[2], row[4], row[6]) + for row in conn.execute(f"PRAGMA foreign_key_list({table})") + } + if foreign_keys != _FOREIGN_KEYS[table]: + return f"Image collections table {table} has invalid foreign keys" + return None + + +def _decoded_too_long(conn, sql: str, limit: int) -> bool: + """Whether a byte-bounded candidate row decodes to over *limit* characters.""" + for row in conn.execute(sql): + for raw in row: + if len(bytes(raw).decode("utf-8", "replace")) > limit: + return True + return False + + +def _value_error(conn: sqlite3.Connection) -> Optional[str]: + # Only this module's constants are interpolated into the SQL below. Bytes + # bound characters from above (UTF-8 is at most 4 bytes each); the few + # candidates over the character bound are measured decoded in Python. + if conn.execute( + f"SELECT 1 FROM {COLLECTIONS_TABLE} WHERE " # nosec B608 + "typeof(id) != 'integer' OR typeof(name) != 'text' " + "OR typeof(name_key) != 'text' OR typeof(created_at) != 'text' " + f"OR length(CAST(name AS BLOB)) > {MAX_NAME_LENGTH * 4} " + "OR length(CAST(name_key AS BLOB)) = 0 " + f"OR length(CAST(name_key AS BLOB)) > {_MAX_KEY_BYTES} " + f"OR length(CAST(created_at AS BLOB)) > {_TIMESTAMP_BYTES} LIMIT 1" + ).fetchone(): + return "Image collections contains invalid values" + # The key is not recomputed: Unicode case tables differ between Python + # versions, and a key written by another version must stay restorable. + # Its UNIQUE constraint keeps names distinct; writes compute it. + for (name,) in conn.execute(f"SELECT name FROM {COLLECTIONS_TABLE}"): # nosec B608 + if ( + not 1 <= len(name) <= MAX_NAME_LENGTH + or _has_control(name) + or name != name.strip() + ): + return "Image collections contains invalid names" + limit = MAX_KEY_LENGTH * 4 + if conn.execute( + f"SELECT 1 FROM {ITEMS_TABLE} WHERE " # nosec B608 + "typeof(collection_id) != 'integer' OR typeof(root_key) != 'text' " + "OR typeof(relative_path) != 'text' OR typeof(added_at) != 'text' " + f"OR length(CAST(root_key AS BLOB)) > {limit} " + f"OR length(CAST(relative_path AS BLOB)) > {limit} " + f"OR length(CAST(added_at AS BLOB)) > {_TIMESTAMP_BYTES} LIMIT 1" + ).fetchone() or _decoded_too_long( + conn, + f"SELECT CAST(root_key AS BLOB), CAST(relative_path AS BLOB) " # nosec B608 + f"FROM {ITEMS_TABLE} WHERE length(CAST(root_key AS BLOB)) > {MAX_KEY_LENGTH} " + f"OR length(CAST(relative_path AS BLOB)) > {MAX_KEY_LENGTH}", + MAX_KEY_LENGTH, + ): + return "Image collections contains invalid members" + if conn.execute(f"PRAGMA foreign_key_check({ITEMS_TABLE})").fetchone(): + return "Image collections contains members of a missing collection" + return None + + +def image_collections_schema_error( + conn: sqlite3.Connection, + *, + allow_missing: bool = True, + check_values: bool = True, +) -> Optional[str]: + """Reject partial or unsupported collections schemas and invalid rows. + + Reads only: nothing is ever repaired. A database without either table is + acceptable for backups that predate them unless ``allow_missing`` is + false. ``check_values=False`` (live startup) checks the structure only. + """ + objects = _catalog(conn) + if not objects: + return None if allow_missing else "Missing image collections schema" + present = {name for kind, name, _sql in objects if kind == "table"} + if present != set(TABLES): + return "Incomplete image collections schema" + actual = {(kind, name, _compact(sql or "")) for kind, name, sql in objects} + if actual != _expected_catalog(): + return "Image collections has an invalid schema definition" + error = _structure_error(conn) + if error or not check_values: + return error + return _value_error(conn) + + +def migrate_image_collections(conn: sqlite3.Connection) -> None: + """Install both tables atomically; validate before and after creating them.""" + conn.execute("SAVEPOINT image_collections_schema") + try: + error = image_collections_schema_error(conn, check_values=False) + if error: + raise sqlite3.DatabaseError(error) + for sql in _TABLE_DDL.values(): + conn.execute(sql.replace("CREATE TABLE", "CREATE TABLE IF NOT EXISTS", 1)) + conn.execute(_INDEX_DDL.replace("INDEX", "INDEX IF NOT EXISTS", 1)) + error = image_collections_schema_error( + conn, allow_missing=False, check_values=False + ) + if error: + raise sqlite3.DatabaseError(error) + conn.execute("RELEASE image_collections_schema") + except BaseException: + conn.execute("ROLLBACK TO image_collections_schema") + conn.execute("RELEASE image_collections_schema") + raise + + +class CollectionMissing(LookupError): + """No collection has the id (a 404 ``collection-not-found``).""" + + +class CollectionNameTaken(ValueError): + """Another collection already has the name's key (a 409 ``name-taken``).""" + + def __init__(self, collection_id: int): + super().__init__("Another collection has this name") + self.collection_id = collection_id + + +class CollectionFilesMissing(LookupError): + """A new name with nothing to insert (a 404 ``files-not-found``).""" + + +LIST_SQL = f"SELECT id, name FROM {COLLECTIONS_TABLE} ORDER BY name_key, id" +GET_SQL = f"SELECT id, name FROM {COLLECTIONS_TABLE} WHERE id = ?" +BY_KEY_SQL = f"SELECT id, name FROM {COLLECTIONS_TABLE} WHERE name_key = ?" +MEMBERS_SQL = ( + f"SELECT relative_path FROM {ITEMS_TABLE} " + "WHERE collection_id = ? AND root_key = ?" +) +ROOT_MEMBERS_SQL = ( + f"SELECT collection_id, relative_path FROM {ITEMS_TABLE} WHERE root_key = ?" +) +COUNTS_SQL = ( + f"SELECT c.id, (SELECT COUNT(*) FROM {ITEMS_TABLE} i " + f"WHERE i.collection_id = c.id) FROM {COLLECTIONS_TABLE} c" +) +FILE_SQL = ( + f"SELECT collection_id FROM {ITEMS_TABLE} " + "WHERE root_key = ? AND relative_path = ? ORDER BY collection_id" +) +# Every read, for the EXPLAIN QUERY PLAN check: none scans the items table and +# only the two whole-list reads (LIST_SQL, COUNTS_SQL) scan the collections. +READ_SQL = ( + LIST_SQL, + GET_SQL, + BY_KEY_SQL, + MEMBERS_SQL, + ROOT_MEMBERS_SQL, + COUNTS_SQL, + FILE_SQL, +) +_INSERT_ITEM_SQL = f"INSERT OR IGNORE INTO {ITEMS_TABLE} VALUES (?, ?, ?, ?)" +_DELETE_ITEM_SQL = ( + f"DELETE FROM {ITEMS_TABLE} " + "WHERE collection_id = ? AND root_key = ? AND relative_path = ?" +) + + +def _collection_id(value: object) -> int: + if type(value) is not int or not 1 <= value <= 999_999_999: + raise CollectionInvalid("Collection id must be a positive integer") + return value + + +def _items(items: object) -> list: + """Distinct ``(root key, relative key)`` pairs, in order.""" + if not isinstance(items, (list, tuple)): + raise CollectionInvalid("Items must be a list") + found = [] + for item in items: + if ( + not isinstance(item, (list, tuple)) + or len(item) != 2 + or not all( + isinstance(part, str) and 1 <= len(part) <= MAX_KEY_LENGTH + for part in item + ) + ): + raise CollectionInvalid("Each item is a root key and a relative key") + found.append((item[0], item[1])) + return list(dict.fromkeys(found)) + + +def _row(row) -> dict: + return {"id": row[0], "name": row[1]} + + +class ImageCollectionOperations: + """PromptDatabase mixin: collections and their members. + + Each write runs in its own ``BEGIN IMMEDIATE`` transaction + (``_pair_transaction``), so two tabs creating one name are serialized. + """ + + def list_collections(self) -> list[dict]: + """Every collection ``{id, name}`` in name-key order, then id.""" + rows = self.model.get_connection().execute(LIST_SQL) + return [_row(row) for row in rows] + + def get_collection(self, collection_id: int) -> Optional[dict]: + """``{id, name}`` of one collection, or None.""" + row = ( + self.model.get_connection() + .execute(GET_SQL, (_collection_id(collection_id),)) + .fetchone() + ) + return _row(row) if row else None + + def collection_exists(self, collection_id: int) -> bool: + return self.get_collection(collection_id) is not None + + def collection_members(self, collection_id: int, root_key: str) -> set[str]: + """The relative keys stored for one collection under one source.""" + rows = self.model.get_connection().execute( + MEMBERS_SQL, (_collection_id(collection_id), root_key) + ) + return {path for (path,) in rows} + + def root_collection_members(self, root_key: str) -> dict[int, set[str]]: + """``{collection id: relative keys}`` stored under one source.""" + found: dict[int, set[str]] = {} + for collection_id, path in self.model.get_connection().execute( + ROOT_MEMBERS_SQL, (root_key,) + ): + found.setdefault(collection_id, set()).add(path) + return found + + def collection_count_snapshot(self, root_keys: list) -> tuple[dict, dict]: + """``({root key: {id: relative keys}}, {id: stored rows})`` read in + one read transaction, so a write between the reads cannot skew them. + """ + conn = self.model.get_connection() + if conn.in_transaction: + raise RuntimeError("Collection counts require their own transaction") + conn.execute("BEGIN") + try: + members = {} + for root_key in dict.fromkeys(root_keys): + found: dict[int, set[str]] = {} + for collection_id, path in conn.execute(ROOT_MEMBERS_SQL, (root_key,)): + found.setdefault(collection_id, set()).add(path) + members[root_key] = found + stored = {cid: count for cid, count in conn.execute(COUNTS_SQL)} + finally: + conn.rollback() # it only read + return members, stored + + def collection_counts(self) -> dict[int, int]: + """``{collection id: stored rows}`` for every collection.""" + rows = self.model.get_connection().execute(COUNTS_SQL) + return {collection_id: count for collection_id, count in rows} + + def file_collections(self, root_key: str, relative_path: str) -> list[int]: + """Ids of the collections holding one file.""" + rows = self.model.get_connection().execute(FILE_SQL, (root_key, relative_path)) + return [collection_id for (collection_id,) in rows] + + def add_to_collection( + self, *, items: object, collection_id: object = None, name: object = None + ) -> dict: + """Add files to an existing id or a name; create the name when needed. + + A name whose key exists uses that collection; otherwise a collection + is created only when at least one item is inserted. Returns + ``{collection, created, added, already}``. + """ + from .models import utc_now_iso # models imports this module's schema + + if (collection_id is None) == (name is None): + raise CollectionInvalid("Give either a collection id or a name") + pairs = _items(items) + if name is not None: + spelled = normalize_collection_name(name) + else: + collection_id = _collection_id(collection_id) + now = utc_now_iso() + with self._pair_transaction() as conn: + created = False + if name is not None: + row = conn.execute( + BY_KEY_SQL, (collection_name_key(spelled),) + ).fetchone() + if row is None: + if not pairs: + raise CollectionFilesMissing("Nothing to add") + cursor = conn.execute( + f"INSERT INTO {COLLECTIONS_TABLE} " # nosec B608 + "(name, name_key, created_at) VALUES (?, ?, ?)", + (spelled, collection_name_key(spelled), now), + ) + row, created = (cursor.lastrowid, spelled), True + else: + row = conn.execute(GET_SQL, (collection_id,)).fetchone() + if row is None: + raise CollectionMissing("No such collection") + collection = _row(row) + added = sum( + conn.execute( + _INSERT_ITEM_SQL, (collection["id"], root, path, now) + ).rowcount + for root, path in pairs + ) + return { + "collection": collection, + "created": created, + "added": added, + "already": len(pairs) - added, + } + + def remove_from_collection(self, collection_id: object, items: object) -> dict: + """Remove files from one collection; ``{collection, removed}``.""" + collection_id = _collection_id(collection_id) + pairs = _items(items) + with self._pair_transaction() as conn: + row = conn.execute(GET_SQL, (collection_id,)).fetchone() + if row is None: + raise CollectionMissing("No such collection") + removed = sum( + conn.execute(_DELETE_ITEM_SQL, (collection_id, root, path)).rowcount + for root, path in pairs + ) + return {"collection": _row(row), "removed": removed} + + def rename_collection(self, collection_id: object, name: object) -> dict: + """The renamed ``{id, name}``; a case variant of its own name is fine.""" + collection_id = _collection_id(collection_id) + spelled = normalize_collection_name(name) + key = collection_name_key(spelled) + with self._pair_transaction() as conn: + if conn.execute(GET_SQL, (collection_id,)).fetchone() is None: + raise CollectionMissing("No such collection") + taken = conn.execute(BY_KEY_SQL, (key,)).fetchone() + if taken is not None and taken[0] != collection_id: + raise CollectionNameTaken(taken[0]) + conn.execute( + f"UPDATE {COLLECTIONS_TABLE} SET name = ?, name_key = ? " # nosec B608 + "WHERE id = ?", + (spelled, key, collection_id), + ) + return {"id": collection_id, "name": spelled} + + def delete_collection(self, collection_id: object) -> bool: + """Delete one collection and its members (never a file). True.""" + collection_id = _collection_id(collection_id) + with self._pair_transaction() as conn: + cursor = conn.execute( + f"DELETE FROM {COLLECTIONS_TABLE} WHERE id = ?", # nosec B608 + (collection_id,), + ) + if cursor.rowcount == 0: + raise CollectionMissing("No such collection") + return True diff --git a/database/models.py b/database/models.py index 25e98e44..805d665f 100644 --- a/database/models.py +++ b/database/models.py @@ -22,6 +22,10 @@ from .managed_connection import ( from . import restore_stage from .library_schema import library_schema_error, migrate_library_state from .image_favorites import image_favorites_schema_error, migrate_image_favorites +from .image_collections import ( + image_collections_schema_error, + migrate_image_collections, +) from .annotation_schema import annotation_schema_error, migrate_annotation_state from .autotag_schema import ( autotag_schema_error, @@ -338,6 +342,7 @@ class PromptModel: migrate_library_state(conn) migrate_image_favorites(conn) migrate_saved_views(conn) + migrate_image_collections(conn) # A review group without its annotation group is refused before the # annotation install could make it look valid (catalog only; the rows # are validated once, in migrate_review_state). @@ -1159,8 +1164,8 @@ class PromptModel: path: File to inspect require_library_state: Also require the tables every current database has (Library state, image favorites, image - categories, saved views, image annotations and the review - queue), as a migrated + categories, saved views, collections, image annotations and + the review queue), as a migrated restore stage must; old backups may lack them. documents: Read every workflow document and refuse a damaged one. A restore checks its upload without it and repairs @@ -1234,6 +1239,11 @@ class PromptModel: ) if views_error: return False, views_error + collections_error = image_collections_schema_error( + conn, allow_missing=not require_library_state + ) + if collections_error: + return False, collections_error annotation_error = annotation_schema_error( conn, allow_missing=not require_library_state ) diff --git a/database/operations.py b/database/operations.py index 3087d615..a40651b4 100644 --- a/database/operations.py +++ b/database/operations.py @@ -18,6 +18,7 @@ from .models import PromptModel, normalize_image_path, utc_now_iso from .pairs import PairDependencyError, PairOperations # noqa: F401 (re-exported) from .library import LibraryOperations, titled from .image_favorites import ImageFavoriteOperations +from .image_collections import ImageCollectionOperations from .annotations import AnnotationOperations from .review import ReviewOperations from .review_decisions import ReviewDecisionOperations @@ -453,6 +454,7 @@ class PromptDatabase( ExecutionOutputOperations, HistoryReadOperations, ImageFavoriteOperations, + ImageCollectionOperations, AnnotationOperations, ReviewOperations, ReviewDecisionOperations, diff --git a/database/restore_stage.py b/database/restore_stage.py index 75d8883a..704435bf 100644 --- a/database/restore_stage.py +++ b/database/restore_stage.py @@ -59,6 +59,7 @@ SUMMARY_KEYS = ( "image_links", "runs", "saved_views", + "collections", ) @@ -144,7 +145,8 @@ def library_summary(conn: sqlite3.Connection) -> Dict[str, int]: ``positives`` are the Library's prompts (``library_counts``'s ``positive_prompts``); ``pairs`` the active saved pairs and ``archived_pairs`` the archived ones; ``image_links`` the indexed image - rows; ``runs`` the recorded executions. + rows; ``runs`` the recorded executions; ``collections`` the image + collections (#331). """ from .library import _ELIGIBLE @@ -169,6 +171,7 @@ def library_summary(conn: sqlite3.Connection) -> Dict[str, int]: "image_links": _count(conn, "generated_images"), "runs": _count(conn, "execution_runs"), "saved_views": _count(conn, "saved_views"), + "collections": _count(conn, "image_collections"), } @@ -180,6 +183,7 @@ _FINGERPRINT_SQL = ( ("prompt_pairs", "SELECT MAX(id), MAX(updated_at) FROM prompt_pairs"), ("execution_runs", "SELECT COUNT(*), MAX(rowid) FROM execution_runs"), ("saved_views", "SELECT MAX(rowid) FROM saved_views"), + ("image_collections", "SELECT MAX(rowid) FROM image_collections"), ) @@ -190,9 +194,9 @@ def live_fingerprint(conn: sqlite3.Connection): tables a restore replaces, or when a prompt, pair, run or saved view is the newest edited (counts plus the newest row and edit of each table); the apply refuses with ``plan-changed`` then. It does not see every - edit (a tag or favorite change on an older row); those are kept in the - restore's safety backup. A read transaction the caller already has is - used and left open; one opened here is ended here. + edit (a tag, favorite or collection membership change on an older row); + those are kept in the restore's safety backup. A read transaction the + caller already has is used and left open; one opened here is ended here. """ import json diff --git a/py/api/__init__.py b/py/api/__init__.py index 0d24ec0c..76fd0bb1 100644 --- a/py/api/__init__.py +++ b/py/api/__init__.py @@ -48,6 +48,7 @@ from .review import ReviewRoutesMixin from .review_recording import ReviewRecordingMixin from .review_events import ReviewEventsMixin from .saved_views import SavedViewRoutesMixin +from .collections import CollectionRoutesMixin from .history import HistoryRoutesMixin from .images import ImageRoutesMixin from .gallery_listing import WALK_STOPPED, RootListing, WalkOutcome @@ -315,6 +316,7 @@ class PromptManagerAPI( ReviewRecordingMixin, ReviewEventsMixin, SavedViewRoutesMixin, + CollectionRoutesMixin, HistoryRoutesMixin, PromptRoutesMixin, ImageRoutesMixin, @@ -686,6 +688,7 @@ class PromptManagerAPI( self._register_review_routes(routes) self._register_review_event_routes(routes) self._register_saved_view_routes(routes) + self._register_collection_routes(routes) self._register_history_routes(routes) self._register_prompt_routes(routes) self._register_image_routes(routes) diff --git a/py/api/collections.py b/py/api/collections.py new file mode 100644 index 00000000..bc8046ec --- /dev/null +++ b/py/api/collections.py @@ -0,0 +1,350 @@ +"""Collections (#331): named, hand-picked groups of images. + +Routes under ``/prompt_manager/collections``. A file is named by its listing +identity ``{root_index, root_scope, path}`` (``media_target.py``) and stored +under the favorites' keys (``_favorite_root_key``, ``_favorite_relative_key``), +which never leave the server. A stale scope (the sources were reordered) is a +409 ``source-changed`` before anything is written. Responses are +``{"success": bool, ...}``; an unexpected failure is a logged 500 with a +generic message, never the exception text. +""" + +import hmac +import re +from functools import wraps +from typing import Callable + +from aiohttp import web + +from .images import ( + _favorite_relative_key, + _favorite_root_key, + _forbidden, + _json_body_or_error, + collection_not_found, +) +from .media_target import media_target +from .prompts import bad_request + +try: + from ...database.image_collections import ( + CollectionFilesMissing, + CollectionInvalid, + CollectionMissing, + CollectionNameTaken, + normalize_collection_name, + ) + from ...database.image_favorites import MAX_KEY_LENGTH +except ImportError: + from database.image_collections import ( + CollectionFilesMissing, + CollectionInvalid, + CollectionMissing, + CollectionNameTaken, + normalize_collection_name, + ) + from database.image_favorites import MAX_KEY_LENGTH + +BASE = "/prompt_manager/collections" +# The most files one add or remove names (the bulk tagging cap, MC12). +MAX_ITEMS = 500 +MAX_ID = 999_999_999 +_PATH_ID = re.compile(r"[1-9][0-9]{0,8}") +_ROOT_INDEX = re.compile(r"[0-9]{1,9}") +_ADD_TARGETS = (frozenset({"collection_id", "items"}), frozenset({"name", "items"})) +_REMOVE_FIELDS = frozenset({"collection_id", "items"}) +_RENAME_FIELDS = frozenset({"name"}) +_MEDIA_MESSAGE = "Only media files can be added to a collection" +READ_FAILED = "Collections could not be read." +CHANGE_FAILED = "The collection could not be changed." +SOURCES_CHANGED = "The sources changed. Refresh the images and try again." +FILES_NOT_FOUND = ( + "None of the selected images could be found. Refresh the images and try again." +) +NAME_TAKEN = "Another collection already has this name." + + +class _Refused(Exception): + """A request refused before any write; carries its response.""" + + def __init__(self, response: web.Response): + super().__init__(response.status) + self.response = response + + +def _coded(status: int, code: str, message: str, **extra) -> web.Response: + return web.json_response( + {"success": False, "code": code, "error": message, **extra}, + status=status, + headers={"Cache-Control": "no-store"}, + ) + + +def _collection_response(failure: str) -> Callable: + """Map the database's refusals to their status; anything else to a 500.""" + + def decorate(handler: Callable) -> Callable: + @wraps(handler) + async def wrapped(self, request: web.Request) -> web.Response: + try: + return await handler(self, request) + except _Refused as refused: + return refused.response + except CollectionInvalid as exc: + return bad_request(str(exc)) + except CollectionMissing: + return collection_not_found() + except CollectionFilesMissing: + return _coded(404, "files-not-found", FILES_NOT_FOUND) + except CollectionNameTaken as exc: + return _coded( + 409, "name-taken", NAME_TAKEN, collection_id=exc.collection_id + ) + except Exception: + self.logger.exception("Collection request failed") + return web.json_response( + {"success": False, "error": failure}, status=500 + ) + + return wrapped + + return decorate + + +def _path_id(request: web.Request) -> int: + raw = request.match_info["collection_id"] + if not raw.isascii() or _PATH_ID.fullmatch(raw) is None: + raise _Refused(bad_request("Invalid collection id")) + return int(raw) + + +def _body_id(value: object) -> int: + if type(value) is not int or not 1 <= value <= MAX_ID: + raise _Refused(bad_request("Invalid collection id")) + return value + + +async def _body(request: web.Request, allowed: tuple) -> dict: + data, error = await _json_body_or_error(request) + if error is not None: + raise _Refused(error) + if frozenset(data) not in allowed: + names = " or ".join(", ".join(sorted(fields)) for fields in allowed) + raise _Refused(bad_request(f"Request must contain exactly {names}")) + return data + + +class CollectionRoutesMixin: + """Mixin providing the collection routes (#331).""" + + def _register_collection_routes(self, routes: web.RouteTableDef) -> None: + # The fixed paths go first: "/items" must never match "/{collection_id}". + routes.get(BASE)(self.list_collections) + routes.post(BASE + "/items")(self.add_collection_items) + routes.delete(BASE + "/items")(self.remove_collection_items) + routes.get(BASE + "/membership")(self.collection_membership) + routes.patch(BASE + "/{collection_id}")(self.rename_collection) + routes.delete(BASE + "/{collection_id}")(self.delete_collection) + + # -- reads ------------------------------------------------------------- + + @_collection_response(READ_FAILED) + async def list_collections(self, request: web.Request) -> web.Response: + """Every collection with its listed ``count`` and its ``missing`` rows. + + ``count`` is the members listed under every configured source (one + primary-key range read per source, matched against its cached + listing); ``missing`` the stored rows beyond it. When a scan or a read + fails both are null for every collection and the names still return. + """ + collections = await self._run_in_executor(self.db.list_collections) + counts = None + if collections: + try: + counts = await self._collection_counts() + except Exception: + self.logger.warning( + "Collection counts could not be read", exc_info=True + ) + rows = [] + for collection in collections: + listed, stored = (None, None) if counts is None else counts + count = None if listed is None else listed.get(collection["id"], 0) + stored_rows = None if stored is None else stored.get(collection["id"], 0) + rows.append( + { + **collection, + "count": count, + # Clamped: never below zero, whatever the snapshot saw. + "missing": None if count is None else max(0, stored_rows - count), + } + ) + return web.json_response({"success": True, "collections": rows}) + + async def _collection_counts(self) -> tuple[dict, dict]: + """``({id: listed members}, {id: stored rows})`` over every source. + + The scans come from the gallery cache; the root keys (realpath), the + listings and one read snapshot of every member and stored count run + in the executor, so a write between two reads cannot skew them. + """ + roots = list(self._get_all_output_dirs()) + root_keys = await self._run_in_executor( + lambda: [_favorite_root_key(root) for root in roots] + ) + distinct = {} # one folder configured twice counts once + for root, root_key in zip(roots, root_keys): + distinct.setdefault(root_key, root) + scans = { + root_key: await self._get_gallery_files(root) + for root_key, root in distinct.items() + } + return await self._run_in_executor(self._count_members, distinct, scans) + + def _count_members(self, roots: dict, scans: dict) -> tuple[dict, dict]: + """Blocking: match one snapshot's members against each listing.""" + members, stored = self.db.collection_count_snapshot(list(roots)) + listed: dict[int, int] = {} + for root_key, root in roots.items(): + listing = self._favorite_listing(root, scans[root_key]) + for collection_id, keys in members.get(root_key, {}).items(): + found = sum(1 for key in keys if key in listing) + listed[collection_id] = listed.get(collection_id, 0) + found + return listed, stored + + @_collection_response(READ_FAILED) + async def collection_membership(self, request: web.Request) -> web.Response: + """GET ``?root_index&root_scope&path``: the ids holding that file.""" + query = request.query + if len(query) != 3 or set(query) != {"root_index", "root_scope", "path"}: + # len() counts a repeated key (?path=a&path=b) twice. + return bad_request("Query must name root_index, root_scope and path once") + raw_index = query["root_index"] + if not raw_index.isascii() or _ROOT_INDEX.fullmatch(raw_index) is None: + return bad_request("Invalid gallery root") + roots = list(self._get_all_output_dirs()) + target = { + "root_index": int(raw_index), + "root_scope": query["root_scope"], + "path": query["path"], + } + root_key, key = await self._run_in_executor( + self._collection_key, target, roots, {} + ) + ids = await self._run_in_executor(self.db.file_collections, root_key, key) + return web.json_response({"success": True, "collection_ids": ids}) + + # -- writes ------------------------------------------------------------ + + @_collection_response(CHANGE_FAILED) + async def add_collection_items(self, request: web.Request) -> web.Response: + """POST ``{collection_id | name, items}``: add the listed files. + + Only files the source's listing holds are added; the rest count in + ``not_found``. A new name with nothing to add creates nothing (404 + ``files-not-found``). + """ + data = await _body(request, _ADD_TARGETS) + target = {} + if "name" in data: + target["name"] = normalize_collection_name(data["name"]) + else: + target["collection_id"] = _body_id(data["collection_id"]) + roots = list(self._get_all_output_dirs()) + keys = await self._run_in_executor(self._collection_items, data["items"], roots) + found, not_found = [], 0 + listings = {} + for index, root_key, key in keys: + if index not in listings: + root = roots[index] + files = await self._get_gallery_files(root) + listings[index] = await self._run_in_executor( + self._favorite_listing, root, files + ) + if key in listings[index]: + found.append((root_key, key)) + else: + not_found += 1 + result = await self._run_in_executor( + lambda: self.db.add_to_collection(items=found, **target) + ) + return web.json_response({"success": True, **result, "not_found": not_found}) + + @_collection_response(CHANGE_FAILED) + async def remove_collection_items(self, request: web.Request) -> web.Response: + """DELETE ``{collection_id, items}``: listed or not, every named file.""" + data = await _body(request, (_REMOVE_FIELDS,)) + collection_id = _body_id(data["collection_id"]) + roots = list(self._get_all_output_dirs()) + keys = await self._run_in_executor(self._collection_items, data["items"], roots) + result = await self._run_in_executor( + self.db.remove_from_collection, + collection_id, + [(root_key, key) for _index, root_key, key in keys], + ) + return web.json_response({"success": True, **result}) + + @_collection_response(CHANGE_FAILED) + async def rename_collection(self, request: web.Request) -> web.Response: + """PATCH ``{name}``: a case variant of its own name is allowed.""" + collection_id = _path_id(request) + data = await _body(request, (_RENAME_FIELDS,)) + name = normalize_collection_name(data["name"]) + collection = await self._run_in_executor( + self.db.rename_collection, collection_id, name + ) + return web.json_response({"success": True, "collection": collection}) + + @_collection_response(CHANGE_FAILED) + async def delete_collection(self, request: web.Request) -> web.Response: + """DELETE: the collection and its members; files are never touched.""" + collection_id = _path_id(request) + await self._run_in_executor(self.db.delete_collection, collection_id) + return web.json_response({"success": True, "deleted": True}) + + # -- request steps ----------------------------------------------------- + + def _collection_items(self, items: object, roots: list) -> list[tuple]: + """``(index, root key, relative key)`` per distinct file, in order. + + Blocking (realpath per root, once per request): run in the executor. + Every item is checked before anything is written: 400 for a malformed + shape, 403 for an unsafe path, 409 for a stale scope. Two spellings + that fold to one key (Windows) count once. + """ + if not isinstance(items, list) or not 1 <= len(items) <= MAX_ITEMS: + raise _Refused(bad_request(f"items must list 1 to {MAX_ITEMS} images")) + keys, sources = {}, {} + for item in items: + index = item.get("root_index") if isinstance(item, dict) else None + root_key, key = self._collection_key(item, roots, sources) + keys.setdefault((root_key, key), index) + return [(index, root_key, key) for (root_key, key), index in keys.items()] + + def _collection_key( + self, item: object, roots: list, sources: dict + ) -> tuple[str, str]: + """The stored keys of one target, or raise its refusal (blocking). + + *sources* caches ``{index: (scope, root key)}`` for one request, so + each root's realpath runs once however many items name it. + """ + target, error = media_target(item, media_message=_MEDIA_MESSAGE) + if error is not None: + raise _Refused(error) + index, scope, relative = target + if index >= len(roots): + raise _Refused(_coded(409, "source-changed", SOURCES_CHANGED)) + if index not in sources: + sources[index] = ( + self._gallery_root_scope(roots[index], index), + _favorite_root_key(roots[index]), + ) + current, root_key = sources[index] + if not hmac.compare_digest(scope, current): + raise _Refused(_coded(409, "source-changed", SOURCES_CHANGED)) + key = _favorite_relative_key(relative) + if max(len(root_key), len(key)) > MAX_KEY_LENGTH: + # A stored key past the schema bound would fail the next restore. + raise _Refused(_forbidden("Path is too long to add to a collection")) + return root_key, key diff --git a/py/api/duplicate_files.py b/py/api/duplicate_files.py index dc7f511c..34e15ed4 100644 --- a/py/api/duplicate_files.py +++ b/py/api/duplicate_files.py @@ -588,7 +588,9 @@ class DuplicateFilesMixin: job.update(total=len(targets), status_line="Removing duplicate files") spellings = await self._run_in_executor(self._source_spellings) removed, failure, keepers = 0, None, {} - totals = dict.fromkeys(("moved", "merged", "favorites", "tags", "review"), 0) + totals = dict.fromkeys( + ("moved", "merged", "favorites", "tags", "review", "collections"), 0 + ) totals["categories"] = 0 # image categories kept on the kept copies (#332) for done, target in enumerate(targets, start=1): if failure is not None: @@ -632,6 +634,11 @@ class DuplicateFilesMixin: job.log_line( f"Moved {_plural(totals['favorites'], 'favorite')} to the kept copies" ) + if totals["collections"]: + job.log_line( + f"Moved {_plural(totals['collections'], 'collection membership')} " + "to the kept copies" + ) if totals["tags"]: job.log_line( f"Added {_plural(totals['tags'], 'image tag')} to the kept copies" diff --git a/py/api/duplicate_spellings.py b/py/api/duplicate_spellings.py index 0e197aa4..7157e994 100644 --- a/py/api/duplicate_spellings.py +++ b/py/api/duplicate_spellings.py @@ -104,7 +104,8 @@ def copy_move(target, kept_stat, spellings): A record found under a folder spelling moves to the kept copy under the same spelling when that folder holds it too, else to its real - path. Its favorite moves under every listed root holding the file. + path. Its favorite and its collection memberships move under every + listed root holding the file. """ removed = spellings.of(target["abs"]) kept = spellings.of(target["keeper_abs"]) diff --git a/py/api/image_tag_jobs.py b/py/api/image_tag_jobs.py index 6baafd4c..61c79422 100644 --- a/py/api/image_tag_jobs.py +++ b/py/api/image_tag_jobs.py @@ -58,7 +58,9 @@ from .annotations import ( from .autotag_routes import BATCH_CANCELLED, MODEL_BUSY_ERROR, model_busy_response from .images import ( MAX_LISTING_SELECTION, + CollectionMissing, ListingSourceChanged, + collection_not_found, _current_fingerprint, _listing_posix, _public_path, @@ -689,6 +691,8 @@ class ImageTagJobsMixin: selection = await self.resolve_listing_filter(spec.filter, cap) except ListingSourceChanged: return None, _coded("source-changed") + except CollectionMissing: + return None, collection_not_found() except ValueError as exc: return None, bad_request(str(exc)) except Exception: diff --git a/py/api/images.py b/py/api/images.py index e030c973..614287b9 100644 --- a/py/api/images.py +++ b/py/api/images.py @@ -18,7 +18,7 @@ import urllib.parse from bisect import bisect_left, bisect_right, insort from collections.abc import Mapping from pathlib import Path -from typing import Callable, NamedTuple +from typing import Callable, Iterable, NamedTuple from aiohttp import web from PIL import Image, UnidentifiedImageError @@ -58,6 +58,7 @@ from .inspection_service import ( ) try: + from ...database.image_collections import CollectionMissing from ...database.image_favorites import MAX_KEY_LENGTH from ...database.library import display_title from ...database.models import normalize_image_path @@ -81,6 +82,7 @@ try: reveal_in_file_manager, ) except ImportError: + from database.image_collections import CollectionMissing from database.image_favorites import MAX_KEY_LENGTH from database.library import display_title from database.models import normalize_image_path @@ -110,6 +112,21 @@ IMAGE_EXTENSIONS = _IMAGE_EXTENSIONS _CARD_RECORDS = ("prompt", "workflow") _PNG_SIGNATURE = b"\x89PNG\r\n\x1a\n" _LISTING_FAILED = "The image list could not be filtered. Check server logs for details." +_COLLECTION_ID = re.compile(r"[1-9][0-9]{0,8}") +COLLECTION_NOT_FOUND = "That collection no longer exists." + + +def collection_not_found() -> web.Response: + """The 404 a write or a listing answers for an unknown collection (#331).""" + return web.json_response( + { + "success": False, + "code": "collection-not-found", + "error": COLLECTION_NOT_FOUND, + }, + status=404, + headers={"Cache-Control": "no-store"}, + ) def _card_header(path: Path) -> tuple: @@ -1110,8 +1127,9 @@ def _workspace_filters(query: Mapping) -> dict: the linked positive's text, title and tag names. ``category`` is exact, ``tags`` is a comma list that must all be present, ``sort=rating`` ranks by the linked positive, and ``favorites=1`` keeps only favorites (the - Favorites page, #249 D51). Raises ValueError for any unknown or oversized - value. + Favorites page, #249 D51). ``collection=`` (1 to 9 digits) keeps only + that collection's members (#331); it wins over ``favorites`` (a page never + sets both). Raises ValueError for any unknown or oversized value. """ text = query.get("q", "").strip() folded = text.casefold() @@ -1125,8 +1143,10 @@ def _workspace_filters(query: Mapping) -> dict: {tag.strip() for tag in query.get("tags", "").split(",") if tag.strip()} ) favorites = query.get("favorites", "0") + collection = query.get("collection", "") if ( favorites not in ("0", "1") + or (collection != "" and _COLLECTION_ID.fullmatch(collection) is None) or len(text) > 500 or len(folded) > 500 or media not in ("all", "image", "video") @@ -1149,7 +1169,8 @@ def _workspace_filters(query: Mapping) -> dict: "path": exact.casefold(), "category": category, "tags": tags, - "favorites": favorites == "1", + "favorites": favorites == "1" and not collection, + "collection": int(collection) if collection else None, } @@ -1167,6 +1188,8 @@ LISTING_FILTER_KEYS = frozenset( "media", "sort", "favorites", + # A collection page's filter (#331): bulk actions act on its members. + "collection", } ) MAX_LISTING_FILTER_VALUE = 2048 @@ -1223,7 +1246,7 @@ def _admitted_entries( entries: list, filters: dict, links: dict | None, - favorites: frozenset | None = None, + members: frozenset | None = None, image_tags: dict | None = None, own_categories: dict | None = None, ): @@ -1236,11 +1259,11 @@ def _admitted_entries( admits a file whose effective category equals it (#332 MC5): its own category (*own_categories*, ``{normalized path: category}``) when it has one, else the inherited one, so an unlinked file with its own category is - admitted. *favorites*, when given, admits only those listed - paths (``favorites=1``). *image_tags* maps normalized paths to their - _ImageTagMatch: a tag filter also admits a file whose current image tags - (with its linked positive's tags) carry every tag, and ``q`` also admits an - image tag name hit. + admitted. *members*, when given, admits only those listed paths (the + favorites of ``favorites=1`` or a ``collection``'s members). *image_tags* + maps normalized paths to their _ImageTagMatch: a tag filter also admits a + file whose current image tags (with its linked positive's tags) carry every + tag, and ``q`` also admits an image tag name hit. """ query, media, folder = filters["query"], filters["media"], filters["folder"] category = filters["category"] @@ -1258,7 +1281,7 @@ def _admitted_entries( continue if media != "all" and (media == "video") != entry.video: continue - if favorites is not None and entry.path not in favorites: + if members is not None and entry.path not in members: continue match = None if links is None else links.get(entry.normalized) image = None if image_tags is None else image_tags.get(entry.normalized) @@ -1286,7 +1309,7 @@ def _filtered_page( size: int, filters: dict, links: dict | None, - favorites: frozenset | None = None, + members: frozenset | None = None, image_tags: dict | None = None, own_categories: dict | None = None, ) -> tuple[int, list, int | None]: @@ -1298,7 +1321,7 @@ def _filtered_page( """ total, page, positives = 0, [], set() for position, entry, match in _admitted_entries( - entries, filters, links, favorites, image_tags, own_categories + entries, filters, links, members, image_tags, own_categories ): total += 1 if match is not None: @@ -2002,6 +2025,7 @@ class ImageRoutesMixin: filters["category"], filters["tags"], filters["favorites"], + filters["collection"], ] ).encode() ).hexdigest() @@ -2012,17 +2036,19 @@ class ImageRoutesMixin: except ValueError as exc: return bad_request(str(exc)) - listings = [ - (index, root, await self._get_gallery_files(root)) - for index, root in enumerate(output_dirs) - if root_index is None or index == root_index - ] try: + if filters["collection"] is not None and not await self._run_in_executor( + self.db.collection_exists, filters["collection"] + ): + raise CollectionMissing("No such collection") + listings = await self._workspace_listings(output_dirs, root_index) page, total, has_more, next_cursor, positive_count = ( await self._run_in_executor( self._select_workspace_page, listings, filters, after, limit, scope ) ) + except CollectionMissing: + return collection_not_found() except Exception: # A filter that needs the prompt links or the image tags cannot be # served without them. Never echo the exception: it can carry SQL @@ -2282,9 +2308,11 @@ class ImageRoutesMixin: ) -> tuple: """The ordered entries and what admits them (blocking; run in executor). - Returns ``(entries, key, links, image_tags, favorites, own)``: the + Returns ``(entries, key, links, image_tags, members, own)``: the listing in the filters' order, its keyset key, and the link, image-tag, - favorite and own-category maps _admitted_entries reads. One SELECT + member and own-category maps _admitted_entries reads (members: a + collection's, else the favorites, else None). A collection deleted since + the route checked it raises CollectionMissing. One SELECT (get_image_link_matches) reads every linked positive when *read_links*; under a category it keeps only links whose raw recipe or positive category is that value (the inherited one is decided in Python), and @@ -2368,10 +2396,14 @@ class ImageRoutesMixin: def key(entry): return _workspace_key(entry, order) - favorites = ( - self._workspace_favorite_paths(listings) if filters["favorites"] else None + return ( + entries, + key, + links, + image_tags, + self._workspace_members(listings, filters), + own, ) - return entries, key, links, image_tags, favorites, own def _own_category_links(self, links: dict, own: dict, filters: dict) -> dict: """*links* plus the links of files whose own category is the filter's. @@ -2423,7 +2455,7 @@ class ImageRoutesMixin: or bool(filters["tags"]) or order == "rating" ) - entries, key, links, image_tags, favorites, own = self._workspace_admission( + entries, key, links, image_tags, members, own = self._workspace_admission( listings, filters, read_links=needed or after is None ) start = 0 @@ -2434,7 +2466,7 @@ class ImageRoutesMixin: else: start = bisect_right(entries, after, key=key) total, page, positive_count = _filtered_page( - entries, start, limit + 1, filters, links, favorites, image_tags, own + entries, start, limit + 1, filters, links, members, image_tags, own ) if not needed and after is not None: positive_count = _COUNT_OMITTED @@ -2456,12 +2488,12 @@ class ImageRoutesMixin: The links are read strictly: the positives must be the listing's own. """ - entries, _key, links, image_tags, favorites, own = self._workspace_admission( + entries, _key, links, image_tags, members, own = self._workspace_admission( listings, filters, read_links=True, strict=True ) targets, positives, unlinked, scopes = [], {}, 0, {} for _position, entry, match in _admitted_entries( - entries, filters, links, favorites, image_tags, own + entries, filters, links, members, image_tags, own ): if len(targets) == cap: return ListingSelection(targets, list(positives), unlinked, True) @@ -2483,9 +2515,11 @@ class ImageRoutesMixin: ) -> ListingSelection: """The images a parse_listing_filter result admits right now (#392). - Raises ValueError for a root outside the current roots, and + Raises ValueError for a root outside the current roots, ListingSourceChanged when the filter's ``root_scope`` no longer names - its root (the sources were renumbered since the listing). + its root (the sources were renumbered since the listing), and + CollectionMissing when its ``collection`` no longer exists (checked + before any scan and again after the members are read). """ output_dirs = list(self._get_all_output_dirs()) root_index = _workspace_root(params, output_dirs) @@ -2495,11 +2529,11 @@ class ImageRoutesMixin: raise ListingSourceChanged() raise ValueError("Invalid gallery root scope") filters = _workspace_filters(params) - listings = [ - (index, root, await self._get_gallery_files(root)) - for index, root in enumerate(output_dirs) - if root_index is None or index == root_index - ] + if filters["collection"] is not None and not await self._run_in_executor( + self.db.collection_exists, filters["collection"] + ): + raise CollectionMissing("No such collection") + listings = await self._workspace_listings(output_dirs, root_index) return await self._run_in_executor( self._select_workspace_matches, listings, filters, cap ) @@ -3684,25 +3718,58 @@ class ImageRoutesMixin: self._favorite_listing_cache[cache_key] = (files, listing) return listing - def _workspace_favorite_paths(self, listings: list[tuple]) -> frozenset: - """The listed paths of every favorite in the selected roots (blocking). + def _workspace_members(self, listings: list[tuple], filters: dict): + """The member set the filters ask for, or None (blocking).""" + collection = filters.get("collection") + if collection is not None: + members = self._workspace_member_paths( + listings, + lambda root_key: self.db.collection_members(collection, root_key), + ) + # Checked after the reads: a collection deleted before or while + # they ran is a 404, never an empty page passing for it. + if not self.db.collection_exists(collection): + raise CollectionMissing("No such collection") + return members + if filters["favorites"]: + return self._workspace_favorite_paths(listings) + return None - One primary-key read per root (get_root_favorites), matched against the - root's cached ``{relative key: listing spelling}``, so the work follows - the favorites rather than the files, and a key folds case exactly as - the toggle stored it (Windows). A favorite whose file is gone matches - nothing. A failed read raises: an unfiltered page must never pass for - the favorites. + def _workspace_favorite_paths(self, listings: list[tuple]) -> frozenset: + """The listed paths of every favorite in the selected roots (blocking).""" + return self._workspace_member_paths(listings, self.db.get_root_favorites) + + def _workspace_member_paths( + self, listings: list[tuple], keys_for_source: Callable[[str], Iterable[str]] + ) -> frozenset: + """The listed paths of the stored keys of each selected root (blocking). + + One read per root (``keys_for_source(root key)``: the favorites' or a + collection's primary-key range), matched against the root's cached + ``{relative key: listing spelling}``, so the work follows the members + rather than the files, and a key folds case exactly as it was stored + (Windows). A member whose file is gone matches nothing. A failed read + raises: an unfiltered page must never pass for the members. """ paths = set() for _index, root, files in listings: listing = self._favorite_listing(root, files) - for key in self.db.get_root_favorites(_favorite_root_key(root)): + for key in keys_for_source(_favorite_root_key(root)): listed = listing.get(key) if listed is not None: paths.add(root / listed) return frozenset(paths) + async def _workspace_listings( + self, output_dirs: list[Path], root_index: int | None + ) -> list[tuple]: + """``(index, root, scan)`` of every selected root (cached scans).""" + return [ + (index, root, await self._get_gallery_files(root)) + for index, root in enumerate(output_dirs) + if root_index is None or index == root_index + ] + def _workspace_favorites(self, page: list[tuple]) -> set[tuple[int, str]]: """``(root index, listing path)`` of the favorites among *page* entries.""" by_root: dict[tuple[int, Path], dict[str, str]] = {} diff --git a/py/api/prompts.py b/py/api/prompts.py index e48296ea..8f5a1554 100644 --- a/py/api/prompts.py +++ b/py/api/prompts.py @@ -1227,7 +1227,9 @@ class PromptRoutesMixin: """ from .images import ( MAX_LISTING_SELECTION, + CollectionMissing, ListingSourceChanged, + collection_not_found, parse_listing_filter, ) @@ -1252,6 +1254,8 @@ class PromptRoutesMixin: }, status=409, ) + except CollectionMissing: + return collection_not_found() except ValueError as exc: return refused(str(exc)) if selection.truncated: diff --git a/tests/browser/full-inspector.spec.mjs b/tests/browser/full-inspector.spec.mjs index e4efc5a7..bd530bed 100644 --- a/tests/browser/full-inspector.spec.mjs +++ b/tests/browser/full-inspector.spec.mjs @@ -676,6 +676,9 @@ test('collection viewer uses real public transforms and resets on navigation',as async function slideshowFixture(page,request,preferences={videoAutoplay:false,videoMute:true,videoLoop:false}) { await expect(await request.post('/__test__/reset',{data:{full_inspector:true,include_video:true}})).toBeOK(); await page.goto('/prompt_manager/workspace#/images'); + // Install the fake clock before the fixture wraps setTimeout: the wrapped timer (which the viewer session captures) + // must be the fake one, so page.clock.runFor alone decides when the 3500 ms slideshow deadline fires. + await page.clock.install(); await page.evaluate(async preferences=>{ if(preferences) localStorage.setItem('gallerySettings',JSON.stringify(preferences)); const [{createMediaViewer},{createInspectionController},{createScope},{createPromptSender}]=await Promise.all([ @@ -695,7 +698,7 @@ async function slideshowFixture(page,request,preferences={videoAutoplay:false,vi },preferences); await expect(mediaDialog(page).getByRole('button',{name:'Zoom in',exact:true})).toHaveAttribute('aria-disabled','false'); await expect(mediaDialog(page).locator('#viewer-details > div > p.small')).toHaveText('complete.png'); - await page.clock.install();await page.clock.pauseAt(new Date(Date.now()+1000)); + await page.clock.pauseAt(new Date(Date.now()+1000)); } const slideshowButton=page=>mediaDialog(page).locator('#viewer-toolbar').getByRole('button',{name:/^(Play|Pause|Stop) slideshow$/}); const timerCount=page=>page.evaluate(()=>window.slideshowTimers.size); diff --git a/tests/browser/server.py b/tests/browser/server.py index 160f4b0a..dab18015 100644 --- a/tests/browser/server.py +++ b/tests/browser/server.py @@ -262,6 +262,8 @@ def create_app(root: Path) -> web.Application: with target.db.model.get_connection() as conn: for table in ( "saved_views", + "image_collection_items", + "image_collections", "image_favorites", "media_review_decisions", "media_review_items", diff --git a/tests/fixtures/legacy_contract_snapshot.json b/tests/fixtures/legacy_contract_snapshot.json index eb2085cd..27568173 100644 --- a/tests/fixtures/legacy_contract_snapshot.json +++ b/tests/fixtures/legacy_contract_snapshot.json @@ -1,5 +1,7 @@ { "routes": [ + "DELETE /prompt_manager/collections/items", + "DELETE /prompt_manager/collections/{collection_id}", "DELETE /prompt_manager/delete/{prompt_id}", "DELETE /prompt_manager/image-annotations/capture/{token}", "DELETE /prompt_manager/images/category", @@ -15,6 +17,8 @@ "GET /prompt_manager/backup", "GET /prompt_manager/backup/status", "GET /prompt_manager/categories", + "GET /prompt_manager/collections", + "GET /prompt_manager/collections/membership", "GET /prompt_manager/diagnostics", "GET /prompt_manager/export", "GET /prompt_manager/gallery", @@ -92,6 +96,7 @@ "GET /prompt_manager/vocabulary/tags/{tag_id}/impact", "GET /prompt_manager/web", "GET /prompt_manager/workspace", + "PATCH /prompt_manager/collections/{collection_id}", "PATCH /prompt_manager/library/{positive_id}", "PATCH /prompt_manager/pairs/{pair_id}", "PATCH /prompt_manager/saved-views/{view_id}", @@ -106,6 +111,7 @@ "POST /prompt_manager/bulk/delete", "POST /prompt_manager/bulk/tags", "POST /prompt_manager/cleanup", + "POST /prompt_manager/collections/items", "POST /prompt_manager/delete_duplicate_images", "POST /prompt_manager/diagnostics/test-link", "POST /prompt_manager/gallery/rescan", diff --git a/tests/js/workspace-system-backups.test.js b/tests/js/workspace-system-backups.test.js index 05899351..3a1454ae 100644 --- a/tests/js/workspace-system-backups.test.js +++ b/tests/js/workspace-system-backups.test.js @@ -63,9 +63,9 @@ test('status line shows only server-reported backups (D103)', async () => { const PREVIEW = Object.freeze({ success: true, token: 'r'.repeat(43), expires_at: '2026-10-25T12:15:00+00:00', - summary: { prompts: 1234, positives: 900, pairs: 3, archived_pairs: 0, image_links: 56, runs: 1, saved_views: 2 }, + summary: { prompts: 1234, positives: 900, pairs: 3, archived_pairs: 0, image_links: 56, runs: 1, saved_views: 2, collections: 1 }, schema: { migrated: true, library_state: true }, integrity: 'ok', documents: { repaired: 0, cleared: 0, images: 0 }, - live: { prompts: 1, positives: 1, pairs: 0, archived_pairs: 0, image_links: 0, runs: 0, saved_views: 0 }, size: 4096, + live: { prompts: 1, positives: 1, pairs: 0, archived_pairs: 0, image_links: 0, runs: 0, saved_views: 0, collections: 0 }, size: 4096, safety_backup: { destination_label: 'prompts.db.backup_' }, }); diff --git a/tests/test_api_collections.py b/tests/test_api_collections.py new file mode 100644 index 00000000..daa0e53b --- /dev/null +++ b/tests/test_api_collections.py @@ -0,0 +1,601 @@ +"""Collections (#331): the routes under /prompt_manager/collections.""" + +import json +import ntpath +import sqlite3 +from unittest.mock import patch + +try: + from test_api_images import ImageAPITestCase, make_png + from open_handles import assert_closed +except ImportError: + from tests.test_api_images import ImageAPITestCase, make_png + from tests.open_handles import assert_closed + +BASE = "/prompt_manager/collections" +ITEMS = BASE + "/items" +MEMBERSHIP = BASE + "/membership" +SOURCES_CHANGED = "The sources changed. Refresh the images and try again." + + +class CollectionRoutesTestCase(ImageAPITestCase): + async def tearDownAsync(self): + self.api.db.close_all() + assert_closed(self, self._temp_db.name, self._output_tmp.name) + await super().tearDownAsync() + + def make(self, *names, root=None): + for name in names: + make_png((root or self.output_dir) / name) + + def scope(self, index=0): + roots = list(self.api._get_all_output_dirs()) + return self.api._gallery_root_scope(roots[index], index) + + def item(self, path, index=0, scope=None): + return { + "root_index": index, + "root_scope": scope or self.scope(index), + "path": path, + } + + def two_sources(self): + second = self.comfy_dir / "second" + second.mkdir() + self.api._get_all_output_dirs = lambda: [self.output_dir, second] + return second + + async def call(self, method, url, body=None, status=200, **params): + response = await self.client.request(method, url, json=body, params=params) + text = await response.text() + self.assertEqual(response.status, status, text) + data = json.loads(text) + self.assertNotIn(self.comfy_dir.as_posix(), text) + self.assertNotIn(str(self.comfy_dir).replace("\\", "\\\\"), text) + return data + + async def add(self, status=200, **body): + return await self.call("POST", ITEMS, body, status) + + async def listed(self): + return (await self.call("GET", BASE))["collections"] + + +class TestCollectionRoutes(CollectionRoutesTestCase): + async def test_each_route_succeeds(self): + self.make("a.png", "b.png", "c.png") + self.assertEqual( + await self.call("GET", BASE), {"success": True, "collections": []} + ) + made = await self.add( + name=" LoRa testing ", items=[self.item("a.png"), self.item("b.png")] + ) + grids = made["collection"] + self.assertEqual(grids["name"], "LoRa testing") + self.assertEqual( + made, + { + "success": True, + "collection": grids, + "created": True, + "added": 2, + "already": 0, + "not_found": 0, + }, + ) + again = await self.add( + collection_id=grids["id"], items=[self.item("b.png"), self.item("c.png")] + ) + self.assertEqual( + (again["created"], again["added"], again["already"]), (False, 1, 1) + ) + self.assertEqual( + await self.listed(), + [{"id": grids["id"], "name": "LoRa testing", "count": 3, "missing": 0}], + ) + membership = await self.call( + "GET", MEMBERSHIP, root_index="0", root_scope=self.scope(), path="a.png" + ) + self.assertEqual(membership, {"success": True, "collection_ids": [grids["id"]]}) + removed = await self.call( + "DELETE", + ITEMS, + {"collection_id": grids["id"], "items": [self.item("a.png")]}, + ) + self.assertEqual(removed, {"success": True, "collection": grids, "removed": 1}) + renamed = await self.call( + "PATCH", f"{BASE}/{grids['id']}", {"name": "LoRA grids"} + ) + self.assertEqual( + renamed, + {"success": True, "collection": {"id": grids["id"], "name": "LoRA grids"}}, + ) + deleted = await self.call("DELETE", f"{BASE}/{grids['id']}") + self.assertEqual(deleted, {"success": True, "deleted": True}) + self.assertEqual(await self.listed(), []) + # Files are never touched. + self.assertTrue((self.output_dir / "a.png").is_file()) + + async def test_names_are_unique_ignoring_case(self): + self.make("a.png", "b.png") + first = (await self.add(name="LoRa testing", items=[self.item("a.png")]))[ + "collection" + ] + second = await self.add(name="lora TESTING", items=[self.item("b.png")]) + self.assertEqual(second["collection"], first) + self.assertFalse(second["created"]) + self.assertEqual(len(await self.listed()), 1) + + async def test_bad_requests_are_400(self): + self.make("a.png") + made = await self.add(name="Set", items=[self.item("a.png")]) + cid = made["collection"]["id"] + good = [self.item("a.png")] + for body in ( + {"items": good}, + {"name": "x", "collection_id": cid, "items": good}, + {"name": "x"}, + {"name": "x", "items": good, "extra": 1}, + {"collection_id": "1", "items": good}, + {"collection_id": True, "items": good}, + {"collection_id": 0, "items": good}, + {"collection_id": 10**10, "items": good}, + {"name": "", "items": good}, + {"name": "x" * 101, "items": good}, + {"name": "bell\x07", "items": good}, + {"name": 5, "items": good}, + {"name": "x", "items": []}, + {"name": "x", "items": "a.png"}, + {"name": "x", "items": [self.item("a.png")] * 501}, + {"name": "x", "items": [{"root_index": 0, "path": "a.png"}]}, + {"name": "x", "items": [{**self.item("a.png"), "root_index": "0"}]}, + {"name": "x", "items": [{**self.item("a.png"), "root_scope": "xyz"}]}, + ): + with self.subTest(body=body): + data = await self.add(400, **body) + self.assertFalse(data["success"]) + for body in ({"items": good}, {"collection_id": cid}): + with self.subTest(remove=body): + await self.call("DELETE", ITEMS, body, 400) + for raw in ("abc", "0", "1234567890", "-1", "1.0"): + with self.subTest(path_id=raw): + await self.call("PATCH", f"{BASE}/{raw}", {"name": "x"}, 400) + await self.call("DELETE", f"{BASE}/{raw}", None, 400) + await self.call("PATCH", f"{BASE}/{cid}", {"name": ""}, 400) + await self.call("PATCH", f"{BASE}/{cid}", {"label": "x"}, 400) + for params in ( + {"root_index": "x", "root_scope": self.scope(), "path": "a.png"}, + {"root_index": "0", "path": "a.png"}, + {"root_index": "0", "root_scope": "nope", "path": "a.png"}, + ): + with self.subTest(membership=params): + await self.call("GET", MEMBERSHIP, status=400, **params) + self.assertEqual((await self.listed())[0]["count"], 1) + + async def test_unsafe_paths_are_403(self): + self.make("a.png") + for path in ( + "../a.png", + "sub/../../a.png", + "C:/a.png", + "a\\b.png", + "/a.png", + "notes.txt", + ): + with self.subTest(path=path): + await self.add(403, name="Set", items=[self.item(path)]) + await self.call( + "GET", + MEMBERSHIP, + status=403, + root_index="0", + root_scope=self.scope(), + path=path, + ) + self.assertEqual(await self.listed(), []) + + async def test_a_body_over_the_cap_is_413(self): + response = await self.client.post( + ITEMS, data=b'{"name":"x","items":[' + b" " * (1024 * 1024 + 1) + b"]}" + ) + self.assertEqual(response.status, 413) + + async def test_missing_collection_and_files_are_404(self): + self.make("a.png") + data = await self.add(404, collection_id=999, items=[self.item("a.png")]) + self.assertEqual(data["code"], "collection-not-found") + data = await self.call( + "DELETE", ITEMS, {"collection_id": 999, "items": [self.item("a.png")]}, 404 + ) + self.assertEqual(data["code"], "collection-not-found") + data = await self.call("PATCH", f"{BASE}/999", {"name": "x"}, 404) + self.assertEqual(data["code"], "collection-not-found") + data = await self.call("DELETE", f"{BASE}/999", None, 404) + self.assertEqual(data["code"], "collection-not-found") + data = await self.add(404, name="New", items=[self.item("never.png")]) + self.assertEqual(data["code"], "files-not-found") + self.assertEqual(await self.listed(), []) + + async def test_unlisted_files_count_as_not_found(self): + self.make("a.png") + made = await self.add( + name="Set", items=[self.item("a.png"), self.item("later.png")] + ) + self.assertEqual((made["added"], made["not_found"]), (1, 1)) + again = await self.add( + collection_id=made["collection"]["id"], + items=[self.item("a.png"), self.item("later.png")], + ) + self.assertEqual( + (again["added"], again["already"], again["not_found"]), (0, 1, 1) + ) + + async def test_name_taken_is_409(self): + self.make("a.png") + first = (await self.add(name="First", items=[self.item("a.png")]))["collection"] + second = (await self.add(name="Second", items=[self.item("a.png")]))[ + "collection" + ] + data = await self.call( + "PATCH", f"{BASE}/{second['id']}", {"name": "FIRST"}, 409 + ) + self.assertEqual( + (data["code"], data["collection_id"]), ("name-taken", first["id"]) + ) + renamed = await self.call("PATCH", f"{BASE}/{second['id']}", {"name": "SECOND"}) + self.assertEqual(renamed["collection"]["name"], "SECOND") + + async def test_stale_scope_is_409_after_reordering_sources(self): + second = self.two_sources() + self.make("a.png") + self.make("b.png", root=second) + stale = self.item("b.png", 1) + made = await self.add(name="Set", items=[self.item("a.png")]) + cid = made["collection"]["id"] + self.api._get_all_output_dirs = lambda: [second, self.output_dir] + for method, body in ( + ("POST", {"collection_id": cid, "items": [self.item("b.png", 0), stale]}), + ("DELETE", {"collection_id": cid, "items": [stale]}), + ): + with self.subTest(method=method): + data = await self.call(method, ITEMS, body, 409) + self.assertEqual(data["code"], "source-changed") + self.assertEqual(data["error"], SOURCES_CHANGED) + data = await self.call( + "GET", + MEMBERSHIP, + status=409, + root_index="1", + root_scope=stale["root_scope"], + path="b.png", + ) + self.assertEqual(data["code"], "source-changed") + # Nothing was written by the refused add; the member follows its folder. + listed = await self.listed() + self.assertEqual((listed[0]["count"], listed[0]["missing"]), (1, 0)) + membership = await self.call( + "GET", MEMBERSHIP, root_index="1", root_scope=self.scope(1), path="a.png" + ) + self.assertEqual(membership["collection_ids"], [cid]) + + async def test_counts_exclude_a_missing_member(self): + second = self.two_sources() + self.make("a.png", "b.png") + self.make("c.png", root=second) + made = await self.add( + name="Grids", + items=[self.item("a.png"), self.item("b.png"), self.item("c.png", 1)], + ) + other = await self.add(name="Other", items=[self.item("c.png", 1)]) + (self.output_dir / "b.png").unlink() + self.api.invalidate_gallery_cache() + listed = await self.listed() + self.assertEqual( + listed, + [ + { + "id": made["collection"]["id"], + "name": "Grids", + "count": 2, + "missing": 1, + }, + { + "id": other["collection"]["id"], + "name": "Other", + "count": 1, + "missing": 0, + }, + ], + ) + # The member is removable although it is no longer listed. + removed = await self.call( + "DELETE", + ITEMS, + {"collection_id": made["collection"]["id"], "items": [self.item("b.png")]}, + ) + self.assertEqual(removed["removed"], 1) + + async def test_counts_are_null_when_a_source_scan_fails(self): + self.make("a.png") + await self.add(name="Set", items=[self.item("a.png")]) + + async def broken(_root): + raise OSError("scan failed at /secret/place") + + with patch.object(self.api, "_get_gallery_files", side_effect=broken): + data = await self.call("GET", BASE) + self.assertEqual( + data["collections"][0] | {"id": 0}, + {"id": 0, "name": "Set", "count": None, "missing": None}, + ) + self.assertNotIn("secret", json.dumps(data)) + + async def test_windows_case_spellings_are_one_member(self): + self.make("Sub/A.png") + with patch("py.api.images._path_flavor", ntpath): + made = await self.add( + name="Set", items=[self.item("Sub/A.png"), self.item("SUB/a.PNG")] + ) + self.assertEqual((made["added"], made["already"]), (1, 0)) + (self.output_dir / "Sub" / "A.png").rename( + self.output_dir / "Sub" / "a.png" + ) + (self.output_dir / "Sub").rename(self.output_dir / "sub") + self.api.invalidate_gallery_cache() + listed = await self.listed() + self.assertEqual((listed[0]["count"], listed[0]["missing"]), (1, 0)) + membership = await self.call( + "GET", + MEMBERSHIP, + root_index="0", + root_scope=self.scope(), + path="sub/a.png", + ) + self.assertEqual(membership["collection_ids"], [made["collection"]["id"]]) + response = await self.client.get( + "/prompt_manager/images/output", + params={ + "mode": "workspace", + "collection": str(made["collection"]["id"]), + }, + ) + page = await response.json() + self.assertEqual([i["path"] for i in page["images"]], ["sub/a.png"]) + + async def test_posix_case_distinct_files_stay_two_members(self): + self.make("A.png", "a.png") + if len(list(self.output_dir.iterdir())) != 2: + self.skipTest("case-insensitive filesystem") + made = await self.add( + name="Set", items=[self.item("A.png"), self.item("a.png")] + ) + self.assertEqual(made["added"], 2) + + async def test_a_500_never_carries_the_exception_text(self): + self.make("a.png") + made = await self.add(name="Set", items=[self.item("a.png")]) + cid = made["collection"]["id"] + + def broken(*_args, **_kwargs): + raise sqlite3.OperationalError("disk I/O error at /secret/prompts.db") + + cases = ( + ( + "list_collections", + "GET", + BASE, + None, + {}, + "Collections could not be read.", + ), + ( + "file_collections", + "GET", + MEMBERSHIP, + None, + {"root_index": "0", "root_scope": self.scope(), "path": "a.png"}, + "Collections could not be read.", + ), + ( + "add_to_collection", + "POST", + ITEMS, + {"collection_id": cid, "items": [self.item("a.png")]}, + {}, + "The collection could not be changed.", + ), + ( + "remove_from_collection", + "DELETE", + ITEMS, + {"collection_id": cid, "items": [self.item("a.png")]}, + {}, + "The collection could not be changed.", + ), + ( + "rename_collection", + "PATCH", + f"{BASE}/{cid}", + {"name": "x"}, + {}, + "The collection could not be changed.", + ), + ( + "delete_collection", + "DELETE", + f"{BASE}/{cid}", + None, + {}, + "The collection could not be changed.", + ), + ) + for method_name, method, url, body, params, message in cases: + with self.subTest(method=method_name): + with patch.object(self.api.db, method_name, side_effect=broken): + data = await self.call(method, url, body, 500, **params) + self.assertEqual(data, {"success": False, "error": message}) + + +class TestCollectionListingFilter(CollectionRoutesTestCase): + """#392's "all N matching" filter carries ``collection`` like the listing.""" + + async def test_a_filter_with_a_collection_resolves_only_members(self): + second = self.two_sources() + self.make("a.png", "b.png") + self.make("c.png", root=second) + made = await self.add( + name="Set", items=[self.item("a.png"), self.item("c.png", 1)] + ) + cid = str(made["collection"]["id"]) + selection = await self.api.resolve_listing_filter({"collection": cid}) + self.assertEqual( + sorted((index, path) for index, _scope, path in selection.targets), + [(0, "a.png"), (1, "c.png")], + ) + selection = await self.api.resolve_listing_filter( + {"collection": cid, "root": "1", "root_scope": self.scope(1)} + ) + self.assertEqual([t[2] for t in selection.targets], ["c.png"]) + for bad in ("0", "x", "1234567890"): + with self.subTest(bad=bad): + with self.assertRaises(ValueError): + from py.api.images import parse_listing_filter + + parse_listing_filter({"collection": bad}) + + async def test_bulk_category_acts_on_collection_members_only(self): + self.make("a.png", "b.png") + inside = self._save_prompt("Inside", category="Old") + outside = self._save_prompt("Outside", category="Old") + self.api.db.link_image_to_prompt(inside, str(self.output_dir / "a.png")) + self.api.db.link_image_to_prompt(outside, str(self.output_dir / "b.png")) + made = await self.add(name="Set", items=[self.item("a.png")]) + cid = str(made["collection"]["id"]) + data = await self.call( + "POST", + "/prompt_manager/bulk/category", + {"images_filter": {"collection": cid}, "category": "New"}, + ) + self.assertEqual((data["updated_count"], data["prompt_count"]), (1, 1)) + self.assertEqual(self.api.db.get_prompt_by_id(inside)["category"], "New") + self.assertEqual(self.api.db.get_prompt_by_id(outside)["category"], "Old") + + async def test_a_deleted_collection_in_a_bulk_filter_is_a_404(self): + self.make("a.png") + data = await self.call( + "POST", + "/prompt_manager/bulk/category", + {"images_filter": {"collection": "77"}, "category": "New"}, + status=404, + ) + self.assertEqual(data["code"], "collection-not-found") + with self.assertRaises(Exception) as caught: + await self.api.resolve_listing_filter({"collection": "77"}) + self.assertEqual(type(caught.exception).__name__, "CollectionMissing") + + +class TestReviewFindings(CollectionRoutesTestCase): + """PR #400 review: loop work, counts snapshot, id and query rules.""" + + async def test_no_realpath_runs_on_the_event_loop(self): + import os + import threading + + names = [f"img{n}.png" for n in range(20)] + self.make(*names) + await self.call("GET", BASE) # warm the listing cache + loop_thread = threading.current_thread() + on_loop = [] + real = os.path.realpath + + def watched(*args, **kwargs): + if threading.current_thread() is loop_thread: + on_loop.append(args[0]) + return real(*args, **kwargs) + + items = [self.item(name) for name in names] + cid = None + with patch("os.path.realpath", side_effect=watched): + made = await self.add(name="Set", items=items) + cid = made["collection"]["id"] + await self.call("DELETE", ITEMS, {"collection_id": cid, "items": items[:5]}) + await self.call( + "GET", + MEMBERSHIP, + root_index="0", + root_scope=items[0]["root_scope"], + path="img9.png", + ) + await self.call("GET", BASE) + self.assertEqual(on_loop, []) + + async def test_missing_is_never_negative(self): + self.make("a.png", "b.png") + made = await self.add( + name="Set", items=[self.item("a.png"), self.item("b.png")] + ) + cid = made["collection"]["id"] + real = self.api.db.collection_count_snapshot + + def raced(root_keys): + members, stored = real(root_keys) + return members, {cid: 1} # a remove landed between two reads + + with patch.object(self.api.db, "collection_count_snapshot", side_effect=raced): + listed = await self.listed() + self.assertEqual((listed[0]["count"], listed[0]["missing"]), (2, 0)) + + async def test_path_ids_follow_the_listing_rule(self): + self.make("a.png") + made = await self.add(name="Set", items=[self.item("a.png")]) + cid = made["collection"]["id"] + for raw in ("0", "007", f"0{cid}"): + with self.subTest(raw=raw): + await self.call("PATCH", f"{BASE}/{raw}", {"name": "x"}, 400) + await self.call("DELETE", f"{BASE}/{raw}", None, 400) + self.assertEqual(len(await self.listed()), 1) + + async def test_membership_refuses_repeated_query_keys(self): + self.make("a.png", "b.png") + response = await self.client.get( + MEMBERSHIP + + f"?root_index=0&root_scope={self.scope()}&path=a.png&path=b.png" + ) + self.assertEqual(response.status, 400) + response = await self.client.get( + MEMBERSHIP + + f"?root_index=0&root_index=0&root_scope={self.scope()}&path=a.png" + ) + self.assertEqual(response.status, 400) + + async def test_deleting_a_collection_leaves_everything_else(self): + from py.api.images import _favorite_root_key + + try: + from tests.test_annotation_schema import insert_group_rows, table_rows + except ImportError: + from test_annotation_schema import insert_group_rows, table_rows + + self.make("a.png") + db = self.api.db + db.set_image_favorite(_favorite_root_key(self.output_dir), "a.png") + prompt = self._save_prompt("Linked") + db.link_image_to_prompt(prompt, str(self.output_dir / "a.png")) + with db.model.get_connection() as conn: + insert_group_rows(conn, locator=str(self.output_dir / "a.png")) + made = await self.add(name="Set", items=[self.item("a.png")]) + tables = ( + "image_favorites", + "generated_images", + "prompts", + "tags", + "media_annotation_files", + "media_annotation_bytes", + "media_image_tags", + ) + before = table_rows(db.model.get_connection(), tables) + file_bytes = (self.output_dir / "a.png").read_bytes() + await self.call("DELETE", f"{BASE}/{made['collection']['id']}") + self.assertEqual(table_rows(db.model.get_connection(), tables), before) + self.assertEqual((self.output_dir / "a.png").read_bytes(), file_bytes) + self.assertTrue(all(before[t] for t in tables)) diff --git a/tests/test_api_duplicate_files.py b/tests/test_api_duplicate_files.py index 9c78ba4a..9350a491 100644 --- a/tests/test_api_duplicate_files.py +++ b/tests/test_api_duplicate_files.py @@ -693,6 +693,39 @@ class TestCleanupJob(DuplicateFilesTestCase): "Moved 2 database links to the kept copies", "\n".join(cleanup.log) ) + async def test_duplicate_removal_moves_collection_memberships(self): + self.trio() + db = self.api.db + root_key = duplicate_files.favorite_root_key(self.output_dir) + grids = db.add_to_collection(name="LoRA grids", items=[(root_key, "b.png")]) + grids = grids["collection"]["id"] + other = db.add_to_collection( + name="Other", items=[(root_key, "sub/c.png"), (root_key, "d.png")] + )["collection"]["id"] + db.add_to_collection(collection_id=grids, items=[(root_key, "sub/c.png")]) + _plan, cleanup = await self.remove_all_but("a.png") + # The union: a.png joins both; c.png's second grids row was a repeat. + self.assertEqual(db.collection_members(grids, root_key), {"a.png"}) + self.assertEqual(db.collection_members(other, root_key), {"a.png", "d.png"}) + self.assertEqual(sorted(db.file_collections(root_key, "a.png")), [grids, other]) + self.assertEqual(db.file_collections(root_key, "b.png"), []) + self.assertEqual(db.file_collections(root_key, "sub/c.png"), []) + self.assertIn( + "Moved 2 collection memberships to the kept copies", "\n".join(cleanup.log) + ) + + def test_one_collection_membership_is_singular(self): + job = type( + "Job", (), {"log": [], "log_line": lambda s, line: s.log.append(line)} + )() + totals = dict.fromkeys( + ("moved", "merged", "favorites", "tags", "review", "collections"), 0 + ) + duplicate_files.DuplicateFilesMixin._log_moved_records( + job, {**totals, "collections": 1} + ) + self.assertEqual(job.log, ["Moved 1 collection membership to the kept copies"]) + async def test_a_failed_removal_leaves_its_links_on_the_kept_copy(self): self.trio() prompt = self.save("Stays on c") @@ -849,8 +882,11 @@ class TestLinkQueries(DuplicateFilesTestCase): with self.subTest(sql=sql): self.assertNotIn("SCAN generated_images", plan) self.assertNotIn("SCAN image_favorites", plan) + self.assertNotIn("SCAN image_collection_items", plan) finally: conn.close() + statements = duplicate_files.query_plan_sql() + self.assertTrue(any("image_collection_items" in sql for sql in statements)) class TestReviewHelpers(DuplicateFilesTestCase): diff --git a/tests/test_api_workspace_images.py b/tests/test_api_workspace_images.py index 604d4ade..cd998722 100644 --- a/tests/test_api_workspace_images.py +++ b/tests/test_api_workspace_images.py @@ -2550,3 +2550,207 @@ class TestListingFilterSelection(ImageAPITestCase): self.assertEqual(response.status, 409) self.assertEqual((await response.json())["code"], "source-changed") + + +class TestWorkspaceCollectionFilter(ImageAPITestCase): + """#331: ``collection=`` keeps only that collection's listed members.""" + + media = TestWorkspaceImages.media + get = TestWorkspaceImages.get + _names = TestWorkspaceImages._names + + async def tearDownAsync(self): + self.api.db.close_all() + assert_closed(self, self._temp_db.name, self._output_tmp.name) + await super().tearDownAsync() + + def keys(self, root, relative): + from py.api.images import _favorite_relative_key, _favorite_root_key + + return (_favorite_root_key(root), _favorite_relative_key(relative)) + + def collect(self, name, *members): + """A collection holding ``(root, relative)`` members; its id.""" + result = self.api.db.add_to_collection( + name=name, items=[self.keys(root, rel) for root, rel in members] + ) + return result["collection"]["id"] + + def two_sources(self): + second = self.comfy_dir / "second" + second.mkdir() + self.api._get_all_output_dirs = lambda: [self.output_dir, second] + return second + + async def test_listing_keeps_only_collection_members(self): + second = self.two_sources() + self.media("a.png", 300) + self.media("b.png", 200) + self.media("sub/c.png", 100, root=second) + self.media("d.png", 50, root=second) + grids = self.collect( + "Grids", + (self.output_dir, "a.png"), + (second, "sub/c.png"), + (self.output_dir, "gone.png"), # stored, never listed + ) + self.collect("Other", (self.output_dir, "b.png")) + response, data = await self.get(collection=str(grids)) + self.assertEqual(response.status, 200, data) + self.assertEqual(self._names(data), ["a.png", "c.png"]) + self.assertEqual(data["total"], 2) + _, data = await self.get(collection=str(grids), root="1") + self.assertEqual(self._names(data), ["c.png"]) + # Without the filter the listing is unchanged. + _, data = await self.get() + self.assertEqual(data["total"], 4) + + async def test_collection_filter_combines_with_the_other_filters(self): + night = self._save_prompt("Neon zebra", category="Night", tags=["a"]) + day = self._save_prompt("Morning field", category="Day", tags=["a"]) + files = { + name: self.media(name, 100 + index) + for index, name in enumerate( + ("night.png", "day.png", "zebra_plain.png", "clip.mp4", "out.png") + ) + } + self.api.db.link_image_to_prompt(night, str(files["night.png"])) + self.api.db.link_image_to_prompt(day, str(files["day.png"])) + members = [(self.output_dir, name) for name in files if name != "out.png"] + grids = str(self.collect("Grids", *members)) + cases = ( + ({}, ["clip.mp4", "day.png", "night.png", "zebra_plain.png"]), + ({"q": "zebra"}, ["night.png", "zebra_plain.png"]), + ({"category": "Night"}, ["night.png"]), + ({"tags": "a"}, ["day.png", "night.png"]), + ({"media": "video"}, ["clip.mp4"]), + ({"media": "image", "q": "day"}, ["day.png"]), + ) + for params, expected in cases: + with self.subTest(**params): + response, data = await self.get(collection=grids, sort="name", **params) + self.assertEqual(response.status, 200, data) + self.assertEqual(self._names(data), expected) + self.assertEqual(data["total"], len(expected)) + + async def test_collection_filter_combines_with_an_images_own_category(self): + """#331 with #332: a member is admitted by its effective category.""" + from py.api.images import image_category_key + + night = self._save_prompt("Neon zebra", category="Night") + files = { + name: self.media(name, 100 + index) + for index, name in enumerate( + ("linked.png", "overridden.png", "own.png", "outside.png") + ) + } + for name in ("linked.png", "overridden.png"): + self.api.db.link_image_to_prompt(night, str(files[name])) + sources = self.api._get_all_output_dirs() + + def own(name, value): + key = image_category_key(sources, files[name]) + self.api.db.set_image_categories([key], value) + + own("overridden.png", "Portraits") + own("own.png", "Night") # unlinked: only its own category + own("outside.png", "Night") # not a member + members = [(self.output_dir, name) for name in files if name != "outside.png"] + grids = str(self.collect("Grids", *members)) + for category, expected in ( + ("Night", ["linked.png", "own.png"]), + ("Portraits", ["overridden.png"]), + ): + with self.subTest(category=category): + response, data = await self.get( + collection=grids, category=category, sort="name" + ) + self.assertEqual(response.status, 200, data) + self.assertEqual(self._names(data), expected) + self.assertEqual(data["total"], len(expected)) + + async def test_invalid_collection_is_a_400(self): + for value in ("x", "0", "-1", "1.5", "1234567890", " 1"): + with self.subTest(value=value): + response, data = await self.get(collection=value) + self.assertEqual(response.status, 400, data) + + async def test_unknown_collection_is_a_404(self): + self.media("a.png") + response, data = await self.get(collection="77") + self.assertEqual(response.status, 404) + self.assertEqual(data["code"], "collection-not-found") + self.assertNotIn("images", data) + self.api._get_all_output_dirs = lambda: [] + response, data = await self.get(collection="77") + self.assertEqual((response.status, data["code"]), (404, "collection-not-found")) + + async def test_a_collection_deleted_during_the_read_is_a_404(self): + self.media("a.png") + grids = self.collect("Grids", (self.output_dir, "a.png")) + real = self.api.db.collection_members + + def deleted_meanwhile(collection_id, root_key): + self.api.db.delete_collection(collection_id) + return real(collection_id, root_key) + + with patch.object( + self.api.db, "collection_members", side_effect=deleted_meanwhile + ): + response, data = await self.get(collection=str(grids)) + self.assertEqual((response.status, data["code"]), (404, "collection-not-found")) + + async def test_a_failed_members_read_is_a_500_never_an_unfiltered_page(self): + self.media("a.png") + self.media("b.png") + grids = self.collect("Grids", (self.output_dir, "a.png")) + + def broken(*_args, **_kwargs): + raise sqlite3.OperationalError("database is locked") + + with patch.object(self.api.db, "collection_members", side_effect=broken): + response, data = await self.get(collection=str(grids)) + self.assertEqual(response.status, 500) + self.assertNotIn("images", data) + self.assertNotIn("locked", json.dumps(data)) + + async def test_reordering_sources_keeps_members(self): + second = self.two_sources() + self.media("a.png", root=second) + self.media("b.png") + grids = str(self.collect("Grids", (second, "a.png"))) + _, data = await self.get(collection=grids) + self.assertEqual( + [(i["root_index"], i["path"]) for i in data["images"]], [(1, "a.png")] + ) + self.api._get_all_output_dirs = lambda: [second, self.output_dir] + _, data = await self.get(collection=grids) + self.assertEqual( + [(i["root_index"], i["path"]) for i in data["images"]], [(0, "a.png")] + ) + + async def test_cursor_from_another_collection_is_refused(self): + for index, name in enumerate(("a.png", "b.png", "c.png")): + self.media(name, 100 + index) + everything = [(self.output_dir, name) for name in ("a.png", "b.png", "c.png")] + first = str(self.collect("First", *everything)) + second = str(self.collect("Second", *everything)) + _, page = await self.get(collection=first, limit=1) + token = page["next_cursor"] + self.assertIsInstance(token, str) + response, _ = await self.get(collection=first, limit=1, cursor=token) + self.assertEqual(response.status, 200) + for params in ({"collection": second}, {}, {"favorites": "1"}): + with self.subTest(**params): + response, _ = await self.get(cursor=token, limit=1, **params) + self.assertEqual(response.status, 400) + + async def test_collection_wins_over_favorites(self): + self.media("a.png") + self.media("b.png") + grids = str(self.collect("Grids", (self.output_dir, "a.png"))) + from py.api.images import _favorite_root_key + + self.api.db.set_image_favorite(_favorite_root_key(self.output_dir), "b.png") + _, data = await self.get(collection=grids, favorites="1") + self.assertEqual(self._names(data), ["a.png"]) diff --git a/tests/test_database_backup.py b/tests/test_database_backup.py index 2437220b..8e08b588 100644 --- a/tests/test_database_backup.py +++ b/tests/test_database_backup.py @@ -614,6 +614,50 @@ class TestRestoreFromFile(BackupTestCase): ) assert_closed(self, str(backups[0])) + def test_backup_and_restore_keep_collections(self): + with self.db.model.get_connection() as conn: + conn.execute( + "INSERT INTO image_collections (id, name, name_key, created_at) " + "VALUES (4, 'LoRa testing', 'lora testing', '2026-10-27T00:00:00Z')" + ) + conn.execute( + "INSERT INTO image_collection_items VALUES " + "(4, 'root', 'grid.png', '2026-10-27T00:00:00Z')" + ) + with tempfile.TemporaryDirectory() as directory: + # The rows still sit in the WAL; a backup must carry them. + backup = os.path.join(directory, "backup.db") + self.assertTrue(self.db.model.backup_database(backup)) + self.assertEqual( + PromptModel.verify_database_file(backup, require_library_state=True), + (True, "ok"), + ) + with self.db.model.get_connection() as conn: + conn.execute("DELETE FROM image_collections") + + self.db.model.restore_from_file(backup) + + conn = self.db.model.get_connection() + self.assertEqual( + [ + tuple(r) + for r in conn.execute("SELECT id, name FROM image_collections") + ], + [(4, "LoRa testing")], + ) + self.assertEqual( + [ + tuple(r) + for r in conn.execute( + "SELECT collection_id, root_key, relative_path " + "FROM image_collection_items" + ) + ], + [(4, "root", "grid.png")], + ) + PromptModel.close_all_instances() + assert_closed(self, directory) + def test_restore_without_a_live_database_returns_empty_backup_path(self): src = self._make_source_db(["fresh"]) self.db.close_all() diff --git a/tests/test_duplicate_image_categories.py b/tests/test_duplicate_image_categories.py index 79452a68..97492454 100644 --- a/tests/test_duplicate_image_categories.py +++ b/tests/test_duplicate_image_categories.py @@ -149,3 +149,36 @@ class TestDuplicateCleanupKeepsCategories(DuplicateFilesTestCase): kept = image_category_key([link], link / "a.png") self.assertFalse((self.output_dir / "b.png").exists()) self.assertEqual(self.api.db.image_category(*kept), "Portraits") + + async def test_a_copy_with_a_collection_and_a_category_moves_both(self): + # #331 + #332: one removed copy carries both records; one cleanup + # moves its membership and copies its category onto the kept copy. + self.trio() + db = self.api.db + root_key = duplicate_files.favorite_root_key(self.output_dir) + b = self.output_dir / "b.png" + grids = db.add_to_collection(name="LoRA grids", items=[(root_key, "b.png")]) + grids = grids["collection"]["id"] + self.set_category(b, "Portraits") + seen = {} + real_log = duplicate_files.DuplicateFilesMixin._log_moved_records + + def log_moved_records(job, totals): + seen.update(totals) + real_log(job, totals) + + with patch.object( + duplicate_files.DuplicateFilesMixin, + "_log_moved_records", + staticmethod(log_moved_records), + ): + cleanup = await self.run_cleanup() + self.assertFalse(b.exists()) + self.assertEqual(db.collection_members(grids, root_key), {"a.png"}) + self.assertEqual(db.file_collections(root_key, "b.png"), []) + self.assertEqual(self.category(self.output_dir / "a.png"), "Portraits") + self.assertEqual(seen["collections"], 1) + self.assertEqual(seen["categories"], 1) + log = "\n".join(cleanup.log) + self.assertIn("Moved 1 collection membership to the kept copies", log) + self.assertIn("Kept 1 image category on the kept copies", log) diff --git a/tests/test_image_collections.py b/tests/test_image_collections.py new file mode 100644 index 00000000..83307b02 --- /dev/null +++ b/tests/test_image_collections.py @@ -0,0 +1,268 @@ +"""Collections (#331): the database operations.""" + +import sqlite3 +import tempfile +import unittest +from pathlib import Path +from unittest.mock import patch + +try: + from tests.open_handles import assert_closed +except ImportError: # discovered with tests/ as the top-level directory + from open_handles import assert_closed + +from database.models import PromptModel +from database.operations import PromptDatabase + +try: # guarded so a missing module fails assertions, not test loading + from database.image_collections import ( + CollectionFilesMissing, + CollectionInvalid, + CollectionMissing, + CollectionNameTaken, + READ_SQL, + ) +except ImportError: + CollectionFilesMissing = CollectionMissing = CollectionNameTaken = None + CollectionInvalid = ValueError + READ_SQL = None + +ROOT = "/real/output" +OTHER = "/real/other" + + +class ImageCollectionOperationsTests(unittest.TestCase): + def setUp(self): + self.temp = tempfile.TemporaryDirectory() + self.addCleanup(self.temp.cleanup) + self.addCleanup(assert_closed, self, self.temp.name) + self.addCleanup(PromptModel.reset_schema_cache) + self.db = PromptDatabase(str(Path(self.temp.name) / "prompts.db")) + self.addCleanup(self.db.close_all) + + def require(self): + self.assertTrue( + hasattr(self.db, "add_to_collection"), + "ImageCollectionOperations is not implemented", + ) + + def add(self, items, **target): + return self.db.add_to_collection(items=items, **target) + + def test_add_creates_a_collection_by_name(self): + self.require() + result = self.add( + [(ROOT, "a.png"), (ROOT, "b.png"), (OTHER, "a.png")], name="LoRa testing" + ) + collection = result["collection"] + self.assertEqual(collection["name"], "LoRa testing") + self.assertEqual( + result, + {"collection": collection, "created": True, "added": 3, "already": 0}, + ) + self.assertEqual(self.db.list_collections(), [collection]) + self.assertTrue(self.db.collection_exists(collection["id"])) + self.assertEqual(self.db.get_collection(collection["id"]), collection) + self.assertEqual( + self.db.collection_members(collection["id"], ROOT), {"a.png", "b.png"} + ) + self.assertEqual( + self.db.root_collection_members(OTHER), {collection["id"]: {"a.png"}} + ) + self.assertEqual(self.db.collection_counts(), {collection["id"]: 3}) + self.assertEqual(self.db.file_collections(ROOT, "a.png"), [collection["id"]]) + self.assertEqual(self.db.file_collections(ROOT, "zzz.png"), []) + + def test_add_by_name_ignores_case_and_unicode_form(self): + self.require() + first = self.add([(ROOT, "a.png")], name="Café grids")["collection"] + # NFD spelling (macOS) in another case joins the same collection. + second = self.add([(ROOT, "b.png")], name="CAFÉ GRIDS") + self.assertEqual(second["collection"], first) + self.assertFalse(second["created"]) + self.assertEqual(len(self.db.list_collections()), 1) + self.assertEqual( + self.db.collection_members(first["id"], ROOT), {"a.png", "b.png"} + ) + + def test_add_counts_already_present(self): + self.require() + made = self.add([(ROOT, "a.png"), (ROOT, "b.png")], name="Set") + result = self.add( + [(ROOT, "b.png"), (ROOT, "c.png"), (ROOT, "c.png")], + collection_id=made["collection"]["id"], + ) + self.assertEqual( + (result["created"], result["added"], result["already"]), (False, 1, 1) + ) + self.assertEqual(self.db.collection_counts(), {made["collection"]["id"]: 3}) + + def test_add_with_a_new_name_and_nothing_inserted_creates_nothing(self): + self.require() + with self.assertRaises(CollectionFilesMissing): + self.add([], name="Empty") + self.assertEqual(self.db.list_collections(), []) + # An existing name with nothing to add is not an error. + made = self.add([(ROOT, "a.png")], name="Kept")["collection"] + self.assertEqual( + self.add([], name="kept"), + {"collection": made, "created": False, "added": 0, "already": 0}, + ) + + def test_add_to_a_deleted_collection_raises_missing(self): + self.require() + made = self.add([(ROOT, "a.png")], name="Gone")["collection"] + self.db.delete_collection(made["id"]) + with self.assertRaises(CollectionMissing): + self.add([(ROOT, "b.png")], collection_id=made["id"]) + with self.assertRaises(CollectionMissing): + self.db.remove_from_collection(made["id"], [(ROOT, "a.png")]) + with self.assertRaises(CollectionMissing): + self.db.rename_collection(made["id"], "Back") + self.assertFalse(self.db.collection_exists(made["id"])) + self.assertIsNone(self.db.get_collection(made["id"])) + + def test_remove_returns_the_count(self): + self.require() + made = self.add([(ROOT, "a.png"), (ROOT, "b.png")], name="Set")["collection"] + result = self.db.remove_from_collection( + made["id"], [(ROOT, "a.png"), (ROOT, "a.png"), (ROOT, "never.png")] + ) + self.assertEqual(result, {"collection": made, "removed": 1}) + self.assertEqual(self.db.collection_members(made["id"], ROOT), {"b.png"}) + # Removing the last member keeps the (now empty) collection. + self.db.remove_from_collection(made["id"], [(ROOT, "b.png")]) + self.assertEqual(self.db.list_collections(), [made]) + self.assertEqual(self.db.collection_counts(), {made["id"]: 0}) + + def test_rename_refuses_a_taken_name(self): + self.require() + first = self.add([(ROOT, "a.png")], name="First")["collection"] + second = self.add([(ROOT, "a.png")], name="Second")["collection"] + with self.assertRaises(CollectionNameTaken) as caught: + self.db.rename_collection(second["id"], " first ") + self.assertEqual(caught.exception.collection_id, first["id"]) + self.assertEqual(self.db.get_collection(second["id"]), second) + + def test_rename_accepts_a_case_variant_of_its_own_name(self): + self.require() + made = self.add([(ROOT, "a.png")], name="lora grids")["collection"] + renamed = self.db.rename_collection(made["id"], "LoRA grids ") + self.assertEqual(renamed, {"id": made["id"], "name": "LoRA grids"}) + self.assertEqual(self.db.list_collections(), [renamed]) + other = self.db.rename_collection(made["id"], "Portraits") + self.assertEqual(other["name"], "Portraits") + + def test_delete_unknown_raises_missing(self): + self.require() + made = self.add([(ROOT, "a.png")], name="Doomed")["collection"] + self.assertTrue(self.db.delete_collection(made["id"])) + self.assertEqual(self.db.root_collection_members(ROOT), {}) + with self.assertRaises(CollectionMissing): + self.db.delete_collection(made["id"]) + with self.assertRaises(CollectionMissing): + self.db.delete_collection(999) + + def test_list_is_in_name_key_order(self): + self.require() + for name in ("beta", "Alpha", "été", "Zeta", "alpha two"): + self.add([(ROOT, "a.png")], name=name) + self.assertEqual( + [row["name"] for row in self.db.list_collections()], + ["Alpha", "alpha two", "beta", "Zeta", "été"], + ) + + def test_names_are_trimmed_and_collapsed(self): + self.require() + made = self.add([(ROOT, "a.png")], name=" LoRa \t testing\n ")["collection"] + self.assertEqual(made["name"], "LoRa testing") + for bad in ("", " ", "x" * 101, "bell\x07", "c1\x81", "del\x7f", 5, None): + with self.subTest(name=bad): + with self.assertRaises(CollectionInvalid): + self.add([(ROOT, "a.png")], name=bad) + self.assertEqual(len(self.db.list_collections()), 1) + + def test_targets_and_items_are_validated(self): + self.require() + made = self.add([(ROOT, "a.png")], name="Set")["collection"] + for target in ( + {}, + {"name": "x", "collection_id": made["id"]}, + {"collection_id": True}, + {"collection_id": 0}, + {"collection_id": "1"}, + ): + with self.subTest(target=target): + with self.assertRaises(CollectionInvalid): + self.add([(ROOT, "a.png")], **target) + for items in ([("root",)], [(ROOT, 5)], "a.png", [(ROOT, "x" * 4097)]): + with self.subTest(items=items): + with self.assertRaises(CollectionInvalid): + self.add(items, collection_id=made["id"]) + + def test_a_failure_after_the_membership_move_rolls_it_back(self): + self.require() + made = self.add([(ROOT, "b.png")], name="Grids")["collection"] + move = { + "removed": [], + "kept": [], + "targets": {}, + "kept_path": "/real/output/a.png", + "favorites": [(ROOT, "b.png", ROOT, "a.png")], + "removed_locator": "/real/output/b.png", + "kept_locator": "/real/output/a.png", + "sha256": "0" * 64, + "kept_size": 1, + "kept_mtime_ns": 1, + } + with patch.object( + type(self.db), "_move_copy_tags", side_effect=RuntimeError("boom") + ): + with self.assertRaises(RuntimeError): + self.db.move_copy_records(move) + self.assertEqual(self.db.file_collections(ROOT, "b.png"), [made["id"]]) + self.assertEqual(self.db.file_collections(ROOT, "a.png"), []) + # Without the failure the same move succeeds. + self.assertEqual(self.db.move_copy_records(move)["collections"], 1) + self.assertEqual(self.db.file_collections(ROOT, "a.png"), [made["id"]]) + + def test_count_snapshot_reads_in_one_transaction(self): + self.require() + made = self.add([(ROOT, "a.png"), (OTHER, "b.png")], name="Set")["collection"] + statements = [] + conn = self.db.model.get_connection() + conn.set_trace_callback(statements.append) + try: + members, stored = self.db.collection_count_snapshot([ROOT, OTHER, ROOT]) + finally: + conn.set_trace_callback(None) + self.assertEqual( + members, {ROOT: {made["id"]: {"a.png"}}, OTHER: {made["id"]: {"b.png"}}} + ) + self.assertEqual(stored, {made["id"]: 2}) + self.assertEqual(statements[0], "BEGIN") + self.assertEqual(statements[-1], "ROLLBACK") + self.assertFalse(conn.in_transaction) + + def test_reads_use_an_index(self): + self.require() + self.add([(ROOT, "a.png")], name="Set") + conn = sqlite3.connect(self.db.model.db_path) + try: + for sql in READ_SQL: + plan = " ".join( + row[3] + for row in conn.execute( + "EXPLAIN QUERY PLAN " + sql, [None] * sql.count("?") + ) + ) + with self.subTest(sql=sql): + self.assertNotIn("SCAN image_collection_items", plan) + if "?" in sql: # a lookup, not the whole list + self.assertNotIn("SCAN image_collections", plan) + finally: + conn.close() + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_image_collections_schema.py b/tests/test_image_collections_schema.py new file mode 100644 index 00000000..df89faf7 --- /dev/null +++ b/tests/test_image_collections_schema.py @@ -0,0 +1,521 @@ +"""Collections schema (#331): install, validation and restore safety.""" + +import re +import sqlite3 +import tempfile +import unittest +import unicodedata +from contextlib import closing +from pathlib import Path +from unittest.mock import patch + +try: + from tests.open_handles import assert_closed + from tests.test_annotation_schema import LEGACY_TABLES, table_rows +except ImportError: # discovered with tests/ as the top-level directory + from open_handles import assert_closed + from test_annotation_schema import LEGACY_TABLES, table_rows + +from database.models import PromptModel + +try: # guarded so a missing module fails assertions, not test loading + from database import image_collections + from database.image_collections import ( + image_collections_schema_error, + migrate_image_collections, + ) +except ImportError: + image_collections = None + image_collections_schema_error = None + migrate_image_collections = None + +COLLECTIONS = "image_collections" +ITEMS = "image_collection_items" +INDEX = "idx_image_collection_items_file" +TABLES = (COLLECTIONS, ITEMS) +DDL = { + COLLECTIONS: ( + f"CREATE TABLE {COLLECTIONS} (id INTEGER PRIMARY KEY AUTOINCREMENT, " + "name TEXT NOT NULL, name_key TEXT NOT NULL UNIQUE, " + "created_at TEXT NOT NULL)" + ), + ITEMS: ( + f"CREATE TABLE {ITEMS} (collection_id INTEGER NOT NULL REFERENCES " + f"{COLLECTIONS}(id) ON DELETE CASCADE, root_key TEXT NOT NULL, " + "relative_path TEXT NOT NULL, added_at TEXT NOT NULL, " + "PRIMARY KEY (collection_id, root_key, relative_path))" + ), +} +INDEX_DDL = f"CREATE INDEX {INDEX} ON {ITEMS}(root_key, relative_path)" +NOW = "2026-10-27T00:00:00.000000+00:00" +IMAGES = 23_000 + + +def _compact(sql): + return re.sub(r"\s+", "", sql or "").lower() + + +def expected_catalog(): + return { + ("table", COLLECTIONS, _compact(DDL[COLLECTIONS])), + ("table", ITEMS, _compact(DDL[ITEMS])), + ("index", INDEX, _compact(INDEX_DDL)), + ("index", f"sqlite_autoindex_{COLLECTIONS}_1", ""), + ("index", f"sqlite_autoindex_{ITEMS}_1", ""), + } + + +def catalog(conn): + return { + (kind, name, _compact(sql)) + for kind, name, sql in conn.execute( + "SELECT type,name,sql FROM sqlite_master " + "WHERE name IN (?,?,?) OR tbl_name IN (?,?)", + (*TABLES, INDEX, *TABLES), + ) + } + + +def key(name): + return unicodedata.normalize("NFC", unicodedata.normalize("NFC", name).casefold()) + + +def schema(conn, exclude=()): + return sorted( + tuple(row) + for row in conn.execute( + "SELECT type,name,tbl_name,sql FROM sqlite_master ORDER BY name" + ) + if row[1] not in exclude and row[2] not in exclude + ) + + +def row_counts(conn): + tables = [ + name + for (name,) in conn.execute( + "SELECT name FROM sqlite_master WHERE type='table' ORDER BY name" + ) + ] + return { + table: conn.execute(f"SELECT COUNT(*) FROM {table}").fetchone()[0] # nosec + for table in tables + } + + +class ImageCollectionsSchemaTests(unittest.TestCase): + def setUp(self): + self.temp = tempfile.TemporaryDirectory() + self.addCleanup(self.temp.cleanup) + self.addCleanup(assert_closed, self, self.temp.name) + self.addCleanup(PromptModel.reset_schema_cache) + self.models = [] + self.addCleanup(self.close_models) + self.path = str(Path(self.temp.name) / "live.db") + + def close_models(self): + for model in self.models: + model.close_all() + + def model(self, path=None): + model = PromptModel(path or self.path) + self.models.append(model) + return model + + def require_module(self): + self.assertIsNotNone( + image_collections_schema_error, + "database.image_collections is not implemented", + ) + + def closed_current(self, name="current.db", *statements): + """A closed current database, altered afterwards by *statements*.""" + path = str(Path(self.temp.name) / name) + model = self.model(path) + model.close_all() + PromptModel.reset_schema_cache() + with closing(sqlite3.connect(path)) as raw, raw: + raw.execute("PRAGMA foreign_keys=OFF") + for statement in statements: + if isinstance(statement, tuple): + raw.execute(*statement) + else: + raw.execute(statement) + return path + + @staticmethod + def drop_group(path): + with closing(sqlite3.connect(path)) as raw, raw: + raw.execute(f"DROP TABLE {ITEMS}") + raw.execute(f"DROP TABLE {COLLECTIONS}") + + def test_migration_creates_tables_and_index(self): + self.require_module() + model = self.model() + with model.get_connection() as conn: + self.assertEqual(catalog(conn), expected_catalog()) + self.assertIsNone(image_collections_schema_error(conn, allow_missing=False)) + self.assertEqual( + conn.execute(f"SELECT COUNT(*) FROM {COLLECTIONS}").fetchone()[0], 0 + ) + self.assertEqual( + conn.execute(f"SELECT COUNT(*) FROM {ITEMS}").fetchone()[0], 0 + ) + self.assertEqual( + PromptModel.verify_database_file(self.path, require_library_state=True), + (True, "ok"), + ) + + def test_migration_is_idempotent(self): + self.require_module() + model = self.model() + with model.get_connection() as conn: + conn.execute( + f"INSERT INTO {COLLECTIONS} (name, name_key, created_at) " + "VALUES ('Kept', 'kept', ?)", + (NOW,), + ) + conn.execute(f"INSERT INTO {ITEMS} VALUES (1, 'root', 'a.png', ?)", (NOW,)) + before = schema(conn) + model.close_all() + for _ in range(2): + PromptModel.reset_schema_cache() + model = self.model() + with model.get_connection() as conn: + migrate_image_collections(conn) + self.assertEqual(schema(conn), before) + self.assertEqual( + [tuple(r) for r in conn.execute(f"SELECT * FROM {ITEMS}")], + [(1, "root", "a.png", NOW)], + ) + model.close_all() + + def test_migration_reads_only_its_own_tables(self): + self.require_module() + model = self.model() + with model.get_connection() as conn: + conn.execute("INSERT INTO prompts(id,text,hash) VALUES(1,'positive','p')") + conn.executemany( + "INSERT INTO generated_images(prompt_id,image_path,filename," + "file_path) VALUES(1,?,?,?)", + ((f"/out/{n}.png", f"{n}.png", f"/out/{n}.png") for n in range(IMAGES)), + ) + conn.execute("INSERT INTO image_favorites VALUES('root','a.png',?)", (NOW,)) + model.close_all() + self.drop_group(self.path) + with closing(sqlite3.connect(self.path)) as raw: + raw.execute("PRAGMA foreign_keys=ON") + counts_before = row_counts(raw) + schema_before = schema(raw) + legacy_before = table_rows(raw, LEGACY_TABLES) + self.assertEqual(counts_before["generated_images"], IMAGES) + touched = [] + + def record(action, arg1, arg2, _db, _trigger): + touched.append((action, arg1, arg2)) + return sqlite3.SQLITE_OK + + raw.set_authorizer(record) + try: + migrate_image_collections(raw) + finally: + raw.set_authorizer(None) + raw.commit() + reads = { + arg1 for action, arg1, _arg2 in touched if action == sqlite3.SQLITE_READ + } + self.assertTrue(reads) + self.assertLessEqual(reads, {"sqlite_master", COLLECTIONS, ITEMS}) + writes = { + arg1 + for action, arg1, _arg2 in touched + if action + in (sqlite3.SQLITE_INSERT, sqlite3.SQLITE_UPDATE, sqlite3.SQLITE_DELETE) + } + self.assertLessEqual(writes, {"sqlite_master"}) + self.assertEqual(catalog(raw), expected_catalog()) + counts_after = row_counts(raw) + self.assertEqual(counts_after.pop(COLLECTIONS), 0) + self.assertEqual(counts_after.pop(ITEMS), 0) + self.assertEqual(counts_after, counts_before) + self.assertEqual(schema(raw, exclude=TABLES), schema_before) + self.assertEqual(table_rows(raw, LEGACY_TABLES), legacy_before) + # A second run is a no-op. + after_first = schema(raw) + changes = raw.total_changes + migrate_image_collections(raw) + raw.commit() + self.assertEqual(schema(raw), after_first) + self.assertEqual(raw.total_changes, changes) + + def test_migration_refuses_a_foreign_object_of_the_same_name(self): + self.require_module() + for statements in ( + (f"CREATE TABLE {COLLECTIONS} (x)",), + (f"CREATE VIEW {COLLECTIONS} AS SELECT 1",), + (f"CREATE INDEX {INDEX} ON prompts(text)",), + ): + with self.subTest(statements=statements): + path = str(Path(self.temp.name) / f"foreign{len(self.models)}.db") + self.model(path).close_all() + PromptModel.reset_schema_cache() + self.drop_group(path) + with closing(sqlite3.connect(path)) as raw, raw: + for statement in statements: + raw.execute(statement) + with closing(sqlite3.connect(path)) as raw: + before = schema(raw) + self.assertIsNotNone(image_collections_schema_error(raw)) + with self.assertRaises(sqlite3.DatabaseError): + self.model(path).get_connection() + PromptModel.close_all_instances() + PromptModel.reset_schema_cache() + with closing(sqlite3.connect(path)) as raw: + self.assertEqual(schema(raw), before) + self.assertIsNone( + raw.execute( + "SELECT 1 FROM sqlite_master WHERE name=?", (ITEMS,) + ).fetchone() + ) + self.assertFalse(PromptModel.verify_database_file(path)[0]) + + def test_migration_rolls_back_on_error(self): + self.require_module() + self.model().close_all() + PromptModel.reset_schema_cache() + self.drop_group(self.path) + with closing(sqlite3.connect(self.path)) as raw: + with patch.object( + image_collections, + "image_collections_schema_error", + side_effect=[None, "Image collections went wrong"], + ): + with self.assertRaisesRegex(sqlite3.DatabaseError, "went wrong"): + migrate_image_collections(raw) + raw.commit() + self.assertEqual(catalog(raw), set()) + + def test_startup_accepts_a_value_saved_under_older_rules(self): + self.require_module() + path = self.closed_current( + "older.db", + ( + f"INSERT INTO {COLLECTIONS} (name, name_key, created_at) " + "VALUES (?, 'not the key', ?)", + (" " + "x" * 150 + " ", NOW), + ), + ) + model = self.model(path) + with model.get_connection() as conn: + self.assertIsNone(image_collections_schema_error(conn, check_values=False)) + self.assertIsNotNone(image_collections_schema_error(conn)) + self.assertEqual( + conn.execute(f"SELECT COUNT(*) FROM {COLLECTIONS}").fetchone()[0], 1 + ) + model.close_all() + self.assertFalse( + PromptModel.verify_database_file(path, require_library_state=True)[0] + ) + + def test_restore_accepts_an_old_backup_without_collections(self): + self.require_module() + path = self.closed_current("old.db") + self.drop_group(path) + with closing(sqlite3.connect(path)) as raw: + self.assertIsNone(image_collections_schema_error(raw)) + self.assertEqual( + image_collections_schema_error(raw, allow_missing=False), + "Missing image collections schema", + ) + self.assertEqual(PromptModel.verify_database_file(path), (True, "ok")) + model = self.model(path) + with model.get_connection() as conn: + self.assertEqual(catalog(conn), expected_catalog()) + model.close_all() + self.assertEqual( + PromptModel.verify_database_file(path, require_library_state=True), + (True, "ok"), + ) + + def test_restore_requires_collections_on_a_current_stage(self): + self.require_module() + path = self.closed_current("stage.db") + self.drop_group(path) + self.assertEqual( + PromptModel.verify_database_file(path, require_library_state=True), + (False, "Missing image collections schema"), + ) + # One table without the other is never acceptable. + partial = self.closed_current("partial.db", f"DROP TABLE {ITEMS}") + self.assertFalse(PromptModel.verify_database_file(partial)[0]) + + def assert_refused(self, name, *statements): + self.require_module() + path = self.closed_current( + f"{name}.db", + ( + f"INSERT INTO {COLLECTIONS} (id, name, name_key, created_at) " + "VALUES (1, 'Good', 'good', ?)", + (NOW,), + ), + (f"INSERT INTO {ITEMS} VALUES (1, 'root', 'a.png', ?)", (NOW,)), + *statements, + ) + with closing(sqlite3.connect(path)) as raw: + self.assertIsNone(image_collections_schema_error(raw, check_values=False)) + error = image_collections_schema_error(raw) + self.assertIsNotNone(error) + self.assertIn("collections", error.lower()) + ok, reason = PromptModel.verify_database_file(path) + self.assertFalse(ok) + self.assertEqual(reason, error) + self.assertFalse( + PromptModel.verify_database_file(path, require_library_state=True)[0] + ) + + def test_a_valid_current_database_is_accepted(self): + self.require_module() + path = self.closed_current( + "valid.db", + ( + f"INSERT INTO {COLLECTIONS} (id, name, name_key, created_at) " + "VALUES (1, ?, ?, ?)", + ("LoRa testing", key("LoRa testing"), NOW), + ), + (f"INSERT INTO {ITEMS} VALUES (1, 'root', ?, ?)", ("é" * 4096, NOW)), + ) + self.assertEqual( + PromptModel.verify_database_file(path, require_library_state=True), + (True, "ok"), + ) + + def test_restore_refuses_a_non_text_name(self): + # Column affinity turns a number into text; a blob stays a blob. + self.assert_refused("blob-name", f"UPDATE {COLLECTIONS} SET name = x'61'") + + def test_restore_refuses_an_empty_name(self): + self.assert_refused("empty", f"UPDATE {COLLECTIONS} SET name = '', name_key=''") + + def test_restore_refuses_a_name_over_100_characters(self): + name = "é" * 101 + self.assert_refused( + "long", + (f"UPDATE {COLLECTIONS} SET name = ?, name_key = ?", (name, key(name))), + ) + + def test_restore_refuses_a_control_character(self): + name = "a\x07b" + self.assert_refused( + "control", + (f"UPDATE {COLLECTIONS} SET name = ?, name_key = ?", (name, key(name))), + ) + + def test_restore_refuses_a_leading_or_trailing_space(self): + for name in (" Good", "Good "): + with self.subTest(name=name): + self.assert_refused( + f"padded{len(name)}{name[0] == ' '}", + ( + f"UPDATE {COLLECTIONS} SET name = ?, name_key = ?", + (name, key(name)), + ), + ) + + def test_restore_accepts_a_name_key_from_other_case_tables(self): + # Python versions fold some characters differently; a key written + # by another version must never make a backup unrestorable. + self.require_module() + path = self.closed_current( + "other-key.db", + ( + f"INSERT INTO {COLLECTIONS} (id, name, name_key, created_at) " + "VALUES (1, 'Good', 'Good', ?)", + (NOW,), + ), + ) + self.assertEqual( + PromptModel.verify_database_file(path, require_library_state=True), + (True, "ok"), + ) + + def test_restore_refuses_an_empty_or_non_text_name_key(self): + self.assert_refused("empty-key", f"UPDATE {COLLECTIONS} SET name_key = ''") + self.assert_refused( + "blob-name-key", f"UPDATE {COLLECTIONS} SET name_key = x'61'" + ) + self.assert_refused( + "long-name-key", + (f"UPDATE {COLLECTIONS} SET name_key = ?", ("k" * 1201,)), + ) + + def test_restore_refuses_a_c1_control_character(self): + for name in ("a\x81b", "a\x7fb"): + with self.subTest(name=repr(name)): + self.assert_refused( + f"c1-{ord(name[1])}", + ( + f"UPDATE {COLLECTIONS} SET name = ?, name_key = ?", + (name, key(name)), + ), + ) + + def test_restore_refuses_a_non_text_key(self): + # Column affinity turns a number into text; a blob stays a blob. + self.assert_refused("blob-key", f"UPDATE {ITEMS} SET relative_path = x'61'") + + def test_restore_refuses_a_key_over_4096_characters(self): + self.assert_refused( + "long-key", (f"UPDATE {ITEMS} SET root_key = ?", ("é" * 4097,)) + ) + + def test_restore_refuses_a_timestamp_over_64_bytes(self): + self.assert_refused( + "stamp", (f"UPDATE {COLLECTIONS} SET created_at = ?", ("t" * 65,)) + ) + self.assert_refused( + "item-stamp", (f"UPDATE {ITEMS} SET added_at = ?", ("t" * 65,)) + ) + + def test_restore_refuses_an_item_of_a_missing_collection(self): + self.assert_refused("orphan", f"INSERT INTO {ITEMS} VALUES (9, 'r', 'b', 'n')") + + def test_restore_refuses_a_non_integer_collection_id(self): + self.assert_refused("text-id", f"UPDATE {ITEMS} SET collection_id = 'one'") + + def test_restore_refuses_a_trigger_or_extra_index(self): + self.require_module() + for name, statement in ( + ( + "trigger", + f"CREATE TRIGGER t AFTER INSERT ON {ITEMS} BEGIN SELECT 1; END", + ), + ("extra", f"CREATE INDEX extra ON {COLLECTIONS}(name)"), + ): + with self.subTest(name=name): + path = self.closed_current(f"{name}.db", statement) + with closing(sqlite3.connect(path)) as raw: + self.assertIsNotNone(image_collections_schema_error(raw)) + self.assertFalse(PromptModel.verify_database_file(path)[0]) + + def test_deleting_a_collection_deletes_its_members(self): + self.require_module() + model = self.model() + with model.get_connection() as conn: + conn.execute( + f"INSERT INTO {COLLECTIONS} (id, name, name_key, created_at) " + "VALUES (1, 'A', 'a', ?), (2, 'B', 'b', ?)", + (NOW, NOW), + ) + conn.executemany( + f"INSERT INTO {ITEMS} VALUES (?, 'root', ?, ?)", + ((1, "a.png", NOW), (1, "b.png", NOW), (2, "a.png", NOW)), + ) + conn.execute(f"DELETE FROM {COLLECTIONS} WHERE id = 1") + self.assertEqual( + [tuple(r) for r in conn.execute(f"SELECT collection_id FROM {ITEMS}")], + [(2,)], + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/tests/test_image_tag_jobs.py b/tests/test_image_tag_jobs.py index 0af74981..f593aaf5 100644 --- a/tests/test_image_tag_jobs.py +++ b/tests/test_image_tag_jobs.py @@ -884,6 +884,16 @@ class TestFilterStart(ImageTagJobTestCase): self.assertEqual((status, data["code"]), (400, "no-images")) self.assertEqual(self.api._jobs.list(), []) + async def test_filter_with_a_deleted_collection_is_a_404_without_a_job(self): + self.png("a.png") + + status, data = await self.start( + {"mode": "add", "filter": {"collection": "77"}, "tags": ["x"]} + ) + + self.assertEqual((status, data["code"]), (404, "collection-not-found")) + self.assertEqual(self.api._jobs.list(), []) + async def test_filter_over_the_cap_is_refused_without_a_job(self): from py.api import image_tag_jobs diff --git a/tests/test_restore_plans.py b/tests/test_restore_plans.py index 5ad20eaa..f5825169 100644 --- a/tests/test_restore_plans.py +++ b/tests/test_restore_plans.py @@ -49,6 +49,7 @@ SUMMARY_KEYS = { "image_links", "runs", "saved_views", + "collections", } diff --git a/tests/test_restore_stage.py b/tests/test_restore_stage.py index 3c863799..72ae806e 100644 --- a/tests/test_restore_stage.py +++ b/tests/test_restore_stage.py @@ -226,6 +226,7 @@ class TestStageAndPublish(StageTestCase): for key in ("pairs", "archived_pairs", "image_links", "runs"): self.assertEqual(staged.summary[key], 0, key) self.assertEqual(staged.summary["saved_views"], 0) + self.assertEqual(staged.summary["collections"], 0) self.assertEqual(staged.schema, {"migrated": False, "library_state": True}) self.assertEqual( staged.digest, @@ -239,6 +240,48 @@ class TestStageAndPublish(StageTestCase): staged.discard() +class TestCollectionsSummary(StageTestCase): + """#331: collections join the review's summary and the live fingerprint.""" + + def add_collection(self, db, name): + with db.model.get_connection() as conn: + conn.execute( + "INSERT INTO image_collections (name, name_key, created_at) " + "VALUES (?, ?, '2026-10-27T00:00:00+00:00')", + (name, name.casefold()), + ) + + def test_summary_counts_collections(self): + src_path = os.path.join(self.uploads, "with-collections.db") + src = PromptDatabase(src_path) + try: + self.save(src, "kept") + self.add_collection(src, "First") + self.add_collection(src, "Second") + finally: + src.close_all() + staged = self.db.model.stage_restore(src_path) + try: + self.assertEqual(staged.summary["collections"], 2) + finally: + staged.discard() + self.assertIn("collections", restore_stage.SUMMARY_KEYS) + + def test_adding_a_collection_changes_the_fingerprint(self): + conn = self.db.model.get_connection() + summary, before = restore_stage.live_fingerprint(conn) + self.assertEqual(summary["collections"], 0) + self.add_collection(self.db, "After the review") + summary, after = restore_stage.live_fingerprint(conn) + self.assertEqual(summary["collections"], 1) + self.assertNotEqual(after, before) + # A delete and a re-add with the same count still changes it (rowid). + with self.db.model.get_connection() as write: + write.execute("DELETE FROM image_collections") + self.add_collection(self.db, "Replacement") + self.assertNotEqual(restore_stage.live_fingerprint(conn)[1], after) + + class TestOlderBackups(StageTestCase): """Rollback-journal and older-schema backups restore exactly as before."""