From 285a4582b02bae22d4541beb6254beb32ec8c934 Mon Sep 17 00:00:00 2001 From: Snider Date: Fri, 30 Jan 2026 19:02:30 +0000 Subject: [PATCH] fix(i18n): address thread-safety issues from code review - hooks.go: Use atomic.Value for missingKeyHandler global - loader.go: Use sync.Once for FSLoader.Languages() caching - service.go: Use atomic.Pointer[Service] for defaultService All globals that could race between goroutines now use proper synchronization primitives. Co-Authored-By: Claude Opus 4.5 --- pkg/i18n/FUTURE_PROOFING.md | 48 ------------------------------------- pkg/i18n/REVIEW.md | 38 ----------------------------- pkg/i18n/hooks.go | 15 ++++++++---- pkg/i18n/i18n_test.go | 4 ++-- pkg/i18n/loader.go | 35 ++++++++++++++------------- pkg/i18n/mode_test.go | 6 ++--- pkg/i18n/service.go | 17 +++++++++---- 7 files changed, 46 insertions(+), 117 deletions(-) delete mode 100644 pkg/i18n/FUTURE_PROOFING.md delete mode 100644 pkg/i18n/REVIEW.md diff --git a/pkg/i18n/FUTURE_PROOFING.md b/pkg/i18n/FUTURE_PROOFING.md deleted file mode 100644 index 79eb204..0000000 --- a/pkg/i18n/FUTURE_PROOFING.md +++ /dev/null @@ -1,48 +0,0 @@ -# Future-Proofing Status - -This document tracks architectural decisions made to ensure the `pkg/i18n` package is resilient to future requirements. - -**Last Updated:** 30 January 2026 -**Status:** Core Complete - -## 1. Extensibility: The "Magic Namespace" Problem - -**Status: ✅ IMPLEMENTED** - -* **Solution:** The `KeyHandler` interface and middleware chain have been implemented. -* **Details:** "Magic" keys like `i18n.label.*` are now handled by specific structs in `handler.go`. The core `T()` method iterates through these handlers. -* **Benefit:** New patterns can be added via `AddHandler()` without modifying the core package. - -## 2. API Design: Context & Disambiguation - -**Status: ✅ IMPLEMENTED** - -* **Solution:** `TranslationContext` struct and `C()` builder have been created in `context.go`. -* **Details:** `getEffectiveFormality()` now checks `*TranslationContext` for formality hints, in addition to `*Subject`. -* **Benefit:** Translations can now be disambiguated via context, and formality can be set per-call without needing a Subject. - -## 3. Storage: Interface-Driven Loading - -**Status: ✅ IMPLEMENTED** - -* **Solution:** The `Loader` interface has been defined in `types.go`, and the default JSON logic moved to `FSLoader` in `loader.go`. -* **Details:** `NewWithLoader` allows injecting any backend. -* **Benefit:** Applications can now load translations from databases, remote APIs, or other file formats. - -## 4. Standardization: Pluralization & CLDR - -**Status: ⏳ PENDING** - -* **Current State:** The package still uses a custom `pluralRules` map in `types.go`. -* **Recommendation:** When the need arises for more languages, replace the internal `pluralRules` map with a call to `golang.org/x/text/feature/plural` or a similar standard library wrapper. The current interface hides this implementation detail, so it can be swapped later without breaking changes. - -## 5. Data Format: Vendor Compatibility - -**Status: ⏳ ENABLED** - -* **Current State:** The default format is still the custom nested JSON. -* **Future Path:** Thanks to the `Loader` interface, we can now implement a `PoLoader` or `ArbLoader` to support standard translation formats used by professional vendors, without changing the core service. - -## Summary - -The critical architectural risks (coupling, storage, and context) have been resolved. The remaining item (Pluralization standard) is an implementation detail that can be addressed incrementally without breaking the public API. \ No newline at end of file diff --git a/pkg/i18n/REVIEW.md b/pkg/i18n/REVIEW.md deleted file mode 100644 index 491d210..0000000 --- a/pkg/i18n/REVIEW.md +++ /dev/null @@ -1,38 +0,0 @@ -# Code Review: i18n Package (Refactored) - -## Executive Summary -The `pkg/i18n` package has undergone a significant refactoring that addresses previous architectural concerns. The introduction of a `Loader` interface and a `KeyHandler` middleware chain has transformed a monolithic service into a modular, extensible system. The file structure is now logical and intuitive. - -## Status: Excellent -The package is now in a state that strongly supports future growth without breaking changes. The code is clean, idiomatic, and follows Go best practices. - -## Improvements Verified - -* **Modular Architecture:** The "magic" namespace logic (e.g., `i18n.label.*`) has been successfully extracted from the core `T()` method into a chain of `KeyHandler` implementations (`handler.go`). This allows for easy extension or removal of these features. -* **Storage Agnosticism:** The new `Loader` interface and `NewWithLoader` constructor decouple the service from the filesystem, allowing for future backends (Database, API, etc.) without API breakage. -* **Logical File Structure:** - * `service.go`: Core service logic (moved from `interfaces.go`). - * `loader.go`: Data loading and flattening (renamed from `mutate.go`). - * `hooks.go`: Callback logic (renamed from `actions.go`). - * `handler.go`: Middleware logic (new). - * `types.go`: Shared interfaces and types (new). -* **Type Safety:** Shared types (`Mode`, `Formality`, etc.) are centralized in `types.go`, improving discoverability. - -## Remaining/New Observations - -| Issue | Severity | Location | Recommendation | -|-------|----------|----------|----------------| -| **Context Integration** | Minor | `service.go` | `TranslationContext` is defined in `context.go` but not yet fully utilized in `resolveWithFallback`. The service checks `Subject` for formality, but doesn't appear to check `TranslationContext` yet. | -| **Handler Performance** | Trivial | `handler.go` | The handler chain is iterated for every `T()` call. For high-performance hot loops, ensure the chain length remains reasonable (current default of 6 is fine). | - -## Recommendations - -1. **Wire up Context:** - * Update `Service.getEffectiveFormality` (and similar helper methods) to check for `*TranslationContext` in addition to `*Subject`. - * This will fully activate the features defined in `context.go`. - -2. **Unit Tests:** - * Ensure the new `handler.go` and `loader.go` have dedicated test coverage (files `handler_test.go` and `loader_test.go` exist, which is good). - -3. **Documentation:** - * Update package-level examples in `i18n.go` to show how to use `WithHandlers` or custom Loaders if advanced usage is expected. \ No newline at end of file diff --git a/pkg/i18n/hooks.go b/pkg/i18n/hooks.go index 2babe49..6694d9c 100644 --- a/pkg/i18n/hooks.go +++ b/pkg/i18n/hooks.go @@ -3,25 +3,32 @@ package i18n import ( "runtime" + "sync/atomic" ) -var missingKeyHandler MissingKeyHandler +var missingKeyHandler atomic.Value // stores MissingKeyHandler // OnMissingKey registers a handler for missing translation keys. // Called when T() can't find a key in ModeCollect. +// Thread-safe: can be called concurrently with translations. // // i18n.SetMode(i18n.ModeCollect) // i18n.OnMissingKey(func(m i18n.MissingKey) { // log.Printf("MISSING: %s at %s:%d", m.Key, m.CallerFile, m.CallerLine) // }) func OnMissingKey(h MissingKeyHandler) { - missingKeyHandler = h + missingKeyHandler.Store(h) } // dispatchMissingKey creates and dispatches a MissingKey event. // Called internally when a key is missing in ModeCollect. func dispatchMissingKey(key string, args map[string]any) { - if missingKeyHandler == nil { + v := missingKeyHandler.Load() + if v == nil { + return + } + h, ok := v.(MissingKeyHandler) + if !ok || h == nil { return } @@ -31,7 +38,7 @@ func dispatchMissingKey(key string, args map[string]any) { line = 0 } - missingKeyHandler(MissingKey{ + h(MissingKey{ Key: key, Args: args, CallerFile: file, diff --git a/pkg/i18n/i18n_test.go b/pkg/i18n/i18n_test.go index 931229f..a3b22a6 100644 --- a/pkg/i18n/i18n_test.go +++ b/pkg/i18n/i18n_test.go @@ -60,7 +60,7 @@ func TestSetLanguage(t *testing.T) { func TestDefaultService(t *testing.T) { // Reset default for test - defaultService = nil + defaultService.Store(nil) defaultOnce = sync.Once{} defaultErr = nil @@ -280,7 +280,7 @@ func TestDebugMode(t *testing.T) { t.Run("package-level SetDebug", func(t *testing.T) { // Reset default - defaultService = nil + defaultService.Store(nil) defaultOnce = sync.Once{} defaultErr = nil diff --git a/pkg/i18n/loader.go b/pkg/i18n/loader.go index 3d51973..56ca5da 100644 --- a/pkg/i18n/loader.go +++ b/pkg/i18n/loader.go @@ -7,6 +7,7 @@ import ( "io/fs" "path/filepath" "strings" + "sync" ) // FSLoader loads translations from a filesystem (embedded or disk). @@ -16,6 +17,7 @@ type FSLoader struct { // Cache of available languages (populated on first Languages() call) languages []string + langOnce sync.Once } // NewFSLoader creates a loader for the given filesystem and directory. @@ -66,25 +68,24 @@ func (l *FSLoader) Load(lang string) (map[string]Message, *GrammarData, error) { } // Languages implements Loader.Languages - returns available language codes. +// Thread-safe: uses sync.Once to ensure the directory is scanned only once. func (l *FSLoader) Languages() []string { - if l.languages != nil { - return l.languages - } - - entries, err := fs.ReadDir(l.fsys, l.dir) - if err != nil { - return nil - } - - for _, entry := range entries { - if entry.IsDir() || !strings.HasSuffix(entry.Name(), ".json") { - continue + l.langOnce.Do(func() { + entries, err := fs.ReadDir(l.fsys, l.dir) + if err != nil { + return } - lang := strings.TrimSuffix(entry.Name(), ".json") - // Normalise underscore to hyphen (en_GB -> en-GB) - lang = strings.ReplaceAll(lang, "_", "-") - l.languages = append(l.languages, lang) - } + + for _, entry := range entries { + if entry.IsDir() || !strings.HasSuffix(entry.Name(), ".json") { + continue + } + lang := strings.TrimSuffix(entry.Name(), ".json") + // Normalise underscore to hyphen (en_GB -> en-GB) + lang = strings.ReplaceAll(lang, "_", "-") + l.languages = append(l.languages, lang) + } + }) return l.languages } diff --git a/pkg/i18n/mode_test.go b/pkg/i18n/mode_test.go index c578af7..a57f4d1 100644 --- a/pkg/i18n/mode_test.go +++ b/pkg/i18n/mode_test.go @@ -82,13 +82,13 @@ func TestOnMissingKey(t *testing.T) { func TestServiceMode(t *testing.T) { // Reset default service after tests - originalService := defaultService + originalService := defaultService.Load() defer func() { - defaultService = originalService + defaultService.Store(originalService) }() t.Run("default mode is normal", func(t *testing.T) { - defaultService = nil + defaultService.Store(nil) defaultOnce = sync.Once{} defaultErr = nil diff --git a/pkg/i18n/service.go b/pkg/i18n/service.go index 840f77a..692ea1c 100644 --- a/pkg/i18n/service.go +++ b/pkg/i18n/service.go @@ -9,6 +9,7 @@ import ( "path/filepath" "strings" "sync" + "sync/atomic" "golang.org/x/text/language" ) @@ -75,7 +76,7 @@ func WithDebug(enabled bool) Option { // Default is the global i18n service instance. var ( - defaultService *Service + defaultService atomic.Pointer[Service] defaultOnce sync.Once defaultErr error ) @@ -148,22 +149,28 @@ func NewWithLoader(loader Loader, opts ...Option) (*Service, error) { // Init initializes the default global service. func Init() error { defaultOnce.Do(func() { - defaultService, defaultErr = New() + svc, err := New() + if err == nil { + defaultService.Store(svc) + } + defaultErr = err }) return defaultErr } // Default returns the global i18n service, initializing if needed. +// Thread-safe: can be called concurrently. func Default() *Service { - if defaultService == nil { + if defaultService.Load() == nil { _ = Init() } - return defaultService + return defaultService.Load() } // SetDefault sets the global i18n service. +// Thread-safe: can be called concurrently with Default(). func SetDefault(s *Service) { - defaultService = s + defaultService.Store(s) } // loadJSON parses nested JSON and flattens to dot-notation keys.