-
Notifications
You must be signed in to change notification settings - Fork 346
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
Implement full batch and transaction support #4358
base: main
Are you sure you want to change the base?
Conversation
This reverts commit 3ab1afe.
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
355bb2f
to
655ca88
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Structurally, this looks great to me.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is definitely starting to paint a clearer picture for the future.
Something like:
- New outer type such as
FhirProcessor
which wrapsFhirRouter
andFhirRepository
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. |
41f9ad6
to
ea92fe0
Compare
9fd8b87
to
d77784b
Compare
Quality Gate passedIssues Measures |
Resolves #4234