Refactor for interface clarity and dependency isolation.
Some checks failed
Go / build-and-release (push) Has been cancelled

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.
This commit is contained in:
2025-12-03 06:04:50 +00:00
parent 6c7d55ff7e
commit 69e2c873d8
11 changed files with 177 additions and 23 deletions

View File

@@ -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/<name>/`)**
- 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

View File

@@ -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/<name>/`**
- 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

View File

@@ -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)

View File

@@ -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)

View File

@@ -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

View File

@@ -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)
}

View File

@@ -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
}

View File

@@ -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
}

View File

@@ -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

View File

@@ -1 +1 @@
v0.31.10
v0.31.11

View File

@@ -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