chore: parse safety for api #801
Conversation
| } | ||
|
|
||
| export type ApiResponse = ClientFeaturesDelta | ClientFeaturesResponse; | ||
| export type ApiResponse = |
There was a problem hiding this comment.
We have no natural discriminator on this but we control the parsing code so thanos_fine_ill_do_it_myself.png
There was a problem hiding this comment.
If you'd like to avoid the type property injection you could also use TS type predicates.
There was a problem hiding this comment.
I don't have strong feelings to be honest. In the spirit of getting a bunch of stuff tested I'm going to merge this. But if you do have feelings here absolutely give me a shout and I'll move this to type predicates in another PR
| Array.isArray(delta.events) | ||
| ) { | ||
| return delta as ClientFeaturesDelta; | ||
| export const parseApiResponse = (data: unknown): ApiResponse => { |
There was a problem hiding this comment.
Parsing deltas is more or less unchanged. I think we should do more but this is probably robust enough since this originates in either Edge or Unleash. The full client response gets some actual validation rather than yeeting any which was what was previously happening
| } else if ('features' in data && Array.isArray(data.features)) { | ||
| return { ...data, type: 'full' } as ClientFeaturesResponse & { type: 'full' }; | ||
| } | ||
| throw new Error( |
There was a problem hiding this comment.
This is trapped in either the fetcher, resulting in an error message or the stream code, which triggers a failover to polling
| this.segments = this.createSegmentLookup(response.segments); | ||
| break; | ||
| } | ||
| default: { |
There was a problem hiding this comment.
Since we control the parse code, this can never occur. If someone yeets in any any from the outside, the parse code will reject before we get here. So this is just compiler sugar for warning a future developer when they break this invariant
There was a problem hiding this comment.
My instinct would be to assume full as default, but your approach is more explicit and probably better.
if (isDelta(response)) {
// do delta things
return;
}
// do full thingsThere was a problem hiding this comment.
This should be a system invariant now. I want "guessing what shape stuff is" to stay in the parsing layers so that we only guess in one place and don't litter our code with poorly understood branches
|
|
||
| if (content && this.notEmpty(content)) { | ||
| await this.save(content, false); | ||
| await this.save(parseApiResponse(content), false); |
There was a problem hiding this comment.
We have two choices here. Either we de-structure and & with {'type': 'full'} or we throw this at the parsing code and let it figure it. Given that this is user defined, I'm going with option 2
| ); | ||
|
|
||
| return obj; | ||
| private convertToMap(features: FeatureInterface[] | undefined | null): FeatureToggleData { |
There was a problem hiding this comment.
This isn't relevant to this PR but gosh this function is just cursed. I think this is clearer and functionally the same thing (it's still cursed, it's just clear why it's cursed)
There was a problem hiding this comment.
Would it be clearer if we early returned in case of empty or invalid features?
if (!features?.length) return {};
// do the thingThere was a problem hiding this comment.
Sure, seems like a reasonable thing to do, patched.
Honestly though, this function needs... well... the reasons for its existence need to go away before we can make this code sane
| return; | ||
| } | ||
| await this.options.onSave(data, true); | ||
| await this.options.onSave(parseApiResponse(data), true); |
There was a problem hiding this comment.
Aw yiss, parse at IO boundary so we don't let goop into the heart of our system. I think this is safe because the parse code isn't a deep check and doesn't reject anything that wouldn't break later anyway
Pull Request Test Coverage Report for Build 19887381135Details
💛 - Coveralls |
3 participants
Cleans up some of the parsing (and lack thereof) code higher up in the flow so that it occurs on IO boundaries rather than lurking like some suspicious octopus in the repository layers.
I've done as much as I have the courage to do here, some of this is cursed but I don't want to break things in the wild