From 074c36b5c4f7c9cf5cbf7eea5802632ef2261fea Mon Sep 17 00:00:00 2001 From: pikann22 Date: Mon, 20 Jul 2026 18:15:30 +0000 Subject: [PATCH 1/2] feat: enhance route matching to prefer literal segments over wildcards and add regression test --- context.go | 53 ++++++++++++++++++++++++++++++++++++++++-------- context_test.go | 51 ++++++++++++++++++++++++++++++++++++++++++++++ wasm_backends.go | 14 +++++++++++-- wasm_imports.go | 11 ++++++++++ 4 files changed, 119 insertions(+), 10 deletions(-) create mode 100644 context_test.go diff --git a/context.go b/context.go index 6a31daa..9e52071 100644 --- a/context.go +++ b/context.go @@ -77,6 +77,14 @@ func newContext(db DBBackend, kv KVBackend, log LogBackend, cfg ConfigBackend, p } } +// matchRoute finds the registered route for method+path. When more than one +// registered pattern matches the same incoming path — e.g. a plugin +// registers both "/views/:viewId/panels/:panelId" and +// "/views/:viewId/panels/layout" under the same method, and the request is +// for ".../panels/layout" — the most specific pattern wins (the one with +// fewer ":param" segments), so a literal segment like "layout" always beats +// a same-position ":panelId" wildcard. Without this, Go's randomized map +// iteration order would make the match nondeterministic across calls. func (c *Context) matchRoute(method, path string) (RouteHandler, map[string]string, bool) { method = strings.ToUpper(method) if handler, ok := c.routes[routeKey{method, path}]; ok { @@ -85,22 +93,51 @@ func (c *Context) matchRoute(method, path string) (RouteHandler, map[string]stri projectID, relativePath, hasProjectScope := splitProjectPath(path) + var ( + bestHandler RouteHandler + bestParams map[string]string + bestSpecificity = -1 + found bool + ) + consider := func(pattern string, handler RouteHandler, tryPath string, withProjectID bool) { + params, ok := matchRoutePattern(pattern, tryPath) + if !ok { + return + } + specificity := routeSpecificity(pattern) + if specificity <= bestSpecificity { + return + } + if withProjectID { + params["projectId"] = projectID + } + bestHandler, bestParams, bestSpecificity, found = handler, params, specificity, true + } + for key, handler := range c.routes { if key.method != method { continue } - if params, ok := matchRoutePattern(key.path, path); ok { - return handler, params, true - } + consider(key.path, handler, path, false) if hasProjectScope { - if params, ok := matchRoutePattern(key.path, relativePath); ok { - params["projectId"] = projectID - return handler, params, true - } + consider(key.path, handler, relativePath, true) } } - return nil, nil, false + return bestHandler, bestParams, found +} + +// routeSpecificity counts a pattern's literal (non-":param") segments — +// used to break ties when multiple registered patterns match the same +// incoming path. +func routeSpecificity(pattern string) int { + n := 0 + for _, seg := range splitPathSegments(pattern) { + if !strings.HasPrefix(seg, ":") { + n++ + } + } + return n } func splitProjectPath(path string) (projectID, relativePath string, ok bool) { diff --git a/context_test.go b/context_test.go new file mode 100644 index 0000000..0392d9b --- /dev/null +++ b/context_test.go @@ -0,0 +1,51 @@ +package plugin + +import "testing" + +// Regression test for a route-matching ambiguity: when a plugin registers +// both "/views/:viewId/panels/:panelId" and "/views/:viewId/panels/layout" +// under the same method, a request for ".../panels/layout" matches both +// patterns. matchRoute must always prefer the more specific (literal +// "layout") pattern over the wildcard ":panelId" one — previously this was +// resolved by Go's randomized map iteration order, so the "wrong" handler +// (treating "layout" as a panel ID) would win nondeterministically across +// calls. Note: plugintest.Context.Call bypasses this entirely by passing the +// literal ":panelId" pattern string as the path, so it never exercised this +// branch — this test calls matchRoute with a real resolved path instead. +func TestMatchRoute_PrefersLiteralSegmentOverWildcard(t *testing.T) { + ctx := newContext(newWASMDBBackend(), newWASMKVBackend(), newWASMLogBackend(), newWASMConfigBackend(), newWASMPermissionBackend()) + + var gotByPanelID, gotLayout bool + ctx.Route("PATCH", "/views/:viewId/panels/:panelId", func(_ *Request, _ *Response) { + gotByPanelID = true + }) + ctx.Route("PATCH", "/views/:viewId/panels/layout", func(_ *Request, _ *Response) { + gotLayout = true + }) + + for i := 0; i < 50; i++ { + gotByPanelID, gotLayout = false, false + handler, params, ok := ctx.matchRoute("PATCH", "/views/view-abc/panels/layout") + if !ok { + t.Fatalf("iteration %d: expected a route match, got none", i) + } + handler(nil, nil) + if !gotLayout || gotByPanelID { + t.Fatalf("iteration %d: expected the literal /panels/layout route to win, got panelId route instead (params=%v)", i, params) + } + } + + // Sanity check the wildcard route still matches a real panel ID. + handler, params, ok := ctx.matchRoute("PATCH", "/views/view-abc/panels/panel-123") + if !ok { + t.Fatal("expected a route match for a real panel ID") + } + gotByPanelID, gotLayout = false, false + handler(nil, nil) + if !gotByPanelID || gotLayout { + t.Fatalf("expected the :panelId route to win for a non-'layout' panel ID, got params=%v", params) + } + if params["panelId"] != "panel-123" { + t.Fatalf("expected panelId param %q, got %q", "panel-123", params["panelId"]) + } +} diff --git a/wasm_backends.go b/wasm_backends.go index 04fd455..093990d 100644 --- a/wasm_backends.go +++ b/wasm_backends.go @@ -59,14 +59,24 @@ func (b *wasmDBBackend) Query(sql string, params []any) (*DBQueryResult, error) return nil, err } - outputBuf := make([]byte, 8) - hostDBQuery( + // 16 bytes: [0:4]=resultPtr [4:8]=resultLen [8:12]=errPtr [12:16]=errLen. + // db_query2 (unlike the deprecated db_query) reports execution errors + // back through errPtr/errLen instead of silently returning a zero-length + // result indistinguishable from a genuine zero-row success. + outputBuf := make([]byte, 16) + hostDBQuery2( int64(ptrOf(sqlBytes)), int64(len(sqlBytes)), int64(ptrOf(paramsJSON)), int64(len(paramsJSON)), int64(ptrOf(outputBuf)), int64(ptrOf(outputBuf[4:])), + int64(ptrOf(outputBuf[8:])), int64(ptrOf(outputBuf[12:])), ) resPtr := int32(uint32(outputBuf[0]) | uint32(outputBuf[1])<<8 | uint32(outputBuf[2])<<16 | uint32(outputBuf[3])<<24) resLen := int32(uint32(outputBuf[4]) | uint32(outputBuf[5])<<8 | uint32(outputBuf[6])<<16 | uint32(outputBuf[7])<<24) + errPtr := int32(uint32(outputBuf[8]) | uint32(outputBuf[9])<<8 | uint32(outputBuf[10])<<16 | uint32(outputBuf[11])<<24) + errLen := int32(uint32(outputBuf[12]) | uint32(outputBuf[13])<<8 | uint32(outputBuf[14])<<16 | uint32(outputBuf[15])<<24) + if errLen > 0 { + return nil, &hostError{string(wasmSlice(errPtr, errLen))} + } if resLen == 0 { return &DBQueryResult{}, nil } diff --git a/wasm_imports.go b/wasm_imports.go index 69cd153..10e8f09 100644 --- a/wasm_imports.go +++ b/wasm_imports.go @@ -15,10 +15,21 @@ func hostLog(level int32, ptr, length int64) // paca.db_query(sqlPtr i64, sqlLen i64, paramsPtr i64, paramsLen i64, resultPtrPtr i64, resultLenPtr i64) // +// Deprecated: has no error channel, so a query that fails to execute is +// indistinguishable from one that succeeded with zero rows. Use +// hostDBQuery2. Kept only so already-compiled plugin binaries that still +// import this signature keep working against newer hosts. +// //go:wasmimport paca db_query //go:noescape func hostDBQuery(sqlPtr, sqlLen, paramsPtr, paramsLen, resultPtrPtr, resultLenPtr int64) +// paca.db_query2(sqlPtr i64, sqlLen i64, paramsPtr i64, paramsLen i64, resultPtrPtr i64, resultLenPtr i64, errPtrPtr i64, errLenPtr i64) +// +//go:wasmimport paca db_query2 +//go:noescape +func hostDBQuery2(sqlPtr, sqlLen, paramsPtr, paramsLen, resultPtrPtr, resultLenPtr, errPtrPtr, errLenPtr int64) + // paca.db_exec(sqlPtr i64, sqlLen i64, paramsPtr i64, paramsLen i64, rowsAffectedPtr i64, errPtrPtr i64, errLenPtr i64) // //go:wasmimport paca db_exec From f8ccd91070f4fa08cb7c9a746f9762ecbd4ab582 Mon Sep 17 00:00:00 2001 From: pikann22 Date: Mon, 20 Jul 2026 18:30:11 +0000 Subject: [PATCH 2/2] feat: add WASM build job and improve route specificity handling with regression tests --- .github/workflows/ci.yml | 20 +++++++++++++++++ context.go | 13 ++++++++++-- context_test.go | 34 +++++++++++++++++++++++++++++ wasm_backends.go | 5 +---- wasm_output.go | 14 ++++++++++++ wasm_output_test.go | 46 ++++++++++++++++++++++++++++++++++++++++ 6 files changed, 126 insertions(+), 6 deletions(-) create mode 100644 wasm_output.go create mode 100644 wasm_output_test.go diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index cc2db60..56d7001 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -58,6 +58,26 @@ jobs: - name: Verify module compiles run: go build ./... + build-wasm: + name: Build (wasip1) + runs-on: ubuntu-latest + timeout-minutes: 10 + + steps: + - name: Checkout repository + uses: actions/checkout@v4 + with: + fetch-depth: 1 + + - name: Setup Go + uses: actions/setup-go@v5 + with: + go-version: ${{ env.GO_VERSION }} + cache-dependency-path: go.sum + + - name: Verify module compiles for the wasip1 plugin target + run: GOOS=wasip1 GOARCH=wasm go build ./... + test: name: Test runs-on: ubuntu-latest diff --git a/context.go b/context.go index 9e52071..34b0fa1 100644 --- a/context.go +++ b/context.go @@ -129,10 +129,19 @@ func (c *Context) matchRoute(method, path string) (RouteHandler, map[string]stri // routeSpecificity counts a pattern's literal (non-":param") segments — // used to break ties when multiple registered patterns match the same -// incoming path. +// incoming path. Any leading "/projects/:projectId" scope prefix is +// stripped first, so a fully-qualified pattern like +// "/projects/:projectId/tasks/:taskId" and its relative-style equivalent +// "/tasks/:taskId" (matched via splitProjectPath's implicit projectId +// injection) score the same — spelling the prefix out explicitly is a +// registration-style choice, not extra genuine specificity. func routeSpecificity(pattern string) int { + segments := splitPathSegments(pattern) + if len(segments) >= 2 && segments[0] == "projects" && strings.HasPrefix(segments[1], ":") { + segments = segments[2:] + } n := 0 - for _, seg := range splitPathSegments(pattern) { + for _, seg := range segments { if !strings.HasPrefix(seg, ":") { n++ } diff --git a/context_test.go b/context_test.go index 0392d9b..9c17fa5 100644 --- a/context_test.go +++ b/context_test.go @@ -49,3 +49,37 @@ func TestMatchRoute_PrefersLiteralSegmentOverWildcard(t *testing.T) { t.Fatalf("expected panelId param %q, got %q", "panel-123", params["panelId"]) } } + +// Regression test for routeSpecificity treating the "/projects/:projectId" +// scope prefix as extra specificity. A fully-qualified pattern that spells +// the prefix out ("/projects/:projectId/tasks/:taskId") has 2 raw literal +// segments ("projects", "tasks"), while a relative-style literal pattern +// matched via splitProjectPath's implicit projectId injection +// ("/tasks/summary") also has 2 ("tasks", "summary") — but is the genuinely +// more specific match for a request path ending in ".../tasks/summary". +// Before stripping the prefix, both scored 2 and the winner depended on map +// iteration order; after stripping, the wildcard pattern scores 1 and the +// literal one correctly and deterministically wins. +func TestMatchRoute_PrefersLiteralOverWildcardAcrossRegistrationStyles(t *testing.T) { + ctx := newContext(newWASMDBBackend(), newWASMKVBackend(), newWASMLogBackend(), newWASMConfigBackend(), newWASMPermissionBackend()) + + var gotWildcard, gotLiteral bool + ctx.Route("GET", "/projects/:projectId/tasks/:taskId", func(_ *Request, _ *Response) { + gotWildcard = true + }) + ctx.Route("GET", "/tasks/summary", func(_ *Request, _ *Response) { + gotLiteral = true + }) + + for i := 0; i < 50; i++ { + gotWildcard, gotLiteral = false, false + handler, _, ok := ctx.matchRoute("GET", "/projects/proj-1/tasks/summary") + if !ok { + t.Fatalf("iteration %d: expected a route match, got none", i) + } + handler(nil, nil) + if !gotLiteral || gotWildcard { + t.Fatalf("iteration %d: expected the literal /tasks/summary route to win, got wildcard route instead", i) + } + } +} diff --git a/wasm_backends.go b/wasm_backends.go index 093990d..84292d6 100644 --- a/wasm_backends.go +++ b/wasm_backends.go @@ -70,10 +70,7 @@ func (b *wasmDBBackend) Query(sql string, params []any) (*DBQueryResult, error) int64(ptrOf(outputBuf)), int64(ptrOf(outputBuf[4:])), int64(ptrOf(outputBuf[8:])), int64(ptrOf(outputBuf[12:])), ) - resPtr := int32(uint32(outputBuf[0]) | uint32(outputBuf[1])<<8 | uint32(outputBuf[2])<<16 | uint32(outputBuf[3])<<24) - resLen := int32(uint32(outputBuf[4]) | uint32(outputBuf[5])<<8 | uint32(outputBuf[6])<<16 | uint32(outputBuf[7])<<24) - errPtr := int32(uint32(outputBuf[8]) | uint32(outputBuf[9])<<8 | uint32(outputBuf[10])<<16 | uint32(outputBuf[11])<<24) - errLen := int32(uint32(outputBuf[12]) | uint32(outputBuf[13])<<8 | uint32(outputBuf[14])<<16 | uint32(outputBuf[15])<<24) + resPtr, resLen, errPtr, errLen := decodeQuery2Output(outputBuf) if errLen > 0 { return nil, &hostError{string(wasmSlice(errPtr, errLen))} } diff --git a/wasm_output.go b/wasm_output.go new file mode 100644 index 0000000..f4210e7 --- /dev/null +++ b/wasm_output.go @@ -0,0 +1,14 @@ +package plugin + +// decodeQuery2Output parses the 16-byte output buffer written by the +// paca.db_query2 host import: [0:4]=resultPtr [4:8]=resultLen +// [8:12]=errPtr [12:16]=errLen. Kept in a non-wasip1-tagged file so it can +// be unit tested on the host build; wasm_backends.go (wasip1-only) is the +// real caller. +func decodeQuery2Output(buf []byte) (resPtr, resLen, errPtr, errLen int32) { + resPtr = int32(uint32(buf[0]) | uint32(buf[1])<<8 | uint32(buf[2])<<16 | uint32(buf[3])<<24) + resLen = int32(uint32(buf[4]) | uint32(buf[5])<<8 | uint32(buf[6])<<16 | uint32(buf[7])<<24) + errPtr = int32(uint32(buf[8]) | uint32(buf[9])<<8 | uint32(buf[10])<<16 | uint32(buf[11])<<24) + errLen = int32(uint32(buf[12]) | uint32(buf[13])<<8 | uint32(buf[14])<<16 | uint32(buf[15])<<24) + return +} diff --git a/wasm_output_test.go b/wasm_output_test.go new file mode 100644 index 0000000..1073712 --- /dev/null +++ b/wasm_output_test.go @@ -0,0 +1,46 @@ +package plugin + +import "testing" + +func TestDecodeQuery2Output(t *testing.T) { + tests := []struct { + name string + buf []byte + wantResPtr, wantResLen, wantErrPtr, wantErrLen int32 + }{ + { + name: "success with rows", + buf: []byte{0x10, 0x00, 0x00, 0x00, 0x20, 0x00, 0x00, 0x00, 0, 0, 0, 0, 0, 0, 0, 0}, + wantResPtr: 0x10, + wantResLen: 0x20, + wantErrPtr: 0, + wantErrLen: 0, + }, + { + name: "success with zero rows", + buf: make([]byte, 16), + wantResPtr: 0, + wantResLen: 0, + wantErrPtr: 0, + wantErrLen: 0, + }, + { + name: "execution error", + buf: []byte{0, 0, 0, 0, 0, 0, 0, 0, 0x40, 0x00, 0x00, 0x00, 0x05, 0x00, 0x00, 0x00}, + wantResPtr: 0, + wantResLen: 0, + wantErrPtr: 0x40, + wantErrLen: 0x05, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + resPtr, resLen, errPtr, errLen := decodeQuery2Output(tt.buf) + if resPtr != tt.wantResPtr || resLen != tt.wantResLen || errPtr != tt.wantErrPtr || errLen != tt.wantErrLen { + t.Fatalf("decodeQuery2Output(%v) = (%d, %d, %d, %d), want (%d, %d, %d, %d)", + tt.buf, resPtr, resLen, errPtr, errLen, tt.wantResPtr, tt.wantResLen, tt.wantErrPtr, tt.wantErrLen) + } + }) + } +}