diff --git a/contracts.go b/contracts.go index abf5d532..ba6c9a7d 100644 --- a/contracts.go +++ b/contracts.go @@ -141,8 +141,9 @@ type ClickHandler interface { // is found it falls back to the event target's HTML id, so it is empty only // when the target has no id either. // - // [ErrEventUnhandled] tries the next handler. Other non-nil errors are logged - // and queued as danger alerts when possible during Request event processing. + // [ErrEventUnhandled] tries the next handler. Request event processing logs + // other errors and, unless they match [ErrEventLogOnly], queues them as danger + // alerts when possible. JawsClick(elem *Element, click Click) (err error) } @@ -158,8 +159,9 @@ type ContextMenuHandler interface { // Events that occur while the bundled client's WebSocket is not open are not // forwarded or replayed. // - // [ErrEventUnhandled] tries the next handler. Other non-nil errors are logged - // and queued as danger alerts when possible during Request event processing. + // [ErrEventUnhandled] tries the next handler. Request event processing logs + // other errors and, unless they match [ErrEventLogOnly], queues them as danger + // alerts when possible. JawsContextMenu(elem *Element, click Click) (err error) } diff --git a/doc/runtime.md b/doc/runtime.md index 2a5e0686..197fb20a 100644 --- a/doc/runtime.md +++ b/doc/runtime.md @@ -61,10 +61,12 @@ page dot does not install them. See `ConnectFn` in Register event handlers during rendering, before the Element is frozen. Dispatch tries attached handlers in reverse registration order, then the -Element's UI. Return `jaws.ErrEventUnhandled` to try the next handler. Other -errors are logged and, when possible, displayed as browser danger alerts. -Recovered panics show a generic alert while logs retain details. -If no handler accepts an event, it is ignored without an error or alert. +Element's UI. Return `jaws.ErrEventUnhandled` to try the next handler; an event +with no accepting handler is ignored. Other errors are logged and, when possible, +displayed as browser danger alerts unless they match `jaws.ErrEventLogOnly`. +Recovered panics and JsVar store rejections match this sentinel. During browser +events, use `Request.Alert` for explicit feedback, including inside +[`ClientCheck`](ui/jsvar.md). Browser event handlers run serially within each Request, concurrently with update processing and handlers in other Requests. Synchronize shared application state diff --git a/doc/transport.md b/doc/transport.md index 375b1a5a..3daba153 100644 --- a/doc/transport.md +++ b/doc/transport.md @@ -117,7 +117,8 @@ known to the Request are removed from server bookkeeping. `Hook` is a synchronous test event. The browser never sends it, and inbound messages do not dispatch it. A broadcast Hook invokes a matching handler; that -handler must not send its own messages. Its returned error becomes an alert. +handler must not send its own messages. Returned errors follow the +[event alert policy](runtime.md#connect-and-handle-events). ## Limits and connection loops diff --git a/doc/ui/jsvar.md b/doc/ui/jsvar.md index d96c923f..91500c5e 100644 --- a/doc/ui/jsvar.md +++ b/doc/ui/jsvar.md @@ -39,8 +39,12 @@ complete tentative Go value under the store's write lock. Validate authorization through the originating Element and validate the full value, including fields omitted from JSON and changes through parent or root paths. A `null` proposal for a struct zeroes all its fields, including unexported and `json:"-"` fields. An -error or panic rolls the proposal back. The callback must only inspect: do not -mutate or retain tentative data, reacquire the lock, or call a store setter. +error or panic rolls the proposal back. The callback must only inspect tentative +data: do not mutate or retain it, reacquire the lock, or call a store setter. + +Rejected proposals, including panicking checks, are logged and corrected without +automatic browser alerts. When handling browser input, `ClientCheck` can call +`source.Request.Alert("warning", "Choose a valid value.")` for user feedback. Every binding receives the same JSON value; use separate stores for data with different visibility. Proposal outcomes can depend on Go fields omitted from diff --git a/errors.go b/errors.go index af9f9aba..bd7013ea 100644 --- a/errors.go +++ b/errors.go @@ -226,13 +226,11 @@ func newErrNoWebSocketRequest(rq *Request) error { return errNoWebSocketRequest{Addr: rq.remoteIP} } -// ErrEventHandlerPanic is returned by [CallEventHandlers] when a user event handler -// panics. +// ErrEventHandlerPanic reports a recovered event-handler panic. // -// Match it with [errors.Is]. When the recovered panic value is itself an error it is -// available via Unwrap (and thus [errors.As] / [errors.Is]); a non-error panic value -// appears only in the formatted message. Request event processing logs the full -// error and queues a generic browser alert. +// [CallEventHandlers] returns an error matching this sentinel and [ErrEventLogOnly]. +// It unwraps to the recovered value if that value is an error; other panic values +// appear only in the error text. var ErrEventHandlerPanic errEventHandlerPanic type errEventHandlerPanic struct { @@ -247,13 +245,8 @@ func (e errEventHandlerPanic) Error() string { return fmt.Sprintf("jaws: %v panic: %v", e.Type, e.Value) } -// JawsClientAlert returns the browser message for a recovered handler panic. -func (errEventHandlerPanic) JawsClientAlert() string { - return "event handler failed" -} - func (errEventHandlerPanic) Is(target error) bool { - return target == ErrEventHandlerPanic + return target == ErrEventHandlerPanic || target == ErrEventLogOnly } func (e errEventHandlerPanic) Unwrap() error { @@ -263,6 +256,13 @@ func (e errEventHandlerPanic) Unwrap() error { return nil } +// ErrEventLogOnly marks event errors that are logged without automatic browser alerts. +// +// Request event processing suppresses an error's alert when [errors.Is] matches +// this sentinel. Recovered handler panics and JsVar rejections match it. +// Explicit [Request.Alert] calls are unaffected. +var ErrEventLogOnly = errors.New("event error is log-only") + type errEventUnhandled struct{} func (errEventUnhandled) Error() string { diff --git a/event.go b/event.go index 7cb6ec92..a420a976 100644 --- a/event.go +++ b/event.go @@ -145,8 +145,9 @@ type InputHandler interface { // its WebSocket is open and does not queue them for later delivery. Native // changes that emit neither input nor change do not invoke JawsInput. // - // [ErrEventUnhandled] tries the next handler. Other non-nil errors are logged - // and queued as danger alerts when possible during Request event processing. + // [ErrEventUnhandled] tries the next handler. Request event processing logs + // other errors and, unless they match [ErrEventLogOnly], queues them as danger + // alerts when possible. JawsInput(elem *Element, value string) (err error) } diff --git a/jaws.go b/jaws.go index 894933f7..4ea1c0e0 100644 --- a/jaws.go +++ b/jaws.go @@ -12,8 +12,10 @@ package jaws // must not block or call back into the same Request. // // Bound-value locks in lib/bind, lib/ui, and lib/named are released before dirtying or -// broadcasting. InitialHTMLAttrHandler callbacks run without caller-held widget or -// bound-value locks; bind.Binder acquires its own lock before calling its narrower +// broadcasting, except that a JsVar ClientCheck may send an explicit Request.Alert. +// Broadcast delivery does not acquire bound-value locks or wait for Requests. +// InitialHTMLAttrHandler callbacks run without caller-held widget or bound-value +// locks; bind.Binder acquires its own lock before calling its narrower // InitialHTMLAttrHook. Code holding a core lock must not invoke UI value methods. // Container reconciliation has one deliberate reverse edge: containerState.mu may be // held while Request.NewElement takes Request.mu. Provider callbacks and validation diff --git a/lib/ui/errjsvar.go b/lib/ui/errjsvar.go index 2cf4b7d4..beefbe58 100644 --- a/lib/ui/errjsvar.go +++ b/lib/ui/errjsvar.go @@ -36,8 +36,5 @@ func (e errJsVarClientWrite) Error() string { return e.cause.Error() } func (e errJsVarClientWrite) Is(target error) bool { // Event dispatch treats ErrEventUnhandled as fallthrough. A rejected proposal // is always handled, even when an application check returns that sentinel. - return target != jaws.ErrEventUnhandled && errors.Is(e.cause, target) + return target == jaws.ErrEventLogOnly || (target != jaws.ErrEventUnhandled && errors.Is(e.cause, target)) } - -// JawsClientAlert returns a generic browser message for a rejected proposal. -func (errJsVarClientWrite) JawsClientAlert() string { return "invalid JsVar update" } diff --git a/lib/ui/jsvar.go b/lib/ui/jsvar.go index c5bd4bde..e6ff8ec2 100644 --- a/lib/ui/jsvar.go +++ b/lib/ui/jsvar.go @@ -82,12 +82,18 @@ func validateJsVarPath(path string) error { // JsVarCheck validates the complete tentative state of a browser proposal. // // A nil check denies browser writes. The check runs under the store's write lock -// after jq tentatively applies a changed Go value. It must only inspect next; -// an error or panic rolls the proposal back. The source can authorize a user -// or session. Validate the complete value, including fields omitted from JSON +// after jq tentatively applies a changed Go value. It must not mutate or retain +// next, reacquire the lock, or call a store setter. An error or panic rolls the +// proposal back. The source can authorize a user or session. +// Validate the complete value, including fields omitted from JSON // and changes through parent or root paths. A null proposal for a struct zeroes // all its fields, including unexported and json:"-" fields. The check sees // converted Go values, not raw browser JSON; see [JsVarBinding.JawsInput]. +// +// Rejection errors are logged during event processing without automatic browser +// alerts. When handling browser input, the check may call [jaws.Request.Alert] +// on source.Request to provide user-facing feedback. An error matching +// [ErrJsVarTooLarge] cancels the Request. type JsVarCheck[T any] func(source *jaws.Element, next *T, path string) error // JSONSizeCheck limits the encoded size of a tentative JsVar store value. @@ -586,8 +592,9 @@ func (store *JsVarStore[T]) projectVisiblePatch(path, root string, visible any) // A rejected, invalid, or unchanged proposal schedules a canonical correction // for its source binding. A changed accepted proposal invalidates every binding. // An unchanged proposal to a complex Go shape is rejected. +// Returned errors match [jaws.ErrEventLogOnly]. // A panicking check rolls back and schedules a root correction before the panic -// continues. [ErrJsVarTooLarge] cancels the source Request for reload recovery. +// continues. [ErrJsVarTooLarge] cancels the source Request. // // Browser JSON numbers are decoded as float64 and converted by jq to the Go // destination before ClientCheck runs. Conversion may lose information: 300 @@ -629,7 +636,6 @@ func (binding *JsVarBinding[T]) JawsInput(elem *jaws.Element, input string) (err if err != nil { if errors.Is(err, ErrJsVarTooLarge) { elem.Request.Cancel(err) - return ErrJsVarTooLarge } return errJsVarClientWrite{err} } diff --git a/lib/ui/jsvar_input_test.go b/lib/ui/jsvar_input_test.go index f5e0e558..ce56a759 100644 --- a/lib/ui/jsvar_input_test.go +++ b/lib/ui/jsvar_input_test.go @@ -23,6 +23,10 @@ func TestJsVarRejectedBrowserInput(t *testing.T) { name string input string checkError error + checkPanic bool + alert string + hook bool + readOnly bool wantPatch string wantChecks int }{ @@ -30,8 +34,13 @@ func TestJsVarRejectedBrowserInput(t *testing.T) { {name: "invalid path", input: "value..field=2", wantPatch: `={"value":1}`}, {name: "missing path", input: "missing=2", wantPatch: "missing="}, {name: "invalid JSON", input: "value={", wantPatch: `={"value":1}`}, + {name: "read-only store", input: "value=2", checkError: ErrJsVarReadOnly, readOnly: true, wantPatch: "value=1"}, {name: "rejected check", input: "value=2", checkError: errors.New("private check detail"), wantPatch: "value=1", wantChecks: 1}, {name: "unhandled check", input: "value=2", checkError: fmt.Errorf("private check detail: %w", jaws.ErrEventUnhandled), wantPatch: "value=1", wantChecks: 1}, + {name: "explicit alert", input: "value=2", checkError: errors.New("private check detail"), alert: "Choose a value no greater than 1.", wantPatch: "value=1", wantChecks: 1}, + {name: "hook rejection", input: "value=2", checkError: errors.New("private check detail"), hook: true, wantPatch: "value=1", wantChecks: 1}, + {name: "panicking check", input: "value=2", checkError: errors.New("private panic detail"), checkPanic: true, wantPatch: `={"value":1}`, wantChecks: 1}, + {name: "hook panic", input: "value=2", checkError: errors.New("private panic detail"), checkPanic: true, hook: true, wantPatch: `={"value":1}`, wantChecks: 1}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { @@ -40,6 +49,8 @@ func TestJsVarRejectedBrowserInput(t *testing.T) { if err != nil { t.Fatal(err) } + logger := new(templateLogger) + jw.Logger = logger go jw.Serve() tr := jawstest.NewTestRequest(jw, nil) <-tr.ReadyCh @@ -55,20 +66,51 @@ func TestJsVarRejectedBrowserInput(t *testing.T) { }{Value: 1} store := newTestJsVarStore(t, jw, "client", &mu, &state) checks := 0 - store.ClientCheck = func(*jaws.Element, *struct { + store.ClientCheck = func(source *jaws.Element, _ *struct { Value int `json:"value"` - }, string, + }, _ string, ) error { checks++ + if tt.alert != "" { + source.Request.Alert("warning", tt.alert) + } + if tt.checkPanic { + panic(tt.checkError) + } return tt.checkError } + if tt.readOnly { + store.ClientCheck = nil + } _, elem, _ := renderTestJsVar(t, tr.Request, store) - tr.InCh <- wire.WsMsg{Jid: elem.Jid(), What: what.JsVar, Data: tt.input} + if tt.hook { + tr.BcastCh <- wire.Message{Dest: store, What: what.Hook, Data: tt.input} + } else { + tr.InCh <- wire.WsMsg{Jid: elem.Jid(), What: what.JsVar, Data: tt.input} + } synctest.Wait() time.Sleep(jaws.DefaultUpdateInterval + time.Millisecond) synctest.Wait() + logged := logger.sync(t, jw) + if len(logged) != 1 { + t.Fatalf("logged errors = %v, want one rejection", logged) + } + if !errors.Is(logged[0], jaws.ErrEventLogOnly) { + t.Fatalf("logged error does not match ErrEventLogOnly: %v", logged[0]) + } + if tt.checkPanic { + if !errors.Is(logged[0], jaws.ErrEventHandlerPanic) || !errors.Is(logged[0], tt.checkError) { + t.Fatalf("logged panic = %v, want %v", logged[0], tt.checkError) + } + } else if tt.checkError != nil && logged[0].Error() != tt.checkError.Error() { + t.Fatalf("logged error = %q, want %q", logged[0], tt.checkError) + } var alerts, patches int + var wantAlerts int + if tt.alert != "" { + wantAlerts = 1 + } for { select { case msg, ok := <-tr.OutCh: @@ -78,7 +120,7 @@ func TestJsVarRejectedBrowserInput(t *testing.T) { switch msg.What { case what.Alert: alerts++ - if msg.Jid != 0 || msg.Data != "danger\ninvalid JsVar update" { + if tt.alert == "" || msg.Jid != 0 || msg.Data != "warning\n"+tt.alert { t.Fatalf("browser alert = %#v", msg) } case what.JsVar: @@ -90,7 +132,7 @@ func TestJsVarRejectedBrowserInput(t *testing.T) { t.Fatalf("unexpected browser message: %#v", msg) } default: - if alerts != 1 || patches != 1 || checks != tt.wantChecks || state.Value != 1 { + if alerts != wantAlerts || patches != 1 || checks != tt.wantChecks || state.Value != 1 { t.Fatalf("alerts=%d patches=%d checks=%d value=%d", alerts, patches, checks, state.Value) } return diff --git a/lib/ui/jsvar_store_test.go b/lib/ui/jsvar_store_test.go index b2f38f52..cc980f3b 100644 --- a/lib/ui/jsvar_store_test.go +++ b/lib/ui/jsvar_store_test.go @@ -128,6 +128,9 @@ func TestJsVarStoreJSONSizeRollback(t *testing.T) { if !errors.Is(err, ErrJsVarTooLarge) || state.Text != "ok" { t.Fatalf("oversize proposal: state=%q err=%v", state.Text, err) } + if !errors.Is(err, jaws.ErrEventLogOnly) { + t.Fatalf("oversize proposal error permits an automatic alert: %v", err) + } if rq.Context().Err() == nil { t.Fatal("oversize proposal did not cancel its Request") } diff --git a/lib/what/what.go b/lib/what/what.go index 66f30669..22b9e784 100644 --- a/lib/what/what.go +++ b/lib/what/what.go @@ -69,12 +69,10 @@ const ( // Hook synchronously invokes the matching event handler. // - // Hook is a testing facility rather than part of the browser wire protocol: - // the JaWS client never sends it, and inbound client messages are never - // dispatched as Hook. Broadcasting a Hook message lets a test drive an - // element's event handler synchronously, without round-tripping through the - // client. The handler must not send messages of its own; any error it - // returns is delivered to the client as an [Alert]. + // Hook is a test-only broadcast event; inbound client messages never dispatch + // it. Handlers must not send messages of their own. Errors are logged and + // normally sent as [Alert] messages; recovered panics and JsVar store rejections + // omit alerts. Hook ) diff --git a/request_test.go b/request_test.go index 689ee42a..bafce7a6 100644 --- a/request_test.go +++ b/request_test.go @@ -1282,48 +1282,89 @@ func TestRequest_Trigger(t *testing.T) { } } -func TestRequest_EventHandlerPanicAlertHidesDetails(t *testing.T) { - tj := newTestJaws() - t.Cleanup(tj.Close) - rq := newWrappedTestRequest(tj.Jaws, nil) - t.Cleanup(rq.Close) - - const detail = "password authentication failed for billing_rw" - id := rq.Register(&testUi{}, func(*Element, string) error { - panic(detail) - }) - rq.InCh <- wire.WsMsg{Jid: id, What: what.Input} - - select { - case msg := <-rq.OutCh: - if msg.What != what.Alert || msg.Data != "danger\nevent handler failed" { - t.Fatalf("panic alert = %#v, want generic danger alert", msg) - } - case <-time.After(testTimeout): - t.Fatal("timed out waiting for panic alert") - } - awaitTestLoggerQueue(t, tj.Jaws) - if got := tj.log.String(); !strings.Contains(got, detail) { - t.Fatalf("operator log omitted panic detail: %q", got) - } -} - -func TestRequest_HookHandlerPanicAlertHidesDetails(t *testing.T) { - rq := newTestRequest(t) - defer rq.Close() - ui := &testUi{} - rq.Register(ui, func(*Element, string) error { - panic("private panic detail") - }) - rq.Jaws.Broadcast(wire.Message{Dest: ui, What: what.Hook}) +func TestRequest_EventErrorAlerts(t *testing.T) { + for _, tt := range []struct { + name string + panic bool + hook bool + alert bool + logOnly bool + }{ + {name: "returned error"}, + {name: "panic", panic: true}, + {name: "hook returned error", hook: true}, + {name: "hook panic", hook: true, panic: true}, + {name: "explicit alert before panic", panic: true, alert: true}, + {name: "wrapped log-only error", logOnly: true}, + {name: "hook wrapped log-only error", hook: true, logOnly: true}, + } { + t.Run(tt.name, func(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + jw, err := New() + if err != nil { + t.Fatal(err) + } + defer jw.Close() + logger := new(eventErrorLogger) + jw.Logger = logger + go jw.Serve() + rq := newWrappedTestRequest(jw, nil) + defer rq.Close() + <-rq.ReadyCh + + wantErr := errors.New("private handler detail") + const alert = "warning\nChoose a valid value." + ui := &testUi{} + id := rq.Register(ui, func(elem *Element, _ string) error { + if tt.alert { + elem.Request.Alert("warning", "Choose a valid value.") + } + if tt.panic { + panic(wantErr) + } + if tt.logOnly { + return fmt.Errorf("%w: %w", ErrEventLogOnly, wantErr) + } + return wantErr + }) + if tt.hook { + rq.Jaws.Broadcast(wire.Message{Dest: ui, What: what.Hook}) + } else { + rq.InCh <- wire.WsMsg{Jid: id, What: what.Input} + } + synctest.Wait() - select { - case msg := <-rq.OutCh: - if msg.What != what.Alert || msg.Data != "danger\nevent handler failed" { - t.Fatalf("hook panic alert = %#v, want generic danger alert", msg) - } - case <-time.After(testTimeout): - t.Fatal("timed out waiting for hook panic alert") + logged := logger.loggedErrors() + if len(logged) != 1 || !errors.Is(logged[0], wantErr) || errors.Is(logged[0], ErrEventHandlerPanic) != tt.panic { + t.Fatalf("logged errors = %v, want handler error with panic=%v", logged, tt.panic) + } + if rq.Context().Err() != nil { + t.Fatal("handler failure cancelled the Request") + } + if tt.alert || (!tt.panic && !tt.logOnly) { + want := wire.WsMsg{What: what.Alert, Data: alert} + if !tt.alert { + want.Data = "danger\n" + wantErr.Error() + if tt.hook { + want.Jid = id + } + } + select { + case msg := <-rq.OutCh: + if msg != want { + t.Fatalf("browser alert = %#v, want %#v", msg, want) + } + default: + t.Fatal("missing browser alert") + } + } + select { + case msg := <-rq.OutCh: + t.Fatalf("unexpected browser message: %#v", msg) + default: + } + }) + }) } } diff --git a/requestloop.go b/requestloop.go index b2275535..fb4e7182 100644 --- a/requestloop.go +++ b/requestloop.go @@ -186,11 +186,10 @@ func (rq *Request) handleBroadcast(tagmsg wire.Message, eventCallCh chan eventFn // Hook messages synchronously invoke the element's event handler; see // [what.Hook]. They exist for testing: the JaWS client never sends Hook, // so they only arrive here via Broadcast. The handler must not send any - // messages itself, but may return an error, which is sent back to the - // client as an alert message. - if err := rq.Jaws.Log(rq.callAllEventHandlers(elem.Jid(), tagmsg.What, tagmsg.Data)); err != nil { + // messages itself. Returned errors follow the event alert policy. + if err := rq.Jaws.Log(rq.callAllEventHandlers(elem.Jid(), tagmsg.What, tagmsg.Data)); err != nil && !errors.Is(err, ErrEventLogOnly) { var m wire.WsMsg - m.FillAlert(eventAlertError(err)) + m.FillAlert(err) m.Jid = elem.Jid() rq.queue(m) } @@ -540,9 +539,9 @@ func (rq *Request) eventCaller(eventCallCh <-chan eventFnCall, outboundMsgCh cha continue default: } - if err := rq.Jaws.Log(call.invoke()); err != nil { + if err := rq.Jaws.Log(call.invoke()); err != nil && !errors.Is(err, ErrEventLogOnly) { var m wire.WsMsg - m.FillAlert(eventAlertError(err)) + m.FillAlert(err) // This error alert is best-effort: unlike queueEvent, which cancels the // Request with ErrRequestOverloaded when its channel fills (dropping a // queued event could desync browser and backend state), a dropped alert @@ -558,20 +557,6 @@ func (rq *Request) eventCaller(eventCallCh <-chan eventFnCall, outboundMsgCh cha } } -// clientAlertError is an internal marker for library errors with safe browser text. -type clientAlertError interface { - JawsClientAlert() string -} - -// eventAlertError limits browser alerts for library errors that carry internal detail. -func eventAlertError(err error) error { - var safe clientAlertError - if errors.As(err, &safe) { - return errors.New(safe.JawsClientAlert()) - } - return err -} - // onConnect calls the [Request]'s [ConnectFn] if it is not nil, returning its // error or converting its panic to an error. Returns nil if [ConnectFn] is nil. func (rq *Request) onConnect() (err error) {