From 69e2c873d863052fab74b94cd374f04513771bdf Mon Sep 17 00:00:00 2001 From: mleku Date: Wed, 3 Dec 2025 06:04:50 +0000 Subject: [PATCH] Refactor for interface clarity and dependency isolation. Replaced inline interface literals with dedicated, documented interface definitions in `pkg/interfaces/`. Introduced `TimeoutError`, `PolicyChecker`, and `Neo4jResultIterator` interfaces to clarify design, improve maintainability, and resolve potential circular dependencies. Updated config and constant usage rules for consistency. Incremented version to v0.31.11. --- .claude/skills/golang/SKILL.md | 63 ++++++++++++++++++++++ CLAUDE.md | 69 +++++++++++++++++++++++-- cmd/subscription-test-simple/main.go | 3 +- cmd/subscription-test/main.go | 3 +- pkg/acl/acl.go | 10 ++-- pkg/interfaces/acl/acl.go | 7 +++ pkg/interfaces/neterr/neterr.go | 8 +++ pkg/interfaces/resultiter/resultiter.go | 16 ++++++ pkg/neo4j/query-events.go | 14 ++--- pkg/version/version | 2 +- relay-tester/client.go | 5 +- 11 files changed, 177 insertions(+), 23 deletions(-) create mode 100644 pkg/interfaces/neterr/neterr.go create mode 100644 pkg/interfaces/resultiter/resultiter.go diff --git a/.claude/skills/golang/SKILL.md b/.claude/skills/golang/SKILL.md index b0ffb81..d30ca64 100644 --- a/.claude/skills/golang/SKILL.md +++ b/.claude/skills/golang/SKILL.md @@ -82,6 +82,49 @@ func (f *File) Read(p []byte) (n int, err error) { } ``` +### Interface Design - CRITICAL RULES + +**Rule 1: Define interfaces in a dedicated package (e.g., `pkg/interfaces//`)** +- Interfaces provide isolation between packages and enable dependency inversion +- Keeping interfaces in a dedicated package prevents circular dependencies +- Each interface package should be minimal (just the interface, no implementations) + +**Rule 2: NEVER use type assertions with interface literals** +- **NEVER** write `.(interface{ Method() Type })` - this is non-idiomatic and unmaintainable +- Interface literals cannot be documented, tested for satisfaction, or reused + +```go +// BAD - interface literal in type assertion (NEVER DO THIS) +if checker, ok := obj.(interface{ Check() bool }); ok { + checker.Check() +} + +// GOOD - use defined interface from dedicated package +import "myproject/pkg/interfaces/checker" + +if c, ok := obj.(checker.Checker); ok { + c.Check() +} +``` + +**Rule 3: Resolving Circular Dependencies** +- If a circular dependency occurs, move the interface to `pkg/interfaces/` +- The implementing type stays in its original package +- The consuming code imports only the interface package +- Pattern: + ``` + pkg/interfaces/foo/ <- interface definition (no dependencies) + ↑ ↑ + pkg/bar/ pkg/baz/ + (implements) (consumes via interface) + ``` + +**Rule 4: Verify interface satisfaction at compile time** +```go +// Add this line to ensure *MyType implements MyInterface +var _ MyInterface = (*MyType)(nil) +``` + ### Concurrency Use goroutines and channels for concurrent programming: @@ -178,6 +221,26 @@ For detailed information, consult the reference files: - Start comments with the name being described - Use godoc format +6. **Configuration - CRITICAL** + - **NEVER** use `os.Getenv()` scattered throughout packages + - **ALWAYS** centralize environment variable parsing in a single config package (e.g., `app/config/`) + - Pass configuration via structs, not by reading environment directly + - This ensures discoverability, documentation, and testability of all config options + +7. **Constants - CRITICAL** + - **ALWAYS** define named constants for values used more than a few times + - **ALWAYS** define named constants if multiple packages depend on the same value + - Constants shared across packages belong in a dedicated package (e.g., `pkg/constants/`) + - Magic numbers and strings are forbidden + ```go + // BAD - magic number + if size > 1024 { + + // GOOD - named constant + const MaxBufferSize = 1024 + if size > MaxBufferSize { + ``` + ## Common Commands ```bash diff --git a/CLAUDE.md b/CLAUDE.md index 8af3a93..05deb60 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -346,10 +346,11 @@ export ORLY_AUTH_TO_WRITE=false # Require auth only for writes 4. Events stored via `database.SaveEvent()` 5. Active subscriptions notified via `publishers.Publish()` -**Configuration System:** +**Configuration System - CRITICAL RULES:** - Uses `go-simpler.org/env` for struct tags -- **IMPORTANT: ALL environment variables MUST be defined in `app/config/config.go`** - - Never use `os.Getenv()` directly in packages - always pass config via structs +- **ALL environment variables MUST be defined in `app/config/config.go`** + - **NEVER** use `os.Getenv()` directly in packages - always pass config via structs + - **NEVER** parse environment variables outside of `app/config/` - This ensures all config options appear in `./orly help` output - Database backends receive config via `database.DatabaseConfig` struct - Use `GetDatabaseConfigValues()` helper to extract DB config from app config @@ -358,6 +359,21 @@ export ORLY_AUTH_TO_WRITE=false # Require auth only for writes - Default data directory: `~/.local/share/ORLY` - Database-specific config (Neo4j, DGraph, Badger) is passed via `DatabaseConfig` struct in `pkg/database/factory.go` +**Constants - CRITICAL RULES:** +- **ALWAYS** define named constants for values used more than a few times +- **ALWAYS** define named constants if multiple packages depend on the same value +- Constants shared across packages should be in a dedicated package (e.g., `pkg/constants/`) +- Magic numbers and strings are forbidden - use named constants with clear documentation +- Example: + ```go + // BAD - magic number + if timeout > 30 { + + // GOOD - named constant + const DefaultTimeoutSeconds = 30 + if timeout > DefaultTimeoutSeconds { + ``` + **Event Publishing:** - `pkg/protocol/publish/` manages publisher registry - Each WebSocket connection registers its subscriptions @@ -394,6 +410,53 @@ export ORLY_AUTH_TO_WRITE=false # Require auth only for writes ``` - This optimization saves memory and enables faster comparisons in the database layer +**Interface Design - CRITICAL RULES:** + +**Rule 1: ALL interfaces MUST be defined in `pkg/interfaces//`** +- Interfaces provide isolation between packages and enable dependency inversion +- Keeping interfaces in a dedicated package prevents circular dependencies +- Each interface package should be minimal (just the interface, no implementations) + +**Rule 2: NEVER use type assertions with interface literals** +- **NEVER** write `.(interface{ Method() Type })` - this is non-idiomatic and unmaintainable +- Interface literals cannot be documented, tested for satisfaction, or reused +- Example of WRONG approach: + ```go + // BAD - interface literal in type assertion + if checker, ok := obj.(interface{ Check() bool }); ok { + checker.Check() + } + ``` +- Example of CORRECT approach: + ```go + // GOOD - use defined interface from pkg/interfaces/ + import "next.orly.dev/pkg/interfaces/checker" + + if c, ok := obj.(checker.Checker); ok { + c.Check() + } + ``` + +**Rule 3: Resolving Circular Dependencies** +- If a circular dependency occurs when adding an interface, move the interface to `pkg/interfaces/` +- The implementing type stays in its original package +- The consuming code imports only the interface package +- This pattern: + ``` + pkg/interfaces/foo/ <- interface definition (no dependencies) + ↑ ↑ + pkg/bar/ pkg/baz/ + (implements) (consumes via interface) + ``` + +**Existing interfaces in `pkg/interfaces/`:** +- `acl/` - ACL and PolicyChecker interfaces +- `neterr/` - TimeoutError interface for network errors +- `resultiter/` - Neo4jResultIterator for database results +- `store/` - Storage-related interfaces +- `publisher/` - Event publishing interfaces +- `typer/` - Type identification interface + ## Development Workflow ### Making Changes to Web UI diff --git a/cmd/subscription-test-simple/main.go b/cmd/subscription-test-simple/main.go index 2ceb1c0..7d0542c 100644 --- a/cmd/subscription-test-simple/main.go +++ b/cmd/subscription-test-simple/main.go @@ -12,6 +12,7 @@ import ( "time" "github.com/gorilla/websocket" + "next.orly.dev/pkg/interfaces/neterr" ) var ( @@ -90,7 +91,7 @@ func main() { if ctx.Err() != nil { return } - if netErr, ok := err.(interface{ Timeout() bool }); ok && netErr.Timeout() { + if netErr, ok := err.(neterr.TimeoutError); ok && netErr.Timeout() { continue } log.Printf("Read error: %v", err) diff --git a/cmd/subscription-test/main.go b/cmd/subscription-test/main.go index 3c26ff6..e9060d9 100644 --- a/cmd/subscription-test/main.go +++ b/cmd/subscription-test/main.go @@ -13,6 +13,7 @@ import ( "time" "github.com/gorilla/websocket" + "next.orly.dev/pkg/interfaces/neterr" ) var ( @@ -123,7 +124,7 @@ func main() { } // Check for timeout errors (these are expected during idle periods) - if netErr, ok := err.(interface{ Timeout() bool }); ok && netErr.Timeout() { + if netErr, ok := err.(neterr.TimeoutError); ok && netErr.Timeout() { consecutiveTimeouts++ if consecutiveTimeouts >= maxConsecutiveTimeouts { log.Printf("Too many consecutive read timeouts (%d), connection may be dead", consecutiveTimeouts) diff --git a/pkg/acl/acl.go b/pkg/acl/acl.go index cc22442..f2c070c 100644 --- a/pkg/acl/acl.go +++ b/pkg/acl/acl.go @@ -2,20 +2,20 @@ package acl import ( "git.mleku.dev/mleku/nostr/encoders/event" - "next.orly.dev/pkg/interfaces/acl" + acliface "next.orly.dev/pkg/interfaces/acl" "next.orly.dev/pkg/utils/atomic" ) var Registry = &S{} type S struct { - ACL []acl.I + ACL []acliface.I Active atomic.String } type A struct{ S } -func (s *S) Register(i acl.I) { +func (s *S) Register(i acliface.I) { (*s).ACL = append((*s).ACL, i) } @@ -85,9 +85,7 @@ func (s *S) CheckPolicy(ev *event.E) (allowed bool, err error) { for _, i := range s.ACL { if i.Type() == s.Active.Load() { // Check if the ACL implementation has a CheckPolicy method - if policyChecker, ok := i.(interface { - CheckPolicy(ev *event.E) (allowed bool, err error) - }); ok { + if policyChecker, ok := i.(acliface.PolicyChecker); ok { return policyChecker.CheckPolicy(ev) } // If no CheckPolicy method, default to allowing diff --git a/pkg/interfaces/acl/acl.go b/pkg/interfaces/acl/acl.go index 88f065b..adaa36e 100644 --- a/pkg/interfaces/acl/acl.go +++ b/pkg/interfaces/acl/acl.go @@ -2,6 +2,7 @@ package acl import ( + "git.mleku.dev/mleku/nostr/encoders/event" "next.orly.dev/pkg/interfaces/typer" ) @@ -31,3 +32,9 @@ type I interface { Syncer() typer.T } + +// PolicyChecker is an optional interface that ACL implementations can implement +// to provide custom event policy checking beyond basic access level checks. +type PolicyChecker interface { + CheckPolicy(ev *event.E) (allowed bool, err error) +} diff --git a/pkg/interfaces/neterr/neterr.go b/pkg/interfaces/neterr/neterr.go new file mode 100644 index 0000000..77fd412 --- /dev/null +++ b/pkg/interfaces/neterr/neterr.go @@ -0,0 +1,8 @@ +// Package neterr defines interfaces for network error handling. +package neterr + +// TimeoutError is an interface for errors that can indicate a timeout. +// This is compatible with net.Error's Timeout() method. +type TimeoutError interface { + Timeout() bool +} diff --git a/pkg/interfaces/resultiter/resultiter.go b/pkg/interfaces/resultiter/resultiter.go new file mode 100644 index 0000000..34eb778 --- /dev/null +++ b/pkg/interfaces/resultiter/resultiter.go @@ -0,0 +1,16 @@ +// Package resultiter defines interfaces for iterating over database query results. +package resultiter + +import ( + "context" + + "github.com/neo4j/neo4j-go-driver/v5/neo4j" +) + +// Neo4jResultIterator defines the interface for iterating over Neo4j query results. +// This is implemented by both neo4j.Result and CollectedResult types. +type Neo4jResultIterator interface { + Next(context.Context) bool + Record() *neo4j.Record + Err() error +} diff --git a/pkg/neo4j/query-events.go b/pkg/neo4j/query-events.go index aff5461..5ea77ad 100644 --- a/pkg/neo4j/query-events.go +++ b/pkg/neo4j/query-events.go @@ -5,12 +5,12 @@ import ( "fmt" "strings" - "github.com/neo4j/neo4j-go-driver/v5/neo4j" - "next.orly.dev/pkg/database/indexes/types" "git.mleku.dev/mleku/nostr/encoders/event" "git.mleku.dev/mleku/nostr/encoders/filter" "git.mleku.dev/mleku/nostr/encoders/hex" "git.mleku.dev/mleku/nostr/encoders/tag" + "next.orly.dev/pkg/database/indexes/types" + "next.orly.dev/pkg/interfaces/resultiter" "next.orly.dev/pkg/interfaces/store" ) @@ -186,14 +186,10 @@ func (n *N) parseEventsFromResult(result any) ([]*event.E, error) { events := make([]*event.E, 0) ctx := context.Background() - // Type assert to the interface we actually use - resultIter, ok := result.(interface { - Next(context.Context) bool - Record() *neo4j.Record - Err() error - }) + // Type assert to the result iterator interface + resultIter, ok := result.(resultiter.Neo4jResultIterator) if !ok { - return nil, fmt.Errorf("invalid result type") + return nil, fmt.Errorf("invalid result type: expected resultiter.Neo4jResultIterator") } // Iterate through result records diff --git a/pkg/version/version b/pkg/version/version index 990bd2d..a97e4f4 100644 --- a/pkg/version/version +++ b/pkg/version/version @@ -1 +1 @@ -v0.31.10 \ No newline at end of file +v0.31.11 \ No newline at end of file diff --git a/relay-tester/client.go b/relay-tester/client.go index 5260a73..f308471 100644 --- a/relay-tester/client.go +++ b/relay-tester/client.go @@ -7,9 +7,10 @@ import ( "time" "github.com/gorilla/websocket" - "lol.mleku.dev/errorf" "git.mleku.dev/mleku/nostr/encoders/event" "git.mleku.dev/mleku/nostr/encoders/hex" + "lol.mleku.dev/errorf" + "next.orly.dev/pkg/interfaces/neterr" ) // Client wraps a WebSocket connection to a relay for testing. @@ -118,7 +119,7 @@ func (c *Client) readLoop() { default: } // Check if it's a timeout - connection might still be alive - if netErr, ok := err.(interface{ Timeout() bool }); ok && netErr.Timeout() { + if netErr, ok := err.(neterr.TimeoutError); ok && netErr.Timeout() { // Pong handler should have extended deadline, but if we timeout, // reset it and continue - connection might still be alive // This can happen during idle periods when no messages are received