Skip to content

fix(api): share opaque path decoding for sensor IDs - #858

Merged
tadelv merged 2 commits into
decentespresso:mainfrom
MaxRink:fix/sensor-encoded-path-id
Sep 16, 2026
Merged

tadelv merged 2 commits into
decentespresso:mainfrom
MaxRink:fix/sensor-encoded-path-id

Conversation

@MaxRink

@MaxRink MaxRink commented Sep 13, 2026

Copy link
Copy Markdown
Contributor

Summary

Fix sensor route lookup for plugin-owned IDs containing URI-reserved characters.
REST manifest, REST command execution, and WebSocket snapshot/rebind paths now
percent-decode the captured sensor ID exactly once before lookup. This restores
the encoded two-instance E64 runtime path that currently returns 404.

No new API route is added. The shared decodeOpaquePathComponent helper is the boundary for these sensor routes and is available for reuse by the separate #833 scale API.

Linked Issue

Fixes #857, including its full compatibility matrix.

Consumer context: #848

Issue #857 now records the reworked decode-once design; this draft implements its sensor-only starting scope. Combined publication validation remains pending.

Verification

  • Combined Linux Flutter gate for the current publication stack: 4,315 passed, 1 skipped, 0 failed; analyzer clean (48.0s). Source was unchanged during the gate.

  • The encoded two-fixture E64 runtime passed REST info and all four reads for both instances, independent WebSocket snapshots, and left fixture B responsive after fixture A disconnected.

  • The encoded two-fixture E64 runtime passed REST info and all four reads for both instances, independent WebSocket snapshots, and left fixture B responsive after fixture A disconnected.

Impact

Clients encode each sensor ID once when placing it in a REST or WebSocket path component; the host decodes that component exactly once. Supported literal colon-composed plugin IDs continue working without client changes, and %252F identifies a literal %2F, never /.

Public plugin, driver, and instance-ID validation excludes percent-bearing components, so synthetic raw-percent identities do not justify a fallback, migration, or widened registration contract. UUID resources and query handlers remain outside this path convention; no #860 query-filter change is included. Unknown IDs retain existing errors, and invalid UTF-8 remains HTTP 400 at the HTTP boundary.

Contributor Responsibility

AI-assisted development is allowed. The submitter remains responsible for the submitted work.

  • I have reviewed and understand all changes in this PR and take responsibility for their correctness, security, behavior, licensing, and provenance, including any AI-assisted or AI-generated work.

tadelv commented Sep 14, 2026

Copy link
Copy Markdown
Member

This looks like the right fix for #857, but I would like us to use it to establish a repo-wide convention for opaque IDs in route path components, rather than leave each device API to rediscover the encoding rules independently.

Proposed convention:

  • Device/resource IDs are opaque application strings. Their contents must not be interpreted by the API layer.
  • When an opaque ID is placed in a REST or WebSocket path component, clients percent-encode that component exactly once.
  • The route boundary percent-decodes the captured component exactly once before lookup.
  • Lookup, connection/session tracking, replacement/rebind logic, etc. always use the decoded opaque ID.
  • Invalid UTF-8/path encoding is rejected consistently at the HTTP boundary.

Could we make the decode-at-boundary behavior a small shared helper/convention and document it somewhere appropriate for future APIs, rather than having new {id} routes directly add their own Uri.decodeComponent(...) calls? We are likely to need the same contract shortly for generic per-device scale endpoints, and probably other device types later.

I would also like compatibility pinned explicitly. Existing path-safe IDs, including the currently common unencoded : form, should continue to resolve exactly as before. A literal % is the interesting case: the canonical client representation should of course be %25, but please add/retain enough regression coverage to establish whether a raw % form was previously accepted and ensure we do not silently reinterpret an existing valid request. If raw literal % was never a valid/accepted route representation, documenting that is sufficient.

The important invariant for me is that adopting this convention must not require changing existing clients whose current IDs already work; it should only make previously unaddressable opaque IDs addressable in a well-defined way.

tadelv commented Sep 14, 2026

Copy link
Copy Markdown
Member

Follow-up to my earlier request for a repo-wide convention: I want to narrow the scope to IDs whose contract is actually opaque/external, rather than mechanically applying it to every {id} route.

In particular, bean IDs are host-assigned UUIDs today: Bean.create() generates them, the create API does not accept an ID, updates preserve the existing ID, and the REST schema documents the ID as a UUID. I have therefore closed #860 as out-of-contract/unnecessary work; its reproduction requires injecting a custom percent-bearing bean ID that normal Decaid creation cannot produce.

So please don't use #860 as a reason to retrofit bean/bean-batch/shot filtering for arbitrary custom bean IDs. The shared decode-once convention remains useful for sensor IDs (and future device/resource IDs that are genuinely opaque and may contain reserved characters), which is the useful scope of this PR.

@MaxRink MaxRink changed the title fix(api): resolve encoded sensor IDs in REST and WebSocket routes fix(api): share opaque path decoding for sensor IDs Sep 14, 2026
@tadelv

tadelv commented Sep 15, 2026

Copy link
Copy Markdown
Member

@MaxRink can you bring this PR branch up to date with origin/main (preferably rebase if possible, no worry about --force)

@tadelv
tadelv requested review from ODevStudio and tadelv September 15, 2026 17:31
@MaxRink
MaxRink force-pushed the fix/sensor-encoded-path-id branch from 673b60e to d15e234 Compare September 15, 2026 17:46
@MaxRink
MaxRink marked this pull request as ready for review September 15, 2026 18:16
tadelv
tadelv previously requested changes Sep 16, 2026

@tadelv tadelv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The decode-once implementation itself looks appropriately scoped: it only changes the three sensor route boundaries, keeps the decoded identity through WS lookup/rebind, leaves UUID/query/proxy/file/token contracts alone, and the API/spec/agent guidance is updated consistently.

There is one merge-blocking repository-rule issue: the current PR body uses Fixes [#857](...), but the contribution-policy gate only accepts forms such as Fixes #857 / Closes #857 / Related #857. As a result, the current head's Contribution policy check fails and the analyze/test/build jobs are skipped. This violates the required PR-template / accepted-issue gate in AGENTS.md and CONTRIBUTING.md, and it also means we do not have CI verification for this head yet.

Please change the linked-issue line to Fixes #857 (the issue is open and currently carries ready-for-human) and trigger a fresh PR check run/event so the real CI gates execute. Once those are green, I don't see another blocking blast-radius concern in this patch.

@@ -0,0 +1,24 @@
# Opaque external sensor route boundary

Issue #858 fixes external plugin sensor IDs containing reserved characters.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This should reference issue #857, not #858. #858 is this PR; #857 is the accepted issue defining the decode-once boundary and compatibility contract. Please correct the archived rationale so the retained design history points to the canonical issue.

@tadelv
tadelv dismissed their stale review September 16, 2026 06:44

The merge-blocking PR-body issue has been fixed (Fixes #857). Leaving the rationale typo comment as non-blocking; CI can now be rerun before approval.

@tadelv tadelv closed this Sep 16, 2026
@tadelv tadelv reopened this Sep 16, 2026
@tadelv
tadelv merged commit d786006 into decentespresso:main Sep 16, 2026
8 of 10 checks passed
@tadelv tadelv added this to the 0.8.7 milestone Sep 16, 2026 — with ChatGPT Codex Connector
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(api): use a shared decode-once boundary for opaque resource IDs

2 participants