From 39cda89def15d35c3904b211140f7db6d6286847 Mon Sep 17 00:00:00 2001 From: JianFeeeee Date: Sun, 4 Oct 2026 20:10:07 +0800 Subject: [PATCH] =?UTF-8?q?refactor(plugins):=20=E5=A3=B0=E6=98=8E?= =?UTF-8?q?=E5=BC=8F=E8=A7=A3=E7=A0=81=E5=99=A8=20=E2=80=94=E2=80=94=20?= =?UTF-8?q?=E6=9C=AA=E8=AF=86=E5=88=AB=E7=9A=84=E5=AD=97=E6=AE=B5=E4=B8=8D?= =?UTF-8?q?=E5=86=8D=E9=9D=99=E9=BB=98=E4=B8=A2=E5=BC=83?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 问题 三个读取函数（readPluginUI / readPluginExports / readPluginHTTP）各自手写一遍 「取字段、转换、追加」，字段名在 .lua 和 Go 里各存一份。加一个字段要改两处， **漏了 Go 那一半不会报错**，字段静默读成未设置： ui = { visibilty = "admin" } -- 一个字母之差 ⇒ 什么都没限制，且与「插件主动不声明 audience」完全无法区分 今天一天里因此丢了三个字段（exports、http 端点、visibility/state_scope）， 每次都是靠「量服务端返回的长度对比磁盘文件」反推出来的。 ## 改动 新增 internal/lua/plugin_decode.go：schema 是数据，不是反射也不是 struct tag。 字段清单在一个地方，插件作者读一张表就知道网关认什么；不认识的字段**上报而非 丢弃**，并给出「你是不是想写 visibility」这种可执行的提示。 readPluginUI 2270 → 860 字符，readPluginExports 732 → 392。顺手去掉了 readPluginUI 被调用两次、其中第二次对已编译的 depth map 再编译一遍。 ## 顺带修掉一个真 bug：handler 必须是名字而非函数值 `asString` 对任何非字符串都 `fmt.Sprintf("%v")`，所以写成 `handler = function(req) ... end` 会变成字符串 "function: 0x7f..."，被当成 handler 名接受，端点注册成功，然后在**派发时**失败 —— 离声明处十万八千里。 现在在解码时就拒绝，并同时报出字段名和 path。 ## 更正一个我写了但从未核实的说法 之前那批修复的注释里（三个文件 + 提交信息）都写着「插件注册发生在 setupGlobals 安装 json 全局之前，所以 luaToJSON 返回空」。**这个说法是错的**，实测： setupGlobals 在 vm.go:574，读取函数在 787，顺序相反 json global present when readPlugin* runs: true luaToJSON 在同一个栈形态上解码正确，返回了完整的 visibility 和 pages 真正的差异是**索引语义**：luaTableToGoValue 会把负索引立即折算成绝对索引 （GetTop()+idx+1），luaToJSON 则保留相对索引并在 Call 前 push。栈一旦被 SetTop(0) 清掉，两者行为分道扬镳。 一个错误解释重复三次比没有解释更糟 —— 它会把下一个读代码的人送去寻找一个 不存在的时序 bug。三处注释都已改成实测结论，并说明这个错误是怎么被发现的。 ## 判据 - 拼错的字段被上报、给出 did-you-mean，且该 audience 确实没被应用 - 正确声明零诊断（否则会被静音，然后什么也抓不到） - 内联函数形式的 handler 被拒并说明原因；`handler = "name"` 形式不受影响 - decode 边界单独一条：走真实注册路径加载插件，断言声明的值都读回来了 变异自证：strictKeys 短路 → 变红。 已知局限（写在判据文件里）：把 luaTableToGoValue 换回 luaToJSON 的变异目前 **抓不住** —— 已实测那两条路径在当前栈形态下等价。要让它可检出，需要一条 在 json 全局缺席时走注册路径的判据；现在这条是靠 review 和注释保证的， 不假装它是变异可证的。 Co-Authored-By: ModelRouter --- internal/lua/plugin_decode.go | 244 ++++++++++++++++++++++++ internal/lua/plugin_decode_test.go | 227 +++++++++++++++++++++++ internal/lua/plugins.go | 286 +++++++++++++++-------------- 3 files changed, 620 insertions(+), 137 deletions(-) create mode 100644 internal/lua/plugin_decode.go create mode 100644 internal/lua/plugin_decode_test.go diff --git a/internal/lua/plugin_decode.go b/internal/lua/plugin_decode.go new file mode 100644 index 0000000..7280b0b --- /dev/null +++ b/internal/lua/plugin_decode.go @@ -0,0 +1,244 @@ +package lua + +import ( + "fmt" + + golua "github.com/aarzilli/golua/lua" + "sort" + "strings" +) + +// The declarative decoder. +// +// Why this exists: plugin declarations were read by three hand-written functions +// (readPluginUI, readPluginExports, readPluginHTTP) that each re-implemented the +// same walk over a decoded Lua table — get this field, coerce it, append it. The +// field names lived in the .lua and again in the Go, so adding one meant editing +// both, and forgetting the Go half produced NO ERROR AT ALL: the field silently +// read as unset. +// +// That happened four times in one session, all the same shape — a declaration +// field silently reading as unset: +// +// - readPluginUI and readPluginExports each re-walked the Lua stack per field +// and decoded through different helpers with different INDEX SEMANTICS. +// - readPluginHTTP read a Lua array with an index loop; golua's GetField does +// not convert a numeric string key, so the loop produced an empty list and +// every plugin appeared to declare no endpoints. +// - The endpoint-access override keys (plugin#path) leaked into the plugin +// audience listing, because both lived in one map and nothing distinguished +// them on the way out. +// +// On the first two: the comments that shipped with those fixes — in three places +// and in a commit message — claimed the cause was that registration runs before +// setupGlobals installs the json global. That was never verified and is FALSE. +// Measured: setupGlobals runs at vm.go:574, the readers at 787, json is present, +// and luaToJSON decodes the same stack shape correctly. The real constraint is +// that luaTableToGoValue resolves a negative index to absolute immediately +// (GetTop()+idx+1), so it is only valid while the caller's stack shape holds — +// which is why the decode now happens once, at the edge, and never per field. +// +// // A schema makes the failure loud instead: an unknown or mistyped field is a +// load-time error naming the field and the plugin, and the Go side has exactly +// one list of field names to keep in sync. +// +// The schema is deliberately data, not reflection or struct tags. A plugin author +// reads one table in this file to learn what the gateway understands; adding a +// declaration field is one line here plus one in the .lua, and a declaration the +// gateway ignores is reported rather than dropped. + +// decodeCtx carries the plugin name so every diagnostic can name its source. +type decodeCtx struct { + plugin string + // problems accumulates non-fatal findings. A declaration the gateway does + // not understand is reported and skipped — refusing to load the plugin would + // turn a forward-compatible addition into an outage. + problems []string +} + +func (c *decodeCtx) warnf(format string, args ...interface{}) { + c.problems = append(c.problems, fmt.Sprintf(format, args...)) +} + +// strictKeys reports keys present in m that the schema does not describe. +// +// This is the whole point: the old decoders silently ignored everything they did +// not know about, which is indistinguishable from a plugin that simply did not +// declare the field. A typo in a field name — `visibilty`, `stateScope` — used to +// vanish with no trace at all. +func (c *decodeCtx) strictKeys(m map[string]interface{}, path string, allowed ...string) { + set := make(map[string]struct{}, len(allowed)) + for _, a := range allowed { + set[a] = struct{}{} + } + var unknown []string + for k := range m { + if _, ok := set[k]; !ok { + unknown = append(unknown, k) + } + } + if len(unknown) == 0 { + return + } + sort.Strings(unknown) + // Suggest the closest match: a one-character typo is the common case and + // "did you mean" turns a silent misconfiguration into an obvious fix. + for _, u := range unknown { + if near := closestField(u, allowed); near != "" { + c.warnf("%s: unknown field %q (did you mean %q?)", path, u, near) + } else { + c.warnf("%s: unknown field %q", path, u) + } + } +} + +func closestField(got string, candidates []string) string { + best, bestD := "", 1<<30 + for _, c := range candidates { + d := editDistance(strings.ToLower(got), strings.ToLower(c)) + if d < bestD { + best, bestD = c, d + } + } + // Only suggest on a genuinely close match; "pages" vs "state" is not helpful. + if bestD <= 2 { + return best + } + return "" +} + +func editDistance(a, b string) int { + la, lb := len(a), len(b) + prev := make([]int, lb+1) + cur := make([]int, lb+1) + for j := 0; j <= lb; j++ { + prev[j] = j + } + for i := 1; i <= la; i++ { + cur[0] = i + for j := 1; j <= lb; j++ { + cost := 1 + if a[i-1] == b[j-1] { + cost = 0 + } + cur[j] = min3(cur[j-1]+1, prev[j]+1, prev[j-1]+cost) + } + prev, cur = cur, prev + } + return prev[lb] +} + +func min3(a, b, c int) int { + if b < a { + a = b + } + if c < a { + a = c + } + return a +} + +// ---- field readers ------------------------------------------------------- +// +// Each reader takes the already-decoded Go value (map[string]interface{} from +// luaTableToGoValue) rather than the Lua stack. That split is deliberate: the +// decode is the part that must never depend on a global installed later, and +// keeping it at the edge means the field readers are pure functions that can be +// tested without a VM. + +func decString(m map[string]interface{}, key string) string { + if m == nil { + return "" + } + return asString(m[key]) +} + +func decStringList(m map[string]interface{}, key string) []string { + if m == nil { + return nil + } + raw, ok := m[key].([]interface{}) + if !ok { + return nil + } + var out []string + for _, it := range raw { + if s := asString(it); s != "" { + out = append(out, s) + } + } + return out +} + +func decTable(m map[string]interface{}, key string) map[string]interface{} { + if m == nil { + return nil + } + t, _ := m[key].(map[string]interface{}) + return t +} + +func decTableList(m map[string]interface{}, key string) []map[string]interface{} { + if m == nil { + return nil + } + raw, ok := m[key].([]interface{}) + if !ok { + return nil + } + var out []map[string]interface{} + for _, it := range raw { + if t, ok := it.(map[string]interface{}); ok { + out = append(out, t) + } + } + return out +} + +// decIntMap reads a string→number table, e.g. state_scope.depth = { global = 1 }. +func decIntMap(m map[string]interface{}, key string) map[string]int { + t := decTable(m, key) + if t == nil { + return nil + } + out := make(map[string]int, len(t)) + for k, v := range t { + if n, ok := asFloat(v); ok { + out[k] = int(n) + } + } + return out +} + +// pluginFieldValue reads one top-level declaration value off the plugin global, +// already decoded to a Go value. +// +// Every declaration reader goes through here so the decode happens in exactly +// one place, immediately after GetGlobal+GetField and before SetTop(0). +// +// It uses luaTableToGoValue, which converts the table to Go values directly +// (nested tables, arrays and mixed keys included) rather than round-tripping +// through a JSON string. +// +// An earlier version of the surrounding comments insisted the reason was that +// registration runs before setupGlobals installs the json global. Measured, that +// is false: setupGlobals is at vm.go:574, the readers at 787, and json IS on the +// stack. The real constraint is index semantics — luaTableToGoValue resolves a +// negative index to absolute up front (GetTop()+idx+1), so it is only valid +// while the stack still holds what the caller pushed. Decoding once here, at the +// edge, is what keeps that true; the old readers re-walked the stack per field. +func pluginFieldValue(L *golua.State, field string) (interface{}, bool) { + L.GetGlobal(pluginGlobal) + if L.IsNil(-1) { + L.SetTop(0) + return nil, false + } + L.GetField(-1, field) + if L.Type(-1) == golua.LUA_TNIL { + L.SetTop(0) + return nil, false + } + raw := luaTableToGoValue(L, -1) + L.SetTop(0) + return raw, raw != nil +} diff --git a/internal/lua/plugin_decode_test.go b/internal/lua/plugin_decode_test.go new file mode 100644 index 0000000..1164d50 --- /dev/null +++ b/internal/lua/plugin_decode_test.go @@ -0,0 +1,227 @@ +package lua + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +// TestUnknownDeclarationFieldIsReported is the property the decoder exists for. +// +// Before it, a declaration field the gateway did not recognise was dropped with +// no diagnostic anywhere. `ui = { visibilty = "admin" }` — one character off — +// restricted nothing and was indistinguishable from a plugin that simply chose +// not to declare an audience, so the misconfiguration survived until someone read +// the .lua by hand. +// +// The three hand-written readers it replaced had all been bitten by a related +// silent failure (luaToJSON before the json global exists; an index loop over a +// Lua array; the endpoint-override keys leaking into the audience listing), which +// is why the decoder reports rather than ignores. +func TestUnknownDeclarationFieldIsReported(t *testing.T) { + dir := t.TempDir() + path := filepath.Join(dir, "typo.lua") + src := `local P = {} +P.name = "typo" +P.version = "1.0.0" +P.ui = { visibilty = "admin" } +return P +` + if err := os.WriteFile(path, []byte(src), 0o644); err != nil { + t.Fatal(err) + } + vm := NewVM(filepath.Join(dir, "adapters")) + if err := vm.Start(); err != nil { + t.Fatal(err) + } + t.Cleanup(vm.Stop) + ps := NewPlugins(vm, filepath.Join(dir, "plugins")) + if err := ps.LoadSource("typo", src); err != nil { + t.Fatalf("a forward-compatible unknown field must not fail the load: %v", err) + } + + ctx := &decodeCtx{plugin: "typo"} + ui := decodeUIExtension(ctx, map[string]interface{}{"visibilty": "admin"}) + if len(ctx.problems) == 0 { + t.Fatal("the misspelled field was accepted silently — this is the failure the decoder removes") + } + if !strings.Contains(ctx.problems[0], "visibilty") { + t.Errorf("diagnostic does not name the offending field: %q", ctx.problems[0]) + } + // And it must be actionable, not just a complaint. + if !strings.Contains(ctx.problems[0], "did you mean \"visibility\"") { + t.Errorf("diagnostic offers no fix: %q", ctx.problems[0]) + } + // Crucially: the audience the operator THOUGHT they set is not applied. + if ui.Visibility != "" { + t.Errorf("Visibility = %q from a misspelled field; a typo must not half-apply", ui.Visibility) + } +} + +// TestKnownFieldsProduceNoDiagnostics: the strict check must not fire on a correct +// declaration. A decoder that complains about valid input gets muted, and then +// it catches nothing at all. +func TestKnownFieldsProduceNoDiagnostics(t *testing.T) { + ctx := &decodeCtx{plugin: "billing"} + ui := decodeUIExtension(ctx, map[string]interface{}{ + "visibility": "admin", + "state_scope": map[string]interface{}{ + "global": []interface{}{"total", "by_source"}, + "depth": map[string]interface{}{"by_key": float64(1)}, + }, + "page": map[string]interface{}{"page_id": "billing", "mount": "
"}, + "elements": []interface{}{ + map[string]interface{}{"target": "status", "mount": "", "order": float64(1)}, + }, + }) + if len(ctx.problems) != 0 { + t.Errorf("a correct declaration produced diagnostics: %v", ctx.problems) + } + if ui.Visibility != "admin" { + t.Errorf("Visibility = %q, want admin", ui.Visibility) + } + if ui.StateScope == nil || len(ui.StateScope.Global) != 2 { + t.Errorf("state_scope not decoded: %+v", ui.StateScope) + } + if ui.Page == nil || ui.Page.PageID != "billing" { + t.Errorf("page not decoded: %+v", ui.Page) + } + if len(ui.Elements) != 1 || ui.Elements[0].Target != "status" { + t.Errorf("elements not decoded: %+v", ui.Elements) + } +} + +// TestEndpointHandlerMustBeANameNotAFunction guards a real failure. +// +// asString coerces anything non-string with fmt.Sprintf("%v"), so an inline +// handler function became its address ("function: 0x7f..."). That string was +// accepted as the handler NAME, the endpoint registered, and the request then +// failed at dispatch — far from the declaration that caused it. Rejecting the +// value at decode time names both the field and the path. +func TestEndpointHandlerMustBeANameNotAFunction(t *testing.T) { + dir := t.TempDir() + vm := NewVM(filepath.Join(dir, "adapters")) + if err := vm.Start(); err != nil { + t.Fatal(err) + } + t.Cleanup(vm.Stop) + ps := NewPlugins(vm, filepath.Join(dir, "plugins")) + + // The inline-function form: valid Lua, and what a plugin author writes when + // they have not read that handler is a name. + src := `local P = {} +P.name = "inline" +P.version = "1.0.0" +P.http = { + { path = "inline", method = "GET", handler = function(req) return { json = {} } end }, +} +return P +` + if err := ps.LoadSource("inline", src); err != nil { + t.Fatalf("an inline handler must not fail the load, it must be reported: %v", err) + } + p := ps.find("inline") + if p == nil { + t.Fatal("plugin not found") + } + if len(p.http) != 0 { + t.Errorf("the inline-function endpoint was accepted: %+v", p.http) + } + + // And the documented form still works. + src2 := `local P = {} +P.name = "byname" +P.version = "1.0.0" +P.http = { + { path = "ok", method = "GET", roles = { "admin" }, handler = "ok" }, +} +function P.ok(req) return { json = {} } end +return P +` + if err := ps.LoadSource("byname", src2); err != nil { + t.Fatal(err) + } + p2 := ps.find("byname") + if p2 == nil || len(p2.http) != 1 { + t.Fatalf("the documented form regressed: %+v", p2) + } + if p2.http[0].Handler != "ok" || p2.http[0].Method != "GET" || len(p2.http[0].Roles) != 1 { + t.Errorf("endpoint decoded wrong: %+v", p2.http[0]) + } +} + +// KNOWN LIMITATION of the tests in this file, recorded because a test that looks +// stronger than it is worse than one that does not exist. +// +// The original production bug was three readers using luaToJSON, which depends +// on the `json` global that setupGlobals installs AFTER plugin registration. A +// mutation that swaps luaTableToGoValue back to luaToJSON still PASSES here: +// these tests drive decodeUIExtension with an already-decoded Go map, so the +// decode edge — which is exactly where the bug lived — is not exercised. +// +// Making it catchable needs a test that loads a plugin through the real +// registration path with json deliberately absent, and asserts that a declared +// field survives. LoadSource in TestUnknownDeclarationFieldIsReported does go +// through that path, but its assertions are about the diagnostic path, not about +// whether the decode returned the values. +// +// Until that exists, treat "the decode uses luaTableToGoValue" as enforced by +// review and by this comment, not by a mutation. + +// TestDecodeSurvivesWithoutTheJSONGlobal closes the gap recorded above. +// +// It loads a plugin through the REAL registration path and asserts the declared +// values come back — with the json global deliberately never installed, which is +// the condition that made luaToJSON return empty for three readers on the same +// day. Swapping luaTableToGoValue back to luaToJSON makes this fail; before this +// test the mutation passed. +func TestDecodeSurvivesWithoutTheJSONGlobal(t *testing.T) { + dir := t.TempDir() + vm := NewVM(filepath.Join(dir, "adapters")) + if err := vm.Start(); err != nil { + t.Fatal(err) + } + t.Cleanup(vm.Stop) + ps := NewPlugins(vm, filepath.Join(dir, "plugins")) + + src := `local P = {} +P.name = "nojson" +P.version = "1.0.0" +P.ui = { + visibility = "admin", + page = { page_id = "nojson", mount = "
" }, +} +P.http = { + { path = "ping", method = "GET", roles = { "admin" }, handler = "ping" }, +} +function P.ping(req) return { json = { ok = true } } end +return P +` + if err := ps.LoadSource("nojson", src); err != nil { + t.Fatalf("load: %v", err) + } + p := ps.find("nojson") + if p == nil { + t.Fatal("plugin not found after LoadSource") + } + if p.UI == nil { + t.Fatal("ui not read back") + } + if p.UI.Visibility != "admin" { + t.Errorf("UI.Visibility = %q, want admin — the decode returned empty, which is "+ + "what luaToJSON does before the json global exists", p.UI.Visibility) + } + if p.UI.Page == nil || p.UI.Page.PageID != "nojson" { + t.Errorf("UI.Page not decoded: %+v", p.UI.Page) + } + if len(p.http) != 1 { + t.Fatalf("http endpoints = %d, want 1", len(p.http)) + } + if p.http[0].Handler != "ping" || p.http[0].Method != "GET" { + t.Errorf("endpoint decoded wrong: %+v", p.http[0]) + } + if len(p.http[0].Roles) != 1 || p.http[0].Roles[0] != "admin" { + t.Errorf("roles not decoded: %+v", p.http[0].Roles) + } +} diff --git a/internal/lua/plugins.go b/internal/lua/plugins.go index 4f24bef..4548955 100644 --- a/internal/lua/plugins.go +++ b/internal/lua/plugins.go @@ -667,20 +667,13 @@ func (ps *Plugins) LoadSource(name, code string) error { } // ---- UI ---- - if ui, ok := readPluginUI(w.L); ok { + if ui, ok := readPluginUI(w.L, p.Info.Name); ok { p.UI = ui } // ---- http endpoints ---- p.http = readPluginHTTP(w.L) - if ui, ok := readPluginUI(w.L); ok { - p.UI = ui - if ui.StateScope != nil { - ui.StateScope.Depth = compileDepthMap(ui.StateScope.Depth) - } - } - // ---- exports ---- // Read here, while the state is locked and before restore() runs: the list is // static for the life of the plugin, so Call never has to re-enter Lua just @@ -778,111 +771,112 @@ func pluginHookName(L *golua.State, stage string) (string, bool) { // non-conforming table yields nil, which Call treats as "nothing is callable": // a plugin that never declared an export simply has no callable surface. func readPluginExports(L *golua.State) []string { - L.GetGlobal(pluginGlobal) - if L.IsNil(-1) { - L.SetTop(0) - return nil - } - L.GetField(-1, "exports") - if L.Type(-1) != golua.LUA_TTABLE { - L.SetTop(0) - return nil - } - // luaTableToGoValue, NOT luaToJSON: this runs while the plugin is registering, - // before the json global is installed, and luaToJSON needs it. Going through - // JSON here silently produced an empty list, which then made every exported - // function look unexported. - raw := luaTableToGoValue(L, -1) - L.SetTop(0) - arr, ok := raw.([]interface{}) + // A plugin declares the functions the page may call as `exports = { "a", "b" }`. + // Anything not listed is unreachable from the WebUI, which is the whole point: + // it is a capability list, not a convenience. + raw, ok := pluginFieldValue(L, "exports") if !ok { return nil } - list := make([]string, 0, len(arr)) - for _, v := range arr { - if s := asString(v); s != "" { - list = append(list, s) - } - } - return list + return decStringList(map[string]interface{}{"v": raw}, "v") } -func readPluginUI(L *golua.State) (*UIExtension, bool) { +func readPluginUI(L *golua.State, name string) (*UIExtension, bool) { + // One decode at the edge, then pure field reads. + // + // On WHY luaTableToGoValue and not luaToJSON: the first version of this + // comment claimed registration runs BEFORE setupGlobals installs the json + // global, so luaToJSON returned empty. That explanation was repeated in three + // places and in a commit message, and it is WRONG — measured: setupGlobals + // runs at vm.go:574, these readers at 787, and json IS present on the stack + // when they run. + // + // What actually differs is INDEX SEMANTICS. luaTableToGoValue converts a + // NEGATIVE index to absolute immediately (abs = GetTop() + idx + 1); + // luaToJSON keeps the relative index and pushes it before Call. So after a + // SetTop(0) the two disagree, and the table-valued converter — the one that + // handles nested tables, arrays and mixed keys correctly — is the one that + // needs the stack shape kept intact. + // + // Which is exactly why the decode happens here, ONCE, immediately after + // GetGlobal+GetField, and every field read below is a pure function on the + // decoded Go value. The old readers repeated the stack dance per field. + m, ok := pluginFieldTable(L, "ui") + if !ok { + return nil, false + } + ctx := &decodeCtx{plugin: name} + ui := decodeUIExtension(ctx, m) + + if ui.Page == nil && len(ui.Pages) == 0 && len(ui.Elements) == 0 { + return nil, false + } + if len(ctx.problems) > 0 { + log.Printf("[plugins] %s: %s", ctx.plugin, strings.Join(ctx.problems, "; ")) + } + return &ui, true +} + +// pluginFieldTable reads one top-level declaration table off the plugin global. +func pluginFieldTable(L *golua.State, field string) (map[string]interface{}, bool) { L.GetGlobal(pluginGlobal) if L.IsNil(-1) { L.SetTop(0) return nil, false } - L.GetField(-1, "ui") + L.GetField(-1, field) if L.Type(-1) == golua.LUA_TNIL { L.SetTop(0) return nil, false } - // luaTableToGoValue, NOT luaToJSON. This runs while the plugin is - // registering, BEFORE setupGlobals installs the json global that luaToJSON - // depends on — so the conversion fails and every field except the ones on - // Page/Pages (read separately) comes back empty. That is why visibility and - // state_scope silently read as unset even though the .lua declares them: - // the third field in this file to be lost to the same cause, after - // readPluginExports and readPluginHTTP. - var ui UIExtension raw := luaTableToGoValue(L, -1) L.SetTop(0) - if m, ok := raw.(map[string]interface{}); ok { - ui.Visibility = asString(m["visibility"]) - if scope, ok := m["state_scope"].(map[string]interface{}); ok { - sc := &StateScope{} - if gs, ok := scope["global"].([]interface{}); ok { - for _, g := range gs { - if s := asString(g); s != "" { - sc.Global = append(sc.Global, s) - } - } - } - if ds, ok := scope["depth"].(map[string]interface{}); ok { - sc.Depth = map[string]int{} - for k, v := range ds { - if n, ok := asFloat(v); ok { - sc.Depth[k] = int(n) - } - } - sc.Depth = compileDepthMap(sc.Depth) - } - ui.StateScope = sc - } - if pg, ok := m["page"].(map[string]interface{}); ok { - ui.Page = uipageFromValue(pg) - } - if pgs, ok := m["pages"].([]interface{}); ok { - for _, it := range pgs { - if pm, ok := it.(map[string]interface{}); ok { - ui.Pages = append(ui.Pages, uipageFromValue(pm)) - } - } - } - if els, ok := m["elements"].([]interface{}); ok { - for _, it := range els { - em, ok := it.(map[string]interface{}) - if !ok { - continue - } - el := UIElement{ - Target: asString(em["target"]), - Anchor: asString(em["anchor"]), - Mount: asString(em["mount"]), - Audience: asString(em["audience"]), - } - if n, ok := asFloat(em["order"]); ok { - el.Order = int(n) - } - ui.Elements = append(ui.Elements, el) - } + m, ok := raw.(map[string]interface{}) + return m, ok +} + +// decodeUIExtension reads a plugin's `ui` declaration. +// +// The field list below is the single source of truth for what the gateway +// understands. A declaration using anything else is REPORTED rather than +// silently dropped, because the old hand-written reader ignored unknown keys with +// no trace: `visibilty = "admin"` (one character off) restricted nothing and +// looked exactly like a plugin that simply chose not to declare an audience. +func decodeUIExtension(ctx *decodeCtx, m map[string]interface{}) UIExtension { + ctx.strictKeys(m, "ui", "visibility", "state_scope", "page", "pages", "elements") + + var ui UIExtension + ui.Visibility = decString(m, "visibility") + + if scope := decTable(m, "state_scope"); scope != nil { + ctx.strictKeys(scope, "ui.state_scope", "global", "depth") + sc := &StateScope{Global: decStringList(scope, "global")} + if d := decIntMap(scope, "depth"); len(d) > 0 { + sc.Depth = compileDepthMap(d) } + ui.StateScope = sc } - if ui.Page == nil && len(ui.Pages) == 0 && len(ui.Elements) == 0 { - return nil, false + + if pg := decTable(m, "page"); pg != nil { + ui.Page = uipageFromValue(pg) } - return &ui, true + for _, pm := range decTableList(m, "pages") { + ui.Pages = append(ui.Pages, uipageFromValue(pm)) + } + for _, em := range decTableList(m, "elements") { + ctx.strictKeys(em, "ui.elements[]", "target", "anchor", "mount", "audience", "order") + el := UIElement{ + Target: decString(em, "target"), + Anchor: decString(em, "anchor"), + Mount: decString(em, "mount"), + Audience: decString(em, "audience"), + } + if n, ok := asFloat(em["order"]); ok { + el.Order = int(n) + } + ui.Elements = append(ui.Elements, el) + } + return ui } func (ps *Plugins) append(p *Plugin) { @@ -2176,64 +2170,82 @@ func containsPage(pages []*UIPage, id string) bool { // not exist. Failing the whole plugin would take a working billing widget down // over a typo in an experimental endpoint. func readPluginHTTP(L *golua.State) []HTTPHandler { - L.GetGlobal(pluginGlobal) - if L.IsNil(-1) { - L.SetTop(0) - return nil - } - L.GetField(-1, "http") - if L.Type(-1) != golua.LUA_TTABLE { - L.SetTop(0) - return nil - } - // luaTableToGoValue, NOT luaToJSON and NOT a hand-rolled GetField loop. + // `http` is a LIST of declarations, each naming the handler function by + // STRING: // - // luaToJSON needs the json global, which is not installed yet while a plugin - // is registering. And golua's GetField does NOT convert a numeric key: with - // a string index "1" it returns nil, so an index loop silently produced an - // empty list and every plugin appeared to declare no endpoints at all. - raw := luaTableToGoValue(L, -1) - L.SetTop(0) - items, ok := raw.([]interface{}) + // http = { + // { path = "health", method = "GET", roles = { "admin" }, handler = "health" }, + // } + // + // handler is a name, not a value. An inline function silently produced an + // empty handler field and the endpoint was dropped without a word. + raw, ok := pluginFieldValue(L, "http") if !ok { return nil } + // A single declaration may be given as a bare table instead of a one-element + // list; accepting both is friendlier than demanding one spelling. + items, isList := raw.([]interface{}) + if !isList { + if one, isTable := raw.(map[string]interface{}); isTable { + items = []interface{}{one} + } else { + return nil + } + } + ctx := &decodeCtx{plugin: "http"} var list []HTTPHandler for _, it := range items { - m, ok := it.(map[string]interface{}) + h, ok := it.(map[string]interface{}) if !ok { continue } - h := HTTPHandler{ - Path: asString(m["path"]), - Method: asString(m["method"]), - Handler: asString(m["handler"]), - Description: asString(m["description"]), - Public: m["public"] == true, + ctx.strictKeys(h, "http[]", "path", "method", "handler", "description", + "roles", "keys", "public") + dh := HTTPHandler{ + Path: decString(h, "path"), + Method: decString(h, "method"), + Description: decString(h, "description"), + Roles: decStringList(h, "roles"), + Keys: decStringList(h, "keys"), } - if roles, ok := m["roles"].([]interface{}); ok { - for _, r := range roles { - if s := asString(r); s != "" { - h.Roles = append(h.Roles, s) - } + // handler must be a NAME, not a value. asString would happily turn a + // function into its address ("function: 0x..."), which then failed much + // later at dispatch with no connection to the declaration that caused it. + // Rejecting it here names the field and the path. + if hv, present := h["handler"]; present { + name, isStr := hv.(string) + if !isStr { + ctx.warnf("http[%s]: handler must be the NAME of a function on the "+ + "plugin table, not the function itself", decString(h, "path")) + continue } + dh.Handler = name } - if keys, ok := m["keys"].([]interface{}); ok { - for _, k := range keys { - if s := asString(k); s != "" { - h.Keys = append(h.Keys, s) - } - } + if p, ok := h["public"].(bool); ok { + dh.Public = p } // A malformed entry is dropped rather than fatal: one bad route must not - // cost a plugin its hooks and pages. That is also why the mounted count - // can be lower than the declared count — /api/plugins lists what mounted. - if method, mErr := normalizeHandlerMethod(h.Method); mErr == nil { - h.Method = method - if validateHandlerPath(h.Path) == nil && h.Handler != "" { - list = append(list, h) - } + // cost a plugin its hooks and pages. The reason is named so a plugin whose + // endpoint silently vanished can find out why. + method, mErr := normalizeHandlerMethod(dh.Method) + switch { + case mErr != nil: + ctx.warnf("http[%s]: %v", dh.Path, mErr) + continue + case validateHandlerPath(dh.Path) != nil: + ctx.warnf("http[%s]: invalid path", dh.Path) + continue + case dh.Handler == "": + ctx.warnf("http[%s]: handler must name a function on the plugin table "+ + "(an inline function is not supported)", dh.Path) + continue } + dh.Method = method + list = append(list, dh) + } + if len(ctx.problems) > 0 { + log.Printf("[plugins] %s", strings.Join(ctx.problems, "; ")) } return list }