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 <noreply@anthropic.com>
This commit is contained in:
parent
3daa0c34db
commit
285a4582b0
7 changed files with 46 additions and 117 deletions
|
|
@ -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.
|
||||
|
|
@ -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.
|
||||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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.
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue