fix(ExperimentWhitelistService): Fix finding variation by key #155
Conversation
| return decision, nil | ||
| // TODO(Matt): Add a VariationsByKey map to the Experiment struct, and use it to look up Variation by key | ||
| for _, variation := range decisionContext.Experiment.Variations { | ||
| variation := variation |
There was a problem hiding this comment.
Why is this reassignment necessary? Can't you just use the variation assigned by the loop iterator?
There was a problem hiding this comment.
The linter forces it https://github.com/kyoh86/scopelint
There was a problem hiding this comment.
Oh... @pawels-optimizely thoughts on this? I won't block the merge on this 🙂
There was a problem hiding this comment.
Appears to be a valid catch for potentially hard to track down bugs. I also just learned something about go range loops :) In this particular case I believe we were safe since we're immediately returning with the current variation, but might as well be in the habit of "pinning".
There was a problem hiding this comment.
I don't think we need it here , but if linters are complaining then that's fine. that assignment is needed only for goroutines , and we are handling that in sidedoor properly : https://github.com/optimizely/sidedoor/blob/master/pkg/admin/handlers/admin_entities.go#L64
There was a problem hiding this comment.
@pawels-optimizely The assignment is not just needed for go routines, that is just a common place it can manifest itself. See the copies example in scopelint readme.
There was a problem hiding this comment.
yeah, goroutines are the most common example. I missed the reference (&) in Matt's code.
Any time you take the reference of a range you need that extra init. That's why I usually use the value (not the reference) coming from the range.
mikecdavis
left a comment
mikecdavis
left a comment
There was a problem hiding this comment.
Thanks for the turnaround on this! Minor comment on the test case.
| } | ||
|
|
||
| // Test an experiment whose key differs from its id | ||
| func (s *ExperimentWhitelistServiceTestSuite) TestWhitelistIncludesDecisionForExpWithKey() { |
There was a problem hiding this comment.
Not sure we need a new test for this or if we update the previous test fixture to have a differing id, key pair.
There was a problem hiding this comment.
Good point, I'll delete the new test and change the old test data.
| return decision, nil | ||
| // TODO(Matt): Add a VariationsByKey map to the Experiment struct, and use it to look up Variation by key | ||
| for _, variation := range decisionContext.Experiment.Variations { | ||
| variation := variation |
There was a problem hiding this comment.
Appears to be a valid catch for potentially hard to track down bugs. I also just learned something about go range loops :) In this particular case I believe we were safe since we're immediately returning with the current variation, but might as well be in the habit of "pinning".
pkg/decision/helpers_test.go
Outdated
| } | ||
|
|
||
| // Experiment with a whitelist, and variation ids are not the same as keys | ||
| const testExpWhitelistKey2 = "test_experiment_whitelist_2" |
There was a problem hiding this comment.
Similar comment above about not needing a new fixture.
…e different ID & Key for variations
Previously,
GetDecisionofExperimentWhitelistServicewas incorrectly using variation key to look up variations in theVariationsmap of anExperimentstruct, in which keys are variation IDs, not variation keys.With this fix, we iterate over variations to find the one with the desired key. To avoid needing to search all variations, we can add a map keyed by variation key as a follow up.