✨ feat: add support for application state management #3360
Conversation
efectn
requested review from
gaby,
sixcolors and
ReneWerner87
and removed request for
a team
|
Warning Rate limit exceeded@efectn has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 29 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis PR introduces application state management features by adding a new Changes
Sequence Diagram(s)sequenceDiagram
participant C as Client/Test
participant A as App
participant S as State
C->>A: Call State() to access state
A->>S: [State accessor] invoked
Note over S: Set/Update operations in State
C->>A: Call app.State().Set("key", "value")
A->>S: Execute Set("key", "value")
S-->>A: Acknowledge the set operation
C->>A: Call app.State().GetString("key")
A->>S: Execute Get("key")
S-->>A: Return "value" and status ok
A-->>C: Return "value"
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3360 +/- ##
==========================================
+ Coverage 83.61% 83.83% +0.21%
==========================================
Files 118 119 +1
Lines 11727 11818 +91
==========================================
+ Hits 9806 9908 +102
+ Misses 1491 1483 -8
+ Partials 430 427 -3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
app.go (1)
143-143: Fix typo in comment.There's a typo in the comment for StrictRouting - "Ftrue" should be "true".
- // When set to Ftrue, the router treats "/foo" and "/foo/" as different. + // When set to true, the router treats "/foo" and "/foo/" as different.state.go (3)
81-84: Fix method comment.The method comment incorrectly describes this as "MustGetString" but the method is actually "Delete".
-// MustGetString retrieves a string value from the State and panics if the key is not found. +// Delete removes a key-value pair from the State.
86-89: Fix method comment.The method comment incorrectly describes this as "Reset" but the method is actually "Clear".
-// Reset resets the State. +// Clear removes all key-value pairs from the State.
91-100: Add error handling for key type assertion.The Keys method assumes all keys are strings, but there's no error handling for the type assertion. This could potentially cause a panic if a non-string key was somehow stored in the map.
func (s *State) Keys() []string { keys := make([]string, 0) s.dependencies.Range(func(key, _ any) bool { - keys = append(keys, key.(string)) + if keyStr, ok := key.(string); ok { + keys = append(keys, keyStr) + } return true }) return keys }🧰 Tools
🪛 GitHub Check: lint
[failure] 95-95:
Error return value is not checked (errcheck)state_test.go (3)
153-159: Optimize struct field alignment.The testCase struct could be optimized for memory efficiency by reordering fields.
type testCase[T any] struct { - name string - key string - value any - expected T - ok bool + expected T + value any + name string + key string + ok bool }🧰 Tools
🪛 GitHub Check: lint
[failure] 153-153:
fieldalignment: struct with 64 pointer bytes could be 56 (govet)
161-169: Add t.Helper() to test helper function.The runGenericTest function should start with t.Helper() to improve test output when this helper fails.
func runGenericTest[T any](t *testing.T, getter func(*State, string) (T, bool), tests []testCase[T]) { + t.Helper() st := newState() for _, tc := range tests { st.Set(tc.key, tc.value) got, ok := getter(st, tc.key) require.Equal(t, tc.ok, ok, tc.name) require.Equal(t, tc.expected, got, tc.name) } }🧰 Tools
🪛 GitHub Check: lint
[failure] 161-161:
test helper function should start from t.Helper() (thelper)
73-87: Use require.InDelta for floating point comparisons.When comparing floating point values, it's better to use require.InDelta or require.InEpsilon instead of require.Equal to handle potential floating point precision issues.
func TestState_GetFloat64(t *testing.T) { t.Parallel() st := newState() st.Set("pi", 3.14) f, ok := st.GetFloat64("pi") require.True(t, ok) - require.Equal(t, 3.14, f) + require.InDelta(t, 3.14, f, 0.0001) // wrong type should return zero value st.Set("int", 10) f, ok = st.GetFloat64("int") require.False(t, ok) - require.Equal(t, 0.0, f) + require.InDelta(t, 0.0, f, 0.0001) }🧰 Tools
🪛 GitHub Check: lint
[failure] 86-86:
float-compare: use require.InEpsilon (or InDelta) (testifylint)
[failure] 80-80:
float-compare: use require.InEpsilon (or InDelta) (testifylint)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app.go(4 hunks)app_test.go(1 hunks)state.go(1 hunks)state_test.go(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: golangci-lint
app.go
[error] 90-90: fieldalignment: struct with 1304 pointer bytes could be 1232 (govet)
🪛 GitHub Check: codecov/patch
state.go
[warning] 36-36: state.go#L36
Added line #L36 was not covered by tests
[warning] 47-47: state.go#L47
Added line #L47 was not covered by tests
[warning] 58-58: state.go#L58
Added line #L58 was not covered by tests
[warning] 69-69: state.go#L69
Added line #L69 was not covered by tests
🪛 GitHub Check: lint
state.go
[failure] 95-95:
Error return value is not checked (errcheck)
[failure] 51-51:
confusing-results: unnamed results of the same type may be confusing, consider using named results (revive)
[failure] 3-3:
should only use grouped 'import' declarations (grouper)
state_test.go
[failure] 161-161:
test helper function should start from t.Helper() (thelper)
[failure] 153-153:
fieldalignment: struct with 64 pointer bytes could be 56 (govet)
[failure] 86-86:
float-compare: use require.InEpsilon (or InDelta) (testifylint)
[failure] 80-80:
float-compare: use require.InEpsilon (or InDelta) (testifylint)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Compare
- GitHub Check: unit (1.23.x, windows-latest)
- GitHub Check: unit (1.24.x, windows-latest)
- GitHub Check: repeated
🔇 Additional comments (11)
app_test.go (1)
1893-1901: LGTM! Good test for the new state functionality.The test correctly verifies the basic state management functionality by setting and retrieving a value, ensuring the expected behavior works as intended.
app.go (3)
132-133: LGTM! Good addition of the state field.Adding the state field to the App struct is a clean way to implement application-wide state management capabilities.
520-522: LGTM! Proper state initialization.The state is properly initialized during app creation, ensuring it's always available.
960-963: LGTM! Clean accessor method implementation.The State() method provides a clean interface for accessing the app's state from handlers.
state.go (5)
5-9: LGTM! Good use of sync.Map for thread-safety.Using sync.Map is a solid choice for the state implementation, as it provides built-in thread safety for concurrent access without manual locking.
11-26: LGTM! Clean implementation of core state functionality.The newState function and basic Set/Get methods are well-implemented with a clean API.
28-70: LGTM! Good implementation of type-specific getters.The type-specific getters (GetString, GetInt, GetBool, GetFloat64) follow a consistent pattern and properly handle type mismatches by returning false when the type doesn't match.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 36-36: state.go#L36
Added line #L36 was not covered by tests
[warning] 47-47: state.go#L47
Added line #L47 was not covered by tests
[warning] 58-58: state.go#L58
Added line #L58 was not covered by tests
[warning] 69-69: state.go#L69
Added line #L69 was not covered by tests🪛 GitHub Check: lint
[failure] 51-51:
confusing-results: unnamed results of the same type may be confusing, consider using named results (revive)
72-79: LGTM! Good implementation of MustGet.The MustGet method provides a convenient way to retrieve a value when the caller knows the key exists, with appropriate panic behavior for missing keys.
102-134: LGTM! Good implementation of helper methods and generic functions.The Len method and generic GetState/MustGetState functions provide a complete API for state management. The generic functions are particularly useful for type-safe state access.
state_test.go (2)
10-101: LGTM! Comprehensive test coverage.The tests thoroughly cover the basic state operations, including setting, getting, type mismatches, and error conditions.
🧰 Tools
🪛 GitHub Check: lint
[failure] 86-86:
float-compare: use require.InEpsilon (or InDelta) (testifylint)
[failure] 80-80:
float-compare: use require.InEpsilon (or InDelta) (testifylint)
171-423: LGTM! Excellent test coverage with benchmarks.The comprehensive test suite covers normal usage, edge cases, and includes benchmarks to ensure performance. The generic tests are particularly valuable for ensuring type safety across different data types.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Performance Alert ⚠️
Possible performance regression was detected for benchmark.
Benchmark result of this commit is worse than the previous benchmark result exceeding threshold 1.50.
| Benchmark suite | Current: 804e8f8 | Previous: 87f3f0c | Ratio |
|---|---|---|---|
Benchmark_Ctx_SendString_B |
15.64 ns/op 0 B/op 0 allocs/op |
9.379 ns/op 0 B/op 0 allocs/op |
1.67 |
Benchmark_Ctx_SendString_B - ns/op |
15.64 ns/op |
9.379 ns/op |
1.67 |
This comment was automatically generated by workflow using github-action-benchmark.
|
|
||
| // Keys retrieves all the keys from the State. | ||
| func (s *State) Keys() []string { | ||
| keys := make([]string, 0) |
There was a problem hiding this comment.
Here we can preallocate the keys slice using s.Len() to reduce the memory allocation; however, it will make the method a little bit slower.
|
Analysis created by Analysis of the Implementation1. Thread Safety and Usage of sync.Map
2. Naming and Exporting
3. Clear Method Implementation
4. Generic Get Methods
5. Additional Convenience Methods
6. Error Handling Philosophy
Summary of Improvements
These improvements enhance the usability, consistency, and robustness of the implementation while maintaining the thread safety provided by Implementation: package fiber
import (
"sync"
)
// State is a thread-safe global key-value store for app dependencies.
type State struct {
dependencies sync.Map
}
// NewState creates and returns a new instance of State.
func NewState() *State {
return &State{
dependencies: sync.Map{},
}
}
// Set sets a key-value pair in the State.
func (s *State) Set(key string, value any) {
s.dependencies.Store(key, value)
}
// Get retrieves a value from the State.
func (s *State) Get(key string) (any, bool) {
return s.dependencies.Load(key)
}
// GetString retrieves a string value from the State.
func (s *State) GetString(key string) (string, bool) {
dep, ok := s.Get(key)
if ok {
if depString, okCast := dep.(string); okCast {
return depString, true
}
}
return "", false
}
// GetInt retrieves an int value from the State.
func (s *State) GetInt(key string) (int, bool) {
dep, ok := s.Get(key)
if ok {
if depInt, okCast := dep.(int); okCast {
return depInt, true
}
}
return 0, false
}
// GetBool retrieves a bool value from the State.
func (s *State) GetBool(key string) (bool, bool) {
dep, ok := s.Get(key)
if ok {
if depBool, okCast := dep.(bool); okCast {
return depBool, true
}
}
return false, false
}
// GetFloat64 retrieves a float64 value from the State.
func (s *State) GetFloat64(key string) (float64, bool) {
dep, ok := s.Get(key)
if ok {
if depFloat64, okCast := dep.(float64); okCast {
return depFloat64, true
}
}
return 0, false
}
// MustGet retrieves a value from the State and panics if the key is not found.
func (s *State) MustGet(key string) any {
if dep, ok := s.Get(key); ok {
return dep
}
panic("state: dependency not found!")
}
// Delete removes a key from the State.
func (s *State) Delete(key string) {
s.dependencies.Delete(key)
}
// Clear removes all key-value pairs from the State.
func (s *State) Clear() {
s.dependencies.Range(func(key, _ any) bool {
s.dependencies.Delete(key)
return true
})
}
// Keys retrieves all the keys from the State.
func (s *State) Keys() []string {
keys := make([]string, 0)
s.dependencies.Range(func(key, _ any) bool {
if keyStr, ok := key.(string); ok {
keys = append(keys, keyStr)
}
return true
})
return keys
}
// Len returns the number of dependencies in the State.
func (s *State) Len() int {
length := 0
s.dependencies.Range(func(_, _ any) bool {
length++
return true
})
return length
}
// Has checks if the given key exists in the State.
func (s *State) Has(key string) bool {
_, ok := s.Get(key)
return ok
}
// GetState retrieves a value from the State, casting it to the desired type.
func (s *State) GetState[T any](key string) (T, bool) {
dep, ok := s.Get(key)
if ok {
if depT, okCast := dep.(T); okCast {
return depT, true
}
}
var zeroVal T
return zeroVal, false
}
// MustGetState retrieves a value from the State, casting it to the desired type.
// It panics if the key is not found.
func (s *State) MustGetState[T any](key string) T {
val, ok := s.GetState[T](key)
if !ok {
panic("state: dependency not found!")
}
return val
} |
|
Here's the implementation for GetWithDefault() // GetWithDefault retrieves a value from the State, casting it to the desired type.
// If the key is not found or the type assertion fails, it returns the provided default value.
func (s *State) GetWithDefault[T any](key string, defaultValue T) T {
if val, ok := s.Get(key); ok {
if typed, ok := val.(T); ok {
return typed
}
}
return defaultValue
} |
efectn
force-pushed
the
state-management
branch
from
95f0eec to
804e8f8
Compare
Description
This PR adds support for basic state management like FastAPI to Fiber using
sync.Map.Fixes #3077
Changes introduced
List the new features or adjustments introduced in this pull request. Provide details on benchmarks, documentation updates, changelog entries, and if applicable, the migration guide.
Type of change
Please delete options that are not relevant.
Checklist
Before you submit your pull request, please make sure you meet these requirements:
/docs/directory for Fiber's documentation.Commit formatting
Please use emojis in commit messages for an easy way to identify the purpose or intention of a commit. Check out the emoji cheatsheet here: CONTRIBUTING.md