Skip to content

Commit 04a2c66

Browse files
NamyalgNamyalg
committed
chore: minor refactors and updates to comments, address review #686 (comment) 
1 parent fd5d9c2 commit 04a2c66

8 files changed

+368
-97
lines changed

‎remoteconfig/condition_evaluator.go

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -148,11 +148,13 @@ func (ce *conditionEvaluator) evaluatePercentCondition(percentCondition *percent
148148
}
149149

150150
func computeInstanceMicroPercentile(seed string, randomizationID string) uint32 {
151-
seedPrefix := ""
151+
var sb strings.Builder
152152
if len(seed) > 0 {
153-
seedPrefix = fmt.Sprintf("%s.", seed)
153+
sb.WriteString(seed)
154+
sb.WriteRune('.')
154155
}
155-
stringToHash := fmt.Sprintf("%s%s", seedPrefix, randomizationID)
156+
sb.WriteString(randomizationID)
157+
stringToHash := sb.String()
156158

157159
hash := sha256.New()
158160
hash.Write([]byte(stringToHash))

‎remoteconfig/condition_evaluator_test.go

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -296,41 +296,41 @@ func TestPercentConditionMicroPercent(t *testing.T) {
296296
}{
297297
{
298298
description: "Evaluate LESS_OR_EQUAL to true when MicroPercent is max",
299-
operator: "LESS_OR_EQUAL",
299+
operator: lessThanOrEqual,
300300
microPercent: 100_000_000,
301301
outcome: true,
302302
},
303303
{
304304
description: "Evaluate LESS_OR_EQUAL to false when MicroPercent is min",
305-
operator: "LESS_OR_EQUAL",
305+
operator: lessThanOrEqual,
306306
microPercent: 0,
307307
outcome: false,
308308
},
309309
{
310310
description: "Evaluate LESS_OR_EQUAL to false when MicroPercent is not set (MicroPercent should use zero)",
311-
operator: "LESS_OR_EQUAL",
311+
operator: lessThanOrEqual,
312312
outcome: false,
313313
},
314314
{
315315
description: "Evaluate GREATER_THAN to true when MicroPercent is not set (MicroPercent should use zero)",
316-
operator: "GREATER_THAN",
316+
operator: greaterThan,
317317
outcome: true,
318318
},
319319
{
320320
description: "Evaluate GREATER_THAN max to false",
321-
operator: "GREATER_THAN",
321+
operator: greaterThan,
322322
outcome: false,
323323
microPercent: 100_000_000,
324324
},
325325
{
326326
description: "Evaluate LESS_OR_EQUAL to 9571542 to true",
327-
operator: "LESS_OR_EQUAL",
327+
operator: lessThanOrEqual,
328328
microPercent: 9_571_542, // instanceMicroPercentile of abcdef.123 (testSeed.testRandomizationID) is 9_571_542
329329
outcome: true,
330330
},
331331
{
332332
description: "Evaluate greater than 9571542 to true",
333-
operator: "GREATER_THAN",
333+
operator: greaterThan,
334334
microPercent: 9_571_541, // instanceMicroPercentile of abcdef.123 (testSeed.testRandomizationID) is 9_571_542
335335
outcome: true,
336336
},
@@ -361,40 +361,40 @@ func TestPercentConditionMicroPercentRange(t *testing.T) {
361361
}{
362362
{
363363
description: "Evaluate to false when microPercentRange is not set",
364-
operator: "BETWEEN",
364+
operator: between,
365365
outcome: false,
366366
},
367367
{
368368
description: "Evaluate to false when upper bound is not set",
369369
microPercentLb: 0,
370-
operator: "BETWEEN",
370+
operator: between,
371371
outcome: false,
372372
},
373373
{
374374
description: "Evaluate to true when lower bound is not set and upper bound is max",
375375
microPercentUb: 100_000_000,
376-
operator: "BETWEEN",
376+
operator: between,
377377
outcome: true,
378378
},
379379
{
380380
description: "Evaluate to true when between lower and upper bound", // instanceMicroPercentile of abcdef.123 (testSeed.testRandomizationID) is 9_571_542
381381
microPercentLb: 9_000_000,
382382
microPercentUb: 9_571_542, // interval is (9_000_000, 9_571_542]
383-
operator: "BETWEEN",
383+
operator: between,
384384
outcome: true,
385385
},
386386
{
387387
description: "Evaluate to false when lower and upper bounds are equal",
388388
microPercentLb: 98_000_000,
389389
microPercentUb: 98_000_000,
390-
operator: "BETWEEN",
390+
operator: between,
391391
outcome: false,
392392
},
393393
{
394394
description: "Evaluate to false when not between 9_400_000 and 9_500_000", // instanceMicroPercentile of abcdef.123 (testSeed.testRandomizationID) is 9_571_542
395395
microPercentLb: 9_400_000,
396396
microPercentUb: 9_500_000,
397-
operator: "BETWEEN",
397+
operator: between,
398398
outcome: false,
399399
},
400400
}

‎remoteconfig/remoteconfig.go

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
// See the License for the specific language governing permissions and
1313
// limitations under the License.
1414

15-
// Package remoteconfig allows clients to use Firebase Remote Config with Go.
15+
// Package remoteconfig provides functions to fetch and evaluate a server-side Remote Config template.
1616
package remoteconfig
1717

1818
import (
@@ -67,6 +67,7 @@ func newRcClient(client *internal.HTTPClient, conf *internal.RemoteConfigClientC
6767
internal.WithHeader("x-goog-api-client", internal.GetMetricsHeader(conf.Version)),
6868
}
6969

70+
// Handles errors for non-success HTTP status codes from Remote Config servers.
7071
client.CreateErrFn = handleRemoteConfigError
7172

7273
return &rcClient{
@@ -77,7 +78,7 @@ func newRcClient(client *internal.HTTPClient, conf *internal.RemoteConfigClientC
7778
}
7879
}
7980

80-
// GetServerTemplate Initializes a new ServerTemplate instance and fetches the server template.
81+
// GetServerTemplate initializes a new ServerTemplate instance and fetches the server template.
8182
func (c *rcClient) GetServerTemplate(ctx context.Context,
8283
defaultConfig map[string]any) (*ServerTemplate, error) {
8384
template, err := c.InitServerTemplate(defaultConfig, "")

‎remoteconfig/server_config.go

Lines changed: 31 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@
1515
package remoteconfig
1616

1717
import (
18+
"slices"
1819
"strconv"
1920
"strings"
2021
)
@@ -26,8 +27,8 @@ type ValueSource int
2627
const (
2728
sourceUnspecified ValueSource = iota
2829
Static // Static represents a statically defined value.
29-
Remote // Default represents a default value.
30-
Default // Remote represents a value fetched from a remote source.
30+
Remote // Remote represents a value fetched from a remote source.
31+
Default // Default represents a default value.
3132
)
3233

3334
// Value defines the interface for configuration values.
@@ -38,39 +39,50 @@ type value struct {
3839

3940
// Default Values for config parameters.
4041
const (
41-
DefaultValueForBoolean = false
42-
DefaultValueForString = ""
43-
DefaultValueForNumber = 0
42+
defaultValueForBoolean = false
43+
defaultValueForString = ""
44+
defaultValueForNumber = 0
4445
)
4546

4647
var booleanTruthyValues = []string{"1", "true", "t", "yes", "y", "on"}
4748

4849
// ServerConfig is the implementation of the ServerConfig interface.
4950
type ServerConfig struct {
50-
ConfigValues map[string]value
51+
configValues map[string]value
5152
}
5253

5354
// NewServerConfig creates a new ServerConfig instance.
54-
func NewServerConfig(configValues map[string]value) *ServerConfig {
55-
return &ServerConfig{ConfigValues: configValues}
55+
func newServerConfig(configValues map[string]value) *ServerConfig {
56+
return &ServerConfig{configValues: configValues}
5657
}
5758

5859
// GetBoolean returns the boolean value associated with the given key.
60+
//
61+
// It returns true if the string value is "1", "true", "t", "yes", "y", or "on" (case-insensitive).
62+
// Otherwise, or if the key is not found, it returns the default boolean value (false).
5963
func (s *ServerConfig) GetBoolean(key string) bool {
6064
return s.getValue(key).asBoolean()
6165
}
6266

6367
// GetInt returns the integer value associated with the given key.
68+
//
69+
// If the parameter value cannot be parsed as an integer, or if the key is not found,
70+
// it returns the default numeric value (0).
6471
func (s *ServerConfig) GetInt(key string) int {
6572
return s.getValue(key).asInt()
6673
}
6774

6875
// GetFloat returns the float value associated with the given key.
76+
//
77+
// If the parameter value cannot be parsed as a float64, or if the key is not found,
78+
// it returns the default float value (0).
6979
func (s *ServerConfig) GetFloat(key string) float64 {
7080
return s.getValue(key).asFloat()
7181
}
7282

7383
// GetString returns the string value associated with the given key.
84+
//
85+
// If the key is not found, it returns the default string value ("").
7486
func (s *ServerConfig) GetString(key string) string {
7587
return s.getValue(key).asString()
7688
}
@@ -82,16 +94,16 @@ func (s *ServerConfig) GetValueSource(key string) ValueSource {
8294

8395
// getValue returns the value associated with the given key.
8496
func (s *ServerConfig) getValue(key string) *value {
85-
if val, ok := s.ConfigValues[key]; ok {
97+
if val, ok := s.configValues[key]; ok {
8698
return &val
8799
}
88-
return newValue(Static, "")
100+
return newValue(Static, defaultValueForString)
89101
}
90102

91103
// newValue creates a new value instance.
92104
func newValue(source ValueSource, customValue string) *value {
93105
if customValue == "" {
94-
customValue = DefaultValueForString
106+
customValue = defaultValueForString
95107
}
96108
return &value{source: source, value: customValue}
97109
}
@@ -104,41 +116,35 @@ func (v *value) asString() string {
104116
// asBoolean returns the value as a boolean.
105117
func (v *value) asBoolean() bool {
106118
if v.source == Static {
107-
return DefaultValueForBoolean
108-
}
109-
110-
for _, truthyValue := range booleanTruthyValues {
111-
if strings.ToLower(v.value) == truthyValue {
112-
return true
113-
}
119+
return defaultValueForBoolean
114120
}
115121

116-
return false
122+
return slices.Contains(booleanTruthyValues, strings.ToLower(v.value))
117123
}
118124

119125
// asInt returns the value as an integer.
120126
func (v *value) asInt() int {
121127
if v.source == Static {
122-
return DefaultValueForNumber
128+
return defaultValueForNumber
123129
}
124130
num, err := strconv.Atoi(v.value)
125131

126132
if err != nil {
127-
return DefaultValueForNumber
133+
return defaultValueForNumber
128134
}
129135

130136
return num
131137
}
132138

133-
// asFloat returns the value as an integer.
139+
// asFloat returns the value as a float.
134140
func (v *value) asFloat() float64 {
135141
if v.source == Static {
136-
return DefaultValueForNumber
142+
return defaultValueForNumber
137143
}
138-
num, err := strconv.ParseFloat(v.value, 64)
144+
num, err := strconv.ParseFloat(v.value, doublePrecision)
139145

140146
if err != nil {
141-
return DefaultValueForNumber
147+
return defaultValueForNumber
142148
}
143149

144150
return num

‎remoteconfig/server_config_test.go

Lines changed: 111 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,111 @@
1+
package remoteconfig
2+
3+
import "testing"
4+
5+
type configGetterTestCase struct {
6+
name string
7+
key string
8+
expectedString string
9+
expectedInt int
10+
expectedBool bool
11+
expectedFloat float64
12+
expectedSource ValueSource
13+
}
14+
15+
func getTestConfig() ServerConfig {
16+
config := ServerConfig{
17+
configValues: map[string]value{
18+
paramOne: {
19+
value: valueOne,
20+
source: Default,
21+
},
22+
paramTwo: {
23+
value: valueTwo,
24+
source: Remote,
25+
},
26+
paramThree: {
27+
value: valueThree,
28+
source: Default,
29+
},
30+
paramFour: {
31+
value: valueFour,
32+
source: Remote,
33+
},
34+
},
35+
}
36+
return config
37+
}
38+
39+
func TestServerConfigGetters(t *testing.T) {
40+
config := getTestConfig()
41+
testCases := []configGetterTestCase{
42+
{
43+
name: "Parameter Value : String, Default Source",
44+
key: paramOne,
45+
expectedString: valueOne,
46+
expectedInt: 0,
47+
expectedBool: false,
48+
expectedFloat: 0,
49+
expectedSource: Default,
50+
},
51+
{
52+
name: "Parameter Value : JSON, Remote Source",
53+
key: paramTwo,
54+
expectedString: valueTwo,
55+
expectedInt: 0,
56+
expectedBool: false,
57+
expectedFloat: 0,
58+
expectedSource: Remote,
59+
},
60+
{
61+
name: "Unknown Parameter Value",
62+
key: "unknown_param",
63+
expectedString: "",
64+
expectedInt: 0,
65+
expectedBool: false,
66+
expectedFloat: 0,
67+
expectedSource: Static,
68+
},
69+
{
70+
name: "Parameter Value - Float, Default Source",
71+
key: paramThree,
72+
expectedString: "123456789.123",
73+
expectedInt: 0,
74+
expectedBool: false,
75+
expectedFloat: 123456789.123,
76+
expectedSource: Default,
77+
},
78+
{
79+
name: "Parameter Value - Boolean, Remote Source",
80+
key: paramFour,
81+
expectedString: "1",
82+
expectedInt: 1,
83+
expectedBool: true,
84+
expectedFloat: 1,
85+
expectedSource: Remote,
86+
},
87+
}
88+
for _, tc := range testCases {
89+
t.Run(tc.name, func(t *testing.T) {
90+
if got := config.GetString(tc.key); got != tc.expectedString {
91+
t.Errorf("GetString(%q): got %q, want %q", tc.key, got, tc.expectedString)
92+
}
93+
94+
if got := config.GetInt(tc.key); got != tc.expectedInt {
95+
t.Errorf("GetInt(%q): got %d, want %d", tc.key, got, tc.expectedInt)
96+
}
97+
98+
if got := config.GetBoolean(tc.key); got != tc.expectedBool {
99+
t.Errorf("GetBoolean(%q): got %t, want %t", tc.key, got, tc.expectedBool)
100+
}
101+
102+
if got := config.GetFloat(tc.key); got != tc.expectedFloat {
103+
t.Errorf("GetFloat(%q): got %f, want %f", tc.key, got, tc.expectedFloat)
104+
}
105+
106+
if got := config.GetValueSource(tc.key); got != tc.expectedSource {
107+
t.Errorf("GetValueSource(%q): got %v, want %v", tc.key, got, tc.expectedSource)
108+
}
109+
})
110+
}
111+
}

0 commit comments

Comments
 (0)