Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: update all ports to support success + error union response #468 #475

Merged
merged 2 commits into from
Mar 2, 2022

Conversation

TillaTheHun0
Copy link
Member

@TillaTheHun0 TillaTheHun0 commented Feb 28, 2022

TL;DR: there shouldn't be any breaking changes in this PR, besides removing the unused error fields on some Cache port methods. Everything else should be backwards compatible. Not a lot of code changes (most of the diff is the duplicated utility), but need to explain my reasoning.


Previously, adapters would send back resolved promises for successful responses and rejected promises for error responses. With #468, that paradigm has shifted, such that an unhandled exceptions returns as a rejected promise and a handled exception should returned as a resolved promise.

Zod schemas wrap each adapter method, that validate the return type of resolved responses. Since errors were previously not resolved responses, these were not validated. Now they run through the zod schema validation flow, which is what we want, but introduces some necessary changes:

status and msg

msg was present on some methods on some ports, but not all of them. status was less present. Firstly, because hyper convention is msg and status only come back for error responses, and the zod schema is not used to validate rejected promises, these fields on the schema were superfluous. Now with the new error convention, they are not superfluous, but a new issue arises: **With object schemas, Zod will parse a value against a schema, then STRIP extraneous fields. This meant that status and msg had to be added explicitly to each return type schema on each port method.

all optional fields on return types

Because the resolve/reject promise boundary separated the success and error responses before, this wasn't an issue. Now both success and error responses can come back as a resolved Promise, and thereby are parsed by zod. If we were to try and encapsulate this into a single Zod object schema, say for queryDocuments, effectively all fields on the return schema, besides ok, would have to be optional():

z.object({
  ok: z.boolean(),
  msg: z.string().optional(),  <-- won't come back on valid success response so must be optional
  status: z.number().optional(), <-- won't come back on valid success response so must be optional
  docs: z.array(z.any()).optional(), <-- won't come back on valid error response so must be optional
}),

This isn't particularly useful, for validation. Ideally we would like some fields to be required if ok === true and other fields to be required if ok === false.

The solution: Zod Discriminated Union Response

Zod introduced a new feature in 3.12.0 called discriminated unions which is precisely what we need. However, due to colinhacks/zod#965 we cannot use that version just yet. For now, z.union will suffice.

We use zod.union to represent the possible return types of each adapter method. I've created a utility called hyperResSchema which enforces the union between a "success" response and a "hyper error" response. This allows each port to validate each type of response, without needing to make each field optional. I've duped this util across ports. perhaps eventually, we move it into hyper-utils

hyper responses everywhere

Eventually, we want each port method to return a "hyper response", that is { ok: boolean, .... }. There are some apis that do not follow this convention. But we would like ways to adapters to incrementally move to this convention without breaking validation. For this, I updated the following methods such that the schema supports the current return type, and a "hyper response" return type:

  • Storage Port ListObjects: can be current z.string().any() or hyper response with { objects: z.string().any() }
  • Queue Port Index: can be current z.string().any() or hyper response with { queues: z.string().any() }
  • Crawler Port get: can be current CrawlerDoc or hyper response with { doc: CrawlerDoc }
  • Cache Port getDoc: can be current any() or hyper response with { doc: any() }
  • Cache Port update, delete, create: removed optional error field from response (it should return a hyper error)
  • Cache Port index: can be current z.string().any() or hyper response with { caches: z.string().any() }

other things I did

@gitpod-io
Copy link

gitpod-io bot commented Feb 28, 2022

@twilson63 twilson63 self-requested a review February 28, 2022 22:43
@TillaTheHun0 TillaTheHun0 force-pushed the tillathehun0/ports-add-status branch from 3204770 to bff1980 Compare March 2, 2022 03:12
@TillaTheHun0 TillaTheHun0 force-pushed the tillathehun0/ports-add-status branch from bff1980 to 13635fa Compare March 2, 2022 03:15
@TillaTheHun0 TillaTheHun0 merged commit 22f8451 into main Mar 2, 2022
@TillaTheHun0 TillaTheHun0 deleted the tillathehun0/ports-add-status branch March 2, 2022 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants