feat: Sync publishes streaming update #371
Conversation
| package model | ||
|
|
||
| // Event for individual flag overrides | ||
| type UpsertOverrideEvent struct { | ||
| FlagKey string | ||
| ProjectKey string | ||
| FlagState FlagState | ||
| } | ||
|
|
||
| // Event for full project sync | ||
| type SyncEvent struct { | ||
| ProjectKey string | ||
| AllFlagsState FlagsState | ||
| } |
There was a problem hiding this comment.
Pulled these into a catalog file of events.
| // Event for full project sync | ||
| type SyncEvent struct { | ||
| ProjectKey string | ||
| AllFlagsState FlagsState |
There was a problem hiding this comment.
This got renamed from FlagsState to AllFlagsState just to add a smidge of clarity
There was a problem hiding this comment.
Nice! It was very confusing.
| case model.SyncEvent: | ||
| clientFlags := clientFlags{} | ||
| for flagKey, flagState := range event.AllFlagsState { | ||
| clientFlags[flagKey] = clientFlag{ | ||
| Version: flagState.Version, | ||
| Value: flagState.Value, | ||
| } | ||
| } | ||
|
|
||
| err := SendMessage(c.updateChan, TYPE_PUT, clientFlags) | ||
| if err != nil { | ||
| panic(errors.Wrap(err, "failed to marshal flag state in observer")) |
There was a problem hiding this comment.
Tried to follow this format as closely as possible - https://launchdarkly.atlassian.net/wiki/spaces/PD/pages/521668284/SDK+streaming+specification#Client-side-%E2%80%9Cput%E2%80%9D-message
| type clientFlag struct { | ||
| Key string `json:"key,omitempty"` | ||
| Version int `json:"version"` | ||
| Value ldvalue.Value `json:"value"` | ||
| } | ||
|
|
||
| type clientFlags map[string]clientFlag |
There was a problem hiding this comment.
Declaring a new struct didn't feel super useful at the moment vs using omitempty. If we see significant delta between different event bodies some bifurcation would become necessary but for now 🤷
| func SendMessage( | ||
| updateChan chan<- Message, | ||
| msgType MessageType, | ||
| data interface{}, | ||
| ) error { | ||
| payload, err := json.Marshal(data) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| updateChan <- Message{ | ||
| Event: msgType, | ||
| Data: payload, | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Not really sure how / if to unit test this - I've used Go channels only sparingly.
On the other hand - emitting messages worked fine before this PR, and testing json.Marshal feels beyond superfluous
There was a problem hiding this comment.
Might be good to add to the integration tests for syncing the project.
RE: unit testing, you could do something like the following.
...
func TestSendMessage(t *testing.T) {
chan := make(chan Message, 1) // needs to be buffered or else you'll deadlock
err := SendMessage(chan, PUT_TYPE, "cat")
require.NoError(t, err)
message := <-chan
...do assertions
}I don't think it's super important to unit test this function: it doesn't really have any logic & it is already being exercised through the integration tests.
There was a problem hiding this comment.
I decided to implement the integration test for anyone reading this after the fact
| defer ld.GetFlagTracker().RemoveFlagValueChangeListener(flagUpdateChan) | ||
| trackers[flagKey] = &flagUpdateChan | ||
|
|
||
| _, err := store.UpsertOverride(ctx, model.Override{ |
There was a problem hiding this comment.
This is probably OK, but it is kinda a hack since we're calling the store directly in what's supposed to be more of an integration test (although not quite since we're mocking out the external stuff anyways).
WDYT about a more realistic scenario where we test resync before we test overrides and we change the mock response from the SDK to have different values and higher version numbers?
Co-authored-by: Mike Zorn <[email protected]>
Requirements
Related issues
Provide links to any issues in this repository or elsewhere relating to this pull request.
Describe the solution you've provided
Syncing a project triggers a
putSSE with all flags in the project. The code currently assumes only one project.Describe alternatives you've considered
Provide a clear and concise description of any alternative solutions or features you've considered.
Additional context
Add any other context about the pull request here.