Implement full batch and transaction support #4358
Conversation
This reverts commit 3ab1afe.
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
2 Ignored Deployments
|
mattwiller
added
the
fhir-datastore
mattwiller
force-pushed
the
batch-transaction
branch
from
355bb2f
to
655ca88
Compare
| @@ -143,31 +183,43 @@ async function patchResource(req: FhirRequest, repo: FhirRepository): Promise<Fh | |||
| return [allOk, resource]; | |||
| } | |||
|
|
|||
| export type RestInteraction = | |||
There was a problem hiding this comment.
Low pri mental note: we may want to try to merge this with packages/server/src/util/auditevent.ts, either here in fhir-router or in core.
| @@ -143,31 +183,43 @@ async function patchResource(req: FhirRequest, repo: FhirRepository): Promise<Fh | |||
| return [allOk, resource]; | |||
| } | |||
|
|
|||
| export type RestInteraction = | |||
| | CapabilityStatementRestInteraction['code'] | |||
There was a problem hiding this comment.
Low pri mental note: This is another case where having a proper type for a ValueSet-derived string union would have been helpful, so you could use CapabilityStatementRestInteractionCode rather than CapabilityStatementRestInteraction['code'] -- I'm pretty sure the former would be faster for tsc, and probably more discoverable.
| this.router.add('DELETE', ':resourceType/:id', deleteResource); | ||
| this.router.add('PATCH', ':resourceType/:id', patchResource); | ||
| this.router.add('POST', '$graphql', graphqlHandler); | ||
| this.router.add('GET', '', searchMultipleTypes, { interaction: 'search-system' }); |
There was a problem hiding this comment.
This is definitely starting to paint a clearer picture for the future.
Something like:
- New outer type such as
FhirProcessorwhich wrapsFhirRouterandFhirRepository registerHandler(handler: OperationHandler)which adds routes and whatnot
I'm not sure if FHIR has a good term that encompasses both interaction and operation ?
|
This is exciting to see taking shape! I have a few questions/concerns to better understand and expectation set.
|
|
@dillonstreator Great questions! At this point, strict uniqueness guarantees for conditional create aren't part of the implementation: it's a hard problem to solve perfectly, as you've pointed out with the concerns around table locking. The FHIR spec is somewhat vague about exactly what the guarantees for this functionality should be, but it's something I'm planning to explore as a fast follow after the initial implementation is complete. In any case, I don't think that we're likely to go with a heavyweight solution like whole table locking — for exactly the scale/performance concerns you mentioned. I'll need to spend some time thinking through other options and the correctness/performance tradeoffs around them |
|
Thank you for the transparency, @mattwiller. I appreciate the thoughtfulness being put into this and the commitment to addressing the uniqueness needs. |
There was a problem hiding this comment.
Overall this looks great. Thanks for slogging through 🙏
Added initial feedback. I want to review more in-depth when I get some time.
One immediate thing we need to add is disabling "read from Redis" when in the middle of a transaction. Otherwise things could get weird (I think?)
packages/fhir-router/src/batch.ts
Outdated
| try { | ||
| resolved = await this.resolveIdentity(entry, `Bundle.entry[${index}]`); | ||
| } catch (err: any) { | ||
| if ((err as OperationOutcomeError).outcome && this.bundle.type !== 'transaction') { |
There was a problem hiding this comment.
Hm, this as OperationOutcomeError seems risky, although I'm not sure why. I've been trying to always use normalizeOperationOutcome or whatever to avoid casts in catch.
| if ((err as OperationOutcomeError).outcome && this.bundle.type !== 'transaction') { | |
| if (err instanceof OperationOutcomeError && this.bundle.type !== 'transaction') { |
packages/fhir-router/src/batch.ts
Outdated
| return this.processModification(entry, path); | ||
| case 'GET': | ||
| case 'HEAD': | ||
| // Ignore read-only requests |
There was a problem hiding this comment.
Huh, I'm surprised this is is valid, and that these are even real values in the Bundle.entry.request.method value set.
packages/fhir-router/src/batch.ts
Outdated
| const route = this.router.find(entry.request?.method as HttpMethod, requestPath); | ||
| const params = route?.params; | ||
|
|
||
| switch (route?.data?.interaction) { |
There was a problem hiding this comment.
Hm, I need to review this more, but I would have assumed that most of this switch statement would have been handled by handleRequest - if nothing else, a lot of these checks are duplicated, which would be nice to clean up if possible.
const result = await this.router.handleRequest(
{
method: request.method as HttpMethod,
pathname: url.pathname,
params: Object.create(null),
query: Object.fromEntries(url.searchParams),
body,
},
this.repo
);
|
No description provided.