feat: stable virtual printer ids — optional explicit id + safe slug — closes #42#43
Merged
Conversation
Virtual ids were derived from the display name with a minimal sanitizer (spaces/parens only) and no uniqueness check, so: distinct names could collide onto one id (shared settings, second printer shadowed), and path-hostile chars like "/" survived into the /api/printers/<id>/settings route. The id also changed whenever the name was edited. - Add an optional explicit `id` to VIRTUAL_PRINTERS entries — a stable, URL-safe identity decoupled from the display name (the virtual analog of #40's serial). Renaming the name no longer orphans saved settings. - When absent, slug the name to a path-safe id (collapse non-word runs to "_"; keep Unicode letters, which %-encode fine; fall back to "printer"). - Validate at config load: reject a non-URL-safe explicit id, and skip an entry whose id collides with an earlier one (warn on both). - Funnel all VirtualPrinter construction through VirtualPrinter.from_config so the resolved id is consistent across listing, lookup, and printing. No version bump — stacked under the per-printer-settings work (#39), ships in the single v1.8.0 that PR cuts. Closes #42. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds stable, URL-safe virtual printer identities to prevent id collisions, path-hostile characters in printer ids, and settings orphaning when display names change.
Changes:
- Introduces optional explicit virtual printer
id(suffix) plus a safer slug fallback for name-derived ids. - Validates virtual printer ids at config load time (charset check + collision detection) and skips invalid/colliding entries with warnings.
- Centralizes virtual printer construction via
VirtualPrinter.from_config()and updates relevant code paths + tests/docs.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| server/virtual_printer.py | Adds slug/validation helpers, optional explicit id support, and from_config() constructor. |
| server/config.py | Validates explicit ids and guards against computed-id collisions during config load. |
| server/printer_service.py | Uses VirtualPrinter.from_config() to ensure consistent id resolution across listing/lookup/printing. |
| server/tests/test_virtual_printer.py | Updates id behavior tests (slugging rules, explicit id, from_config). |
| server/tests/test_config.py | Adds coverage for explicit id retention, invalid id skipping, and collision handling. |
| README.md | Documents optional id field and collision behavior for VIRTUAL_PRINTERS. |
| docs/ARCHITECTURE.md | Updates identity model documentation for virtual printers (explicit id + slug fallback). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
get_virtual_printers now computes the id (re.sub over the name) during
load, so a config like {"name": 123} would raise TypeError and crash
loading instead of skipping the bad entry (only JSONDecodeError was
caught). Validate name/path are strings up front and warn+skip, matching
the existing invalid-entry handling.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
re's `$` matches before a trailing newline, so is_valid_virtual_id accepted ids like "office\n" — which would carry a newline into the settings-route path. Use fullmatch so the whole string must be URL-safe. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #42. Stacked under #39 (per-printer settings); no version bump — ships in the single v1.8.0 that #39 cuts.
Why
Virtual ids were derived from the display name with a minimal sanitizer (spaces/parens only) and no uniqueness check (
virtual_printer.py,config.py):/) survived into the/api/printers/<path:printer_id>/settingsroute;What
idinVIRTUAL_PRINTERSentries →virtual:<id>: a stable, URL-safe identity decoupled from the display name (the virtual analog of Key per-printer settings by serial number, not the unstable USB bus/address id #40's serial). Renamingnameno longer orphans settings.id: collapse non-word runs to_, keep Unicode letters (they%-encode fine), fall back toprinterfor an empty result.id([A-Za-z0-9_-]); skip an entry whose id collides with an earlier one — both warn.VirtualPrinter.from_config: single construction path, so the resolved id is consistent across listing, lookup, and printing (replaces 4 duplicated constructions).Compatibility
Existing
id-less configs keep working via the slug. The slug differs from the old sanitizer only for path-hostile names (/,., emoji) — acceptable, since per-printer settings (#20/#39) aren't released yet, so nothing durable re-keys. Existing tests asserting the old incidental behavior (test_slashes_preservedetc.) updated to the new contract.Tests
virtual_printer(slug cases, explicit id,from_config),config(explicit id kept, invalid skipped, derived + explicit collisions skipped). Full backend suite green (231); no import cycle from the newconfig → virtual_printerdependency (smoke imports all modules).🤖 Generated with Claude Code