Throw an error when a patch attempts to modify resourceType or Id #4360
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
3 Ignored Deployments
|
|
@gusIreland is attempting to deploy a commit to the Medplum Team on Vercel. A member of the Team first needs to authorize it. |
| }, | ||
| ]); | ||
|
|
||
| expect(result.id).toEqual(patient.id); |
There was a problem hiding this comment.
I added this spec to wrap my head around the patchResource function - I copied the spec from this spec. I'm a little unclear as I thought the patch would modify the resource in place, instead of having to use and check the result object
reshmakh
added
the
fhir-datastore
| if (patch.find((operation) => operation.path === '/resourceType')) { | ||
| throw new OperationOutcomeError(badRequest('Incorrect Resource Type')); | ||
| } | ||
| if (patch.find((operation) => operation.path === '/id')) { | ||
| throw new OperationOutcomeError(badRequest('Incorrect Id')); | ||
| } |
There was a problem hiding this comment.
I'd prefer for these checks to live in updateResourceImpl(), which several different Repository methods call into, so that this issue can be prevented in depth for the future. @gusIreland If you don't mind making that change I'd appreciate it, but I'm also happy to handle it if you're short on time — let me know either way!
There was a problem hiding this comment.
That makes sense! I'll update it and ping you when it's ready
| if (patch.find((operation) => operation.path === '/id')) { | ||
| throw new OperationOutcomeError(badRequest('Incorrect Id')); | ||
| } | ||
|
|
||
| try { | ||
| const patchResult = applyPatch(resource, patch).filter(Boolean); |
There was a problem hiding this comment.
@mattwiller I moved the checks to updateResourceImpl(), but my new spec fails with a validation error(Invalid additional property) from the applyPatch function, which gets called before updateResourceImpl().
Would it make sense to have checks at both levels? One to check that the patch call is valid, and another at the updateResourceImpl to prevent the issue in depth
| if (resource.resourceType !== resourceType) { | ||
| throw new OperationOutcomeError(badRequest('Incorrect resource type')); | ||
| } | ||
| if (resource.id !== id) { | ||
| throw new OperationOutcomeError(badRequest('Incorrect ID')); | ||
| } |
There was a problem hiding this comment.
These checks can never fail:
const { resourceType, id } = resource; // <-- resourceType and id come from resource
// ...
if (resource.resourceType !== resourceType) { // <-- comparing back to resource must succeed
throw new OperationOutcomeError(badRequest('Incorrect resource type'));
}
if (resource.id !== id) { // <-- same here
throw new OperationOutcomeError(badRequest('Incorrect ID'));
}In order to make this work correctly, the updated resourceType and id should be compared against the existing ones in a stored resource to ensure they will not be changed in the update. This might require some deeper refactoring; I can take a look in the next day or two to evaluate
There was a problem hiding this comment.
Ah ok, I was trying to add some specs for that change and running into that problem - I'll start poking at it on my end
This PR addresses #3888
When a resource is PATCHed and the field attempted to be updated is either
idorresourceType, an error is thrown