diff --git a/CLAUDE.md b/CLAUDE.md index 5cf7344..24ca331 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -75,9 +75,9 @@ go tool gsx fmt -w . - Apply globally via `WithMiddlewares` or per-page via a `Middlewares()` method (also applies to descendants) 6. **Dependency Injection**: Type-based DI via `WithArgs(...)` - - `args.go`: `argRegistry` matches by type; a registered pointer fills a value param (not the reverse), with an assignability fallback + - `args.go`: `argRegistry` matches by type; a registered pointer fills a value param (not the reverse); an interface param takes the one registration implementing it, and two are an error - Each registered type appears once; use named types to disambiguate - - Generic types are supported; interface-typed params are not filled by a registered implementation (see `generics_injection_test.go`) + - `Mount` checks every `Props`/extended `ServeHTTP` param for ambiguity up front (`checkInjection`); `Parse` skips it. A missing dependency stays a request-time error, since pages may be mounted in configurations that don't register their services ### Key Design Patterns diff --git a/args.go b/args.go index 7f163be..da49edc 100644 --- a/args.go +++ b/args.go @@ -3,6 +3,8 @@ package structpages import ( "fmt" "reflect" + "slices" + "strings" ) type argRegistry map[reflect.Type]reflect.Value @@ -20,8 +22,51 @@ func (args argRegistry) addArg(v any) error { return nil } -// note that p.args are always pointers -func (args argRegistry) getArg(pt reflect.Type) (reflect.Value, bool) { +// getArg finds the registered value for a parameter of type pt. A +// registration of exactly that type wins (a registered pointer also fills +// its value type). Failing that, an interface parameter takes the one +// registered value whose type implements it; more than one is an error, +// since picking one would be a guess. found is false when nothing matches. +func (args argRegistry) getArg(pt reflect.Type) (v reflect.Value, found bool, err error) { + if pt.Kind() == reflect.Interface { + if iv, ok := args[pt]; ok { + return iv, true, nil + } + return args.implementationOf(pt) + } + v, found = args.getConcreteArg(pt) + return v, found, nil +} + +// implementationOf returns the single registered value implementing +// iface. +func (args argRegistry) implementationOf(iface reflect.Type) (reflect.Value, bool, error) { + var impls []reflect.Type + for t := range args { + if t.Implements(iface) { + impls = append(impls, t) + } + } + switch len(impls) { + case 0: + return reflect.Value{}, false, nil + case 1: + return args[impls[0]], true, nil + } + names := make([]string, len(impls)) + for i, t := range impls { + names[i] = t.String() + } + slices.Sort(names) + return reflect.Value{}, false, fmt.Errorf( + "%s is implemented by more than one registered value (%s); "+ + "declare the parameter as the concrete type, or narrow the interface", + iface, strings.Join(names, ", ")) +} + +// getConcreteArg finds the registered value for a non-interface +// parameter type. Note that p.args are always pointers. +func (args argRegistry) getConcreteArg(pt reflect.Type) (reflect.Value, bool) { st := pt needsElem, needsPtr := false, false if pt.Kind() != reflect.Pointer { diff --git a/args_test.go b/args_test.go index a2a1065..a62aaa8 100644 --- a/args_test.go +++ b/args_test.go @@ -129,12 +129,12 @@ func TestArgRegistry_getArg(t *testing.T) { wantFound: true, }, { - name: "implementation not assignable to interface", + name: "registered implementation fills an interface parameter", registry: argRegistry{ reflect.TypeOf(implVal): reflect.ValueOf(implVal), }, lookupType: reflect.TypeOf((*testInterface)(nil)).Elem(), - wantFound: false, + wantFound: true, }, { name: "empty registry returns not found", @@ -175,7 +175,7 @@ func TestArgRegistry_getArg(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - gotValue, gotFound := tt.registry.getArg(tt.lookupType) + gotValue, gotFound := lookupArg(t, tt.registry, tt.lookupType) if gotFound != tt.wantFound { t.Errorf("argRegistry.getArg() found = %v, want %v", gotFound, tt.wantFound) return @@ -257,7 +257,7 @@ func TestArgRegistry_getArg_coverageGaps(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, gotFound := tt.registry.getArg(tt.lookupType) + _, gotFound := lookupArg(t, tt.registry, tt.lookupType) if gotFound != tt.wantFound { t.Errorf("argRegistry.getArg() found = %v, want %v", gotFound, tt.wantFound) } @@ -316,12 +316,12 @@ func TestArgRegistry_getArg_edgeCases(t *testing.T) { wantFound: false, }, { - name: "interface assignability - value types not directly assignable", + name: "registered wider interface fills a narrower interface parameter", registry: argRegistry{ reflect.TypeOf((*derivedInterface)(nil)).Elem(): reflect.ValueOf(fullImpl{}), }, lookupType: reflect.TypeOf((*baseInterface)(nil)).Elem(), - wantFound: false, + wantFound: true, }, { name: "type stored with exact match in loop", @@ -347,7 +347,7 @@ func TestArgRegistry_getArg_edgeCases(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, gotFound := tt.registry.getArg(tt.lookupType) + _, gotFound := lookupArg(t, tt.registry, tt.lookupType) if gotFound != tt.wantFound { t.Errorf("argRegistry.getArg() found = %v, want %v", gotFound, tt.wantFound) } @@ -387,13 +387,13 @@ func TestArgRegistry_integration(t *testing.T) { } // Verify retrieval - if v, ok := registry.getArg(reflect.TypeOf(s1)); !ok || v.Interface().(*testStruct).Value != "first" { + if v, ok := lookupArg(t, registry, reflect.TypeOf(s1)); !ok || v.Interface().(*testStruct).Value != "first" { t.Error("Failed to retrieve pointer type") } // When we ask for testStruct, getArg will find *testStruct first and dereference it // This is the designed behavior - it prefers finding compatible types through conversion - if v, ok := registry.getArg(reflect.TypeOf(s2)); ok { + if v, ok := lookupArg(t, registry, reflect.TypeOf(s2)); ok { // Check if value is correct if v.CanInterface() && v.Type() == reflect.TypeOf(s2) { val := v.Interface().(testStruct) @@ -408,9 +408,9 @@ func TestArgRegistry_integration(t *testing.T) { t.Error("Failed to retrieve non-pointer type") } - // Test that interface retrieval fails because we didn't store interface type - if _, ok := registry.getArg(reflect.TypeOf((*testInterface)(nil)).Elem()); ok { - t.Error("Should not retrieve interface type when implementation was stored") + // An interface parameter takes the registered implementation + if _, ok := lookupArg(t, registry, reflect.TypeOf((*testInterface)(nil)).Elem()); !ok { + t.Error("Should retrieve the registered implementation for its interface type") } } @@ -427,7 +427,7 @@ func TestArgRegistry_assignability(t *testing.T) { // Try to retrieve as pointer to interface - should trigger pt.AssignableTo(t) path ptrType := reflect.TypeOf((*customInterface)(nil)) - if v, ok := registry.getArg(ptrType); ok { + if v, ok := lookupArg(t, registry, ptrType); ok { t.Logf("Successfully retrieved pointer to interface: %v", v.Type()) } @@ -440,7 +440,7 @@ func TestArgRegistry_assignability(t *testing.T) { // Try to retrieve as interface - should trigger st.AssignableTo(t) path ifaceType := reflect.TypeOf((*customInterface)(nil)).Elem() - if v, ok := registry2.getArg(ifaceType); ok { + if v, ok := lookupArg(t, registry2, ifaceType); ok { t.Logf("Successfully retrieved as interface: %v", v.Type()) } } @@ -454,7 +454,7 @@ func TestArgRegistry_getArg_unaddressable(t *testing.T) { // Try to get a pointer to int - should fail because value is not addressable ptrType := reflect.TypeOf((*int)(nil)) - _, ok := registry.getArg(ptrType) + _, ok := lookupArg(t, registry, ptrType) if ok { t.Error("Expected not to find pointer type for unaddressable value") } @@ -469,7 +469,7 @@ func TestArgRegistry_getArg_extendedAssignability(t *testing.T) { registry[reflect.TypeOf(ch)] = reflect.ValueOf(ch) // Exact match should work - v, ok := registry.getArg(reflect.TypeOf(ch)) + v, ok := lookupArg(t, registry, reflect.TypeOf(ch)) if !ok { t.Error("Expected to find channel type") } @@ -489,7 +489,7 @@ func TestArgRegistry_getArg_needsPtr(t *testing.T) { // Now look up a pointer type - this should trigger needsPtr = true and v.Addr() ptrType := reflect.TypeOf(&testStruct{}) - v, ok := registry.getArg(ptrType) + v, ok := lookupArg(t, registry, ptrType) if !ok { t.Fatal("Expected to find value") } @@ -531,7 +531,7 @@ func TestArgRegistry_getArg_assignabilityLoop(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - _, found := registry.getArg(tt.lookupType) + _, found := lookupArg(t, registry, tt.lookupType) if found != tt.wantFound { t.Errorf("getArg() found = %v, want %v", found, tt.wantFound) } @@ -566,7 +566,7 @@ func TestArgRegistry_getArg_assignabilityPaths(t *testing.T) { // Since we're looking for a pointer (but needsPtr=false because it's already a pointer type) // This should trigger line 64: return v, true ifacePtrType := reflect.TypeOf((*simpleInterface)(nil)) - v, ok := registry.getArg(ifacePtrType) + v, ok := lookupArg(t, registry, ifacePtrType) // This test demonstrates that the current logic doesn't find matches // when looking for interface pointers from concrete implementations @@ -587,7 +587,7 @@ func TestArgRegistry_getArg_assignabilityPaths(t *testing.T) { // st = simpleInterface, the loop will check if simpleInterface is assignable to simpleImpl // Since needsElem=false, this should trigger line 70: return v, true ifaceType := reflect.TypeOf((*simpleInterface)(nil)).Elem() - v, ok := registry.getArg(ifaceType) + v, ok := lookupArg(t, registry, ifaceType) if ok { t.Logf("Found value of type %v", v.Type()) @@ -610,7 +610,7 @@ func TestArgRegistry_getArg_assignabilityPaths(t *testing.T) { // The loop will find the string type and check if *string is assignable to string // Since needsPtr=true but the value is not addressable, it will continue stringPtrType := reflect.TypeOf((*string)(nil)) - _, ok := registry.getArg(stringPtrType) + _, ok := lookupArg(t, registry, stringPtrType) if ok { t.Error("Should not find pointer type for unaddressable value") } @@ -627,7 +627,7 @@ func TestArgRegistry_getArg_assignabilityPaths(t *testing.T) { registry[reflect.TypeOf((*simpleInterface)(nil))] = reflect.ValueOf(&super).Elem() // Look up by pointer to SubType - v, ok := registry.getArg(reflect.TypeOf(subPtr)) + v, ok := lookupArg(t, registry, reflect.TypeOf(subPtr)) if ok { t.Logf("Line 64 path: found %v", v.Type()) } @@ -637,7 +637,7 @@ func TestArgRegistry_getArg_assignabilityPaths(t *testing.T) { var superVal simpleInterface = simpleImpl{data: "val"} registry2[reflect.TypeOf((*simpleInterface)(nil)).Elem()] = reflect.ValueOf(superVal) - v2, ok2 := registry2.getArg(reflect.TypeOf(simpleImpl{})) + v2, ok2 := lookupArg(t, registry2, reflect.TypeOf(simpleImpl{})) if ok2 { t.Logf("Line 70 path: found %v", v2.Type()) } @@ -668,7 +668,7 @@ func TestArgRegistry_getArg_assignability(t *testing.T) { // *simpleImpl is assignable to simpleInterface, but the stored value // is not addressable, so it should skip and return not found ptrType := reflect.TypeOf(&simpleImpl{}) - _, ok := registry.getArg(ptrType) + _, ok := lookupArg(t, registry, ptrType) if ok { t.Error("Should not find pointer when stored value is not addressable") } @@ -685,7 +685,7 @@ func TestArgRegistry_getArg_assignability(t *testing.T) { // Look up the same interface type - should find via exact match ifaceType := reflect.TypeOf((*simpleInterface)(nil)).Elem() - _, ok := registry.getArg(ifaceType) + _, ok := lookupArg(t, registry, ifaceType) if !ok { t.Error("Should find interface type") } @@ -699,7 +699,7 @@ func TestArgRegistry_getArg_assignability(t *testing.T) { // Now try to find by interface - this won't work because // interface is not assignable to *simpleImpl - v2, ok2 := registry2.getArg(ifaceType) + v2, ok2 := lookupArg(t, registry2, ifaceType) if ok2 { // If found, verify it implements the interface if _, implements := v2.Interface().(simpleInterface); !implements { @@ -725,7 +725,7 @@ func TestArgRegistry_getArg_remainingPaths(t *testing.T) { // Look up pointer to same type - this goes through assignability loop // because exact match fails, then it checks assignability ptrType := reflect.TypeOf(&simpleImpl{}) - v, ok := registry.getArg(ptrType) + v, ok := lookupArg(t, registry, ptrType) if !ok { t.Error("Expected to find pointer via assignability") } else if v.Kind() != reflect.Pointer { @@ -748,7 +748,7 @@ func TestArgRegistry_getArg_remainingPaths(t *testing.T) { // The loop will find baseType first, check pt.AssignableTo(t) = true // But v.CanAddr() = false, so it continues ptrType := reflect.TypeOf(&baseType{}) - _, ok := registry.getArg(ptrType) + _, ok := lookupArg(t, registry, ptrType) if ok { t.Error("Should not find pointer when only non-addressable value exists") } @@ -783,7 +783,7 @@ func TestArgRegistry_getArg_remainingPaths(t *testing.T) { // Actually, we need to find a case where a non-pointer type is assignable to something // This is rare in Go, but let's try with type aliases ifaceType := reflect.TypeOf((*simpleInterface)(nil)).Elem() - v, ok := registry2.getArg(ifaceType) + v, ok := lookupArg(t, registry2, ifaceType) if ok { t.Logf("Found: %v", v.Type()) if v.Kind() == reflect.Pointer { @@ -813,3 +813,14 @@ func TestArgRegistry_getArg_remainingPaths(t *testing.T) { t.Log("Remaining uncovered paths are theoretical edge cases in Go's type system") }) } + +// lookupArg calls getArg and fails the test on an ambiguity error, for +// lookups that expect a single answer or none. +func lookupArg(t *testing.T, args argRegistry, typ reflect.Type) (reflect.Value, bool) { + t.Helper() + v, ok, err := args.getArg(typ) + if err != nil { + t.Fatalf("getArg(%s): %v", typ, err) + } + return v, ok +} diff --git a/di_interface_test.go b/di_interface_test.go new file mode 100644 index 0000000..2c01b82 --- /dev/null +++ b/di_interface_test.go @@ -0,0 +1,155 @@ +package structpages + +import ( + "bytes" + "context" + "fmt" + "io" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// namer is a consumer-side interface: a Props method declares only what it +// needs, so a unit test can call it with a fake. +type namer interface{ Name() string } + +type pgStore struct{ name string } + +func (s *pgStore) Name() string { return s.name } + +type memCache struct{} + +func (*memCache) Name() string { return "cache" } + +type namerPage struct{} + +func (namerPage) Props(n namer) (string, error) { return n.Name(), nil } +func (namerPage) Page(name string) component { return testComponent{name} } + +type namerRoot struct { + Namer namerPage `route:"/namer Namer"` +} + +func TestInjection_interfaceParamTakesTheOneImplementation(t *testing.T) { + mux := http.NewServeMux() + if _, err := Mount(mux, namerRoot{}, "/", "App", WithArgs(&pgStore{name: "pg"})); err != nil { + t.Fatalf("Mount: %v", err) + } + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/namer", http.NoBody)) + if rec.Code != http.StatusOK || rec.Body.String() != "pg" { + t.Errorf("GET /namer = %d %q, want 200 %q", rec.Code, rec.Body.String(), "pg") + } +} + +func TestInjection_twoImplementationsFailMount(t *testing.T) { + _, err := Mount(http.NewServeMux(), namerRoot{}, "/", "App", + WithArgs(&pgStore{name: "pg"}, &memCache{})) + if err == nil { + t.Fatal("Mount succeeded; want an ambiguity error") + } + for _, want := range []string{"page Namer", "namer", "*structpages.memCache, *structpages.pgStore"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("Mount error = %v, want it to contain %q", err, want) + } + } +} + +// concreteNamerPage asks for the concrete type, which is registered +// exactly, so a second implementation of namer doesn't make it ambiguous. +type concreteNamerPage struct{} + +func (concreteNamerPage) Props(s *pgStore) (string, error) { return s.Name(), nil } +func (concreteNamerPage) Page(name string) component { return testComponent{name} } + +func TestInjection_exactRegistrationWins(t *testing.T) { + type root struct { + Concrete concreteNamerPage `route:"/concrete Concrete"` + } + mux := http.NewServeMux() + if _, err := Mount(mux, root{}, "/", "App", WithArgs(&pgStore{name: "pg"}, &memCache{})); err != nil { + t.Fatalf("Mount: %v", err) + } + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/concrete", http.NoBody)) + if rec.Body.String() != "pg" { + t.Errorf("GET /concrete = %q, want %q", rec.Body.String(), "pg") + } +} + +// writerPropsPage asks for an io.Writer, which the per-request +// http.ResponseWriter fills before any registration is considered. +type writerPropsPage struct{} + +func (writerPropsPage) Props(out io.Writer) (string, error) { + _, isResponse := out.(http.ResponseWriter) + return fmt.Sprint(isResponse), nil +} +func (writerPropsPage) Page(got string) component { return testComponent{got} } + +func TestInjection_requestValueWinsOverRegistrations(t *testing.T) { + type root struct { + Writer writerPropsPage `route:"/w Writer"` + } + mux := http.NewServeMux() + // Two registered io.Writers would be ambiguous, but the request's + // writer fills the parameter first. + _, err := Mount(mux, root{}, "/", "App", WithArgs(&bytes.Buffer{}, &strings.Builder{})) + if err != nil { + t.Fatalf("Mount: %v", err) + } + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/w", http.NoBody)) + if rec.Body.String() != "true" { + t.Errorf("GET /w: Props got the response writer = %q, want %q", rec.Body.String(), "true") + } +} + +// writerHandler's own w already takes the request's writer, so its extra +// io.Writer comes from the registry, where two candidates are ambiguous. +// Mount must reject it, as the request would fail. +type writerHandler struct{} + +func (writerHandler) ServeHTTP(w http.ResponseWriter, _ *http.Request, out io.Writer) error { + _, err := io.WriteString(out, "") + return err +} + +func TestInjection_eachRequestValueFillsOneParameter(t *testing.T) { + type root struct { + Handler writerHandler `route:"/h Handler"` + } + _, err := Mount(http.NewServeMux(), root{}, "/", "App", WithArgs(&bytes.Buffer{}, &strings.Builder{})) + if err == nil || !strings.Contains(err.Error(), "io.Writer is implemented by more than one registered value") { + t.Errorf("Mount error = %v, want io.Writer ambiguity", err) + } +} + +// With nothing registered, namerPage's namer is missing. That stays a +// request-time error: a page may be mounted in a configuration that doesn't +// use it. +func TestInjection_missingDependencyFailsTheRequestNotMount(t *testing.T) { + mux := http.NewServeMux() + if _, err := Mount(mux, namerRoot{}, "/", "App"); err != nil { + t.Fatalf("Mount: %v", err) + } + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/namer", http.NoBody)) + if rec.Code != http.StatusInternalServerError { + t.Errorf("GET /namer = %d, want 500", rec.Code) + } +} + +// TestInjection_parseDoesNotCheck pins that Parse, used by tests that only +// need URLFor/ID, doesn't require Props dependencies to be registered. +func TestInjection_parseDoesNotCheck(t *testing.T) { + sp, err := Parse(namerRoot{}, "/", "App") + if err != nil { + t.Fatalf("Parse: %v", err) + } + if _, err := URLFor(sp.PageContext(context.Background()), namerPage{}); err != nil { + t.Errorf("URLFor: %v", err) + } +} diff --git a/docs/dependency-injection.md b/docs/dependency-injection.md index dc321b7..6bc6c0a 100644 --- a/docs/dependency-injection.md +++ b/docs/dependency-injection.md @@ -63,10 +63,26 @@ Each `WithArgs` value is registered under its type, and a parameter takes the re ``` - **Pointers fill values, not the reverse.** A registered `*AppContext` also fills a parameter of type `AppContext`. A registered `AppContext` value can't fill an `*AppContext` parameter, so register the pointer. -- **Declare the type you registered, not an interface.** A concrete registration doesn't fill an interface-typed parameter: registering `*PostgresStore` doesn't satisfy a `Store` interface parameter. (The request-scoped values in the table do fill their interface types: the writer arrives as `http.ResponseWriter`.) +- **An interface parameter takes the one registration that implements it.** A method can declare just what it needs, so a unit test can call it with a fake: + + ```go + type userLister interface { + ListUsers(ctx context.Context) ([]User, error) + } + + // Mount registers *PostgresStore, which implements userLister. + func (p usersPage) Props(r *http.Request, users userLister) (UsersProps, error) { … } + + // In a test, call Props directly with a fake. + props, err := usersPage{}.Props(req, fakeUsers{}) + ``` + + If two registrations implement the interface, `Mount` fails and names both: declare the concrete type, or narrow the interface. A registration of the parameter's exact type, and the per-request values in the table, are always used first. - **Generic types work** — type parameters, slices and maps as dependencies, aliases, function types and complex constraints. Anywhere these docs say "type", read it as "any reflect-distinguishable type". -A parameter that matches nothing fails the call with `method X requires argument of type T, but not found`. +`Mount` checks every `Props` and `ServeHTTP` parameter for ambiguity up front, so two registrations implementing one interface parameter fail at startup with the page and both types named, not on the first request to that page. `Parse` skips the check. + +A parameter that matches nothing fails the request with `method X requires argument of type T, but not found`. That isn't checked at `Mount`, so a page can stay mounted in a configuration, or a test, that doesn't register its dependencies. ## Init diff --git a/generics_injection_test.go b/generics_injection_test.go index 16cb761..e290d35 100644 --- a/generics_injection_test.go +++ b/generics_injection_test.go @@ -622,31 +622,19 @@ func TestGenerics_MethodMatching(t *testing.T) { } } -// Test interface injection limitations with generics +// TestGenerics_InterfaceInjection pins that a generic interface parameter +// takes the registered implementation, as a non-generic one does. func TestGenerics_InterfaceInjection(t *testing.T) { - // This test documents a known limitation: when you store an interface type - // in the argRegistry, Go's reflection shows the concrete type, not the interface. - // This means Props methods need to accept the concrete type, not the interface. - - t.Run("interface stored as concrete type", func(t *testing.T) { - var repo repository[string] = &memoryRepository[string]{ - items: make(map[string]string), - } - - // Even though repo is declared as an interface, reflect.TypeOf shows concrete type - concreteType := reflect.TypeOf(repo).String() - if !strings.Contains(concreteType, "memoryRepository") { - t.Errorf("Expected concrete type memoryRepository, got %s", concreteType) - } - - // This is why interface injection doesn't work as expected with generics - t.Log("Interface variables show concrete type in reflection:", concreteType) - }) - - // Workaround: use concrete types in Props methods - t.Run("workaround using concrete types", func(t *testing.T) { - // Instead of accepting repository[T], accept *memoryRepository[T] - // This is what we did in the productList.Props method - t.Log("Workaround: Props methods should accept concrete types, not interface types") - }) + repo := &memoryRepository[string]{items: make(map[string]string)} + args := argRegistry{} + if err := args.addArg(repo); err != nil { + t.Fatalf("addArg: %v", err) + } + got, ok := lookupArg(t, args, reflect.TypeFor[repository[string]]()) + if !ok { + t.Fatal("repository[string] parameter not filled by the registered *memoryRepository[string]") + } + if got.Interface() != repo { + t.Errorf("filled with %v, want the registered repository", got.Interface()) + } } diff --git a/parse.go b/parse.go index 4d46814..81d3fb3 100644 --- a/parse.go +++ b/parse.go @@ -261,7 +261,10 @@ func (p *parseContext) fillMethodArgs( } // If not found in available args, try the registry - val, ok := p.args.getArg(argType) + val, ok, err := p.args.getArg(argType) + if err != nil { + return fmt.Errorf("method %s: %w", formatMethod(method), err) + } if !ok { return fmt.Errorf("method %s requires argument of type %s, but not found", formatMethod(method), argType.String()) @@ -271,6 +274,30 @@ func (p *parseContext) fillMethodArgs( return nil } +// checkMethodArgs reports a parameter of method on pn that more than one +// registration could fill. It models fillMethodArgs by type, ahead of any +// call: each provided value fills one parameter, an exact type match before +// an assignable one, and only the rest reach the registry. A parameter the +// registry can't fill is not reported; that stays a request-time error. +func (p *parseContext) checkMethodArgs(pn *PageNode, method *reflect.Method, provided []reflect.Type) error { + unused := append(slices.Clip(provided), reflect.TypeFor[*PageNode](), reflect.TypeFor[PageNode]()) + for i := 1; i < method.Type.NumIn(); i++ { + argType := method.Type.In(i) + j := slices.Index(unused, argType) + if j < 0 { + j = slices.IndexFunc(unused, func(t reflect.Type) bool { return t.AssignableTo(argType) }) + } + if j >= 0 { + unused = slices.Delete(unused, j, j+1) + continue + } + if _, _, err := p.args.getArg(argType); err != nil { + return fmt.Errorf("page %s: method %s: %w", pn.Name, formatMethod(method), err) + } + } + return nil +} + // findMatchingArg tries to find a matching argument from available args func (p *parseContext) findMatchingArg( argType reflect.Type, diff --git a/parse_test.go b/parse_test.go index 2bfca05..a221b20 100644 --- a/parse_test.go +++ b/parse_test.go @@ -250,7 +250,7 @@ func Test_parseContext_getArg(t *testing.T) { t.Fatalf("Failed to add arg: %v", err) } } - got, ok := args.getArg(tt.in) + got, ok := lookupArg(t, args, tt.in) if !ok { t.Errorf("getArg() did not find type %v", tt.in) return diff --git a/skills/structpages/SKILL.md b/skills/structpages/SKILL.md index decf8a4..a87a771 100644 --- a/skills/structpages/SKILL.md +++ b/skills/structpages/SKILL.md @@ -377,9 +377,10 @@ sp, err := structpages.Mount(mux, pages{}, "/", "App", Registered values are matched by type into `Props`, `ServeHTTP`, `Middlewares`, `Init` and DI-injected components. Each type may be registered once (use named types to register two values of one type). A registered pointer also fills a value parameter, but a registered value does not fill a pointer -parameter, so register services as pointers. Parameters match the registered concrete type; an -interface parameter is not filled by a registered implementation (only the built-in -`http.ResponseWriter` and `RenderTarget` are). `*PageNode` is always injectable. Application services go here, not in globals. +parameter, so register services as pointers. An interface parameter takes the one registration that +implements it, so a `Props` can declare a narrow interface and a test can call it with a fake; two +implementations fail `Mount` (`Parse` doesn't check). A missing dependency fails the request, not `Mount`. +`*PageNode` is always injectable. Application services go here, not in globals. ## 8. Testing renders with a bare context diff --git a/skills/structpages/reference.md b/skills/structpages/reference.md index 1a908e6..ec0afb8 100644 --- a/skills/structpages/reference.md +++ b/skills/structpages/reference.md @@ -14,8 +14,9 @@ func Parse(page any, route, title string, options ...Option) (*StructPages, erro `http.DefaultServeMux`). `route` is the base path (usually `"/"`), `title` the root page title. `Parse` builds the same tree without registering anything; use it in tests and tooling. It applies -`WithArgs`, `WithURLPrefix` and `WithTargetSelector`; middleware and error-handler options are inert. It -currently ignores `WithMaxIDLength`, so ids generated under `Parse` use the default budget of 40. +`WithArgs`, `WithURLPrefix`, `WithTargetSelector` and `WithMaxIDLength`, so its ids match `Mount`'s; +middleware and error-handler options are inert. Unlike `Mount`, it doesn't check `Props` and +`ServeHTTP` parameters for ambiguous dependencies. ```go type Mux interface { @@ -144,7 +145,7 @@ component. | Option | Effect | |---|---| -| `WithArgs(args ...any)` | DI registry. Each type once; duplicates fail `Mount`. | +| `WithArgs(args ...any)` | DI registry. Each type once; duplicates fail `Mount`. An interface parameter takes the one registration implementing it. A parameter two registrations could fill fails `Mount`; one nothing provides fails the request. | | `WithErrorHandler(func(http.ResponseWriter, *http.Request, error))` | Renders every error from `Props`, `ServeHTTP`, component lookup and render. Default: plain 500. | | `WithMiddlewares(mw ...MiddlewareFunc)` | Global middleware, first is outermost, runs before page `Middlewares`. | | `WithTargetSelector(TargetSelector)` | Chooses the `RenderTarget`. Default `HTMXRenderTarget`; use `HTMXv4RenderTarget` for htmx 4. | diff --git a/struct_pages.go b/struct_pages.go index b69df16..0c68405 100644 --- a/struct_pages.go +++ b/struct_pages.go @@ -147,6 +147,9 @@ func Mount(mux Mux, page any, route, title string, options ...Option) (*StructPa if err := sp.parse(page, route, title); err != nil { return nil, err } + if err := sp.checkInjection(); err != nil { + return nil, err + } // Register all pages middlewares := append([]MiddlewareFunc{withPcCtx(sp.pc), extractURLParams}, sp.middlewares...) @@ -478,9 +481,38 @@ var ( errHandlerType = reflect.TypeOf((*httpErrHandler)(nil)).Elem() ) -func (sp *StructPages) asHandler(pn *PageNode) http.Handler { - v := pn.Value - st, pt := v.Type(), v.Type() +// requestArgTypes are the declared types of the values every Props and +// extended ServeHTTP call receives per request, besides the *PageNode every +// injected method gets. A parameter one of them can fill never reaches the +// registry. +var requestArgTypes = []reflect.Type{ + reflect.TypeFor[http.ResponseWriter](), + reflect.TypeFor[*http.Request](), + reflect.TypeFor[RenderTarget](), +} + +// checkInjection reports every Props and extended ServeHTTP parameter in +// the tree that more than one registration could fill, so the ambiguity +// fails Mount instead of the first request to that page. A parameter +// nothing fills is left to request time: a page may be mounted but unused +// in a configuration that doesn't register its dependencies. +func (sp *StructPages) checkInjection() error { + var errs []error + for pn := range sp.pc.root.All() { + if m, ok := pn.Props["Props"]; ok && m.Func.IsValid() { + errs = append(errs, sp.pc.checkMethodArgs(pn, &m, requestArgTypes)) + } + if m, ok := serveHTTPMethod(pn); ok && isExtendedServeHTTP(&m) { + errs = append(errs, sp.pc.checkMethodArgs(pn, &m, requestArgTypes)) + } + } + return errors.Join(errs...) +} + +// serveHTTPMethod returns pn's own (not promoted) ServeHTTP method, on +// the value or the pointer receiver. +func serveHTTPMethod(pn *PageNode) (reflect.Method, bool) { + st, pt := pn.Value.Type(), pn.Value.Type() if st.Kind() == reflect.Pointer { st = st.Elem() } else { @@ -490,9 +522,24 @@ func (sp *StructPages) asHandler(pn *PageNode) http.Handler { if !ok || isPromotedMethod(&method) { method, ok = pt.MethodByName("ServeHTTP") if !ok || isPromotedMethod(&method) { - return nil + return reflect.Method{}, false } } + return method, true +} + +// isExtendedServeHTTP reports whether method is the ServeHTTP form that +// takes parameters beyond the writer and request, filled by injection. +func isExtendedServeHTTP(method *reflect.Method) bool { + return method.Type.NumIn() > 3 // receiver, http.ResponseWriter, *http.Request +} + +func (sp *StructPages) asHandler(pn *PageNode) http.Handler { + v := pn.Value + method, ok := serveHTTPMethod(pn) + if !ok { + return nil + } if v.Type().Implements(handlerType) { return v.Interface().(http.Handler) @@ -517,7 +564,7 @@ func (sp *StructPages) asHandler(pn *PageNode) http.Handler { }) } // extended ServeHTTP method with extra arguments - if method.Type.NumIn() > 3 { // receiver, http.ResponseWriter, *http.Request + if isExtendedServeHTTP(&method) { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { var wv reflect.Value // ResponseWriter, will be buffered if handler returns error var bw *buffered