fix: GenericResponse response must exist if sub properties tested. #54
Conversation
…se exists before testing for sub properties.
| const response = await this.handler.read(resourceType, id); | ||
| const updatedReadResponse = await this.authService.authorizeAndFilterReadResponse({ | ||
| operation: 'read', | ||
| userIdentity: res.locals.userIdentity, | ||
| readResponse: response, | ||
| }); | ||
| if (updatedReadResponse && updatedReadResponse.meta) { | ||
| res.set({ | ||
| ETag: `W/"${updatedReadResponse.meta.versionId}"`, | ||
| 'Last-Modified': updatedReadResponse.meta.lastUpdated, | ||
| }); | ||
| } | ||
| res.send(updatedReadResponse); |
There was a problem hiding this comment.
I agree that since we are using any for some return values, type safety cannot be guaranteed and this kind of additional checks are warranted. But, have this caused runtime errors on your side? I don't think that this.authService.authorizeAndFilterReadResponse can actually return null or undefined, even if the typing allows it (but I haven't tested extensively)
The change on L45 only matters if updatedReadResponse is null or undefined and it prevents the code from crashing on L45. But then an undefined updatedReadResponse would be sent as response on L51 and that would break things elsewhere. This change looks good, some future work for us would be to better handle undefined responses or to narrow down our typescript types if undefined responses don't occur to begin with.
There was a problem hiding this comment.
We've only seen runtime errors for the 'create' operation case when not returning a response body. I just filled in the cases for other operations returning a GenericResponse type without too much thought!
I agree that this is an obscure corner case - by design & the FHIR spec the persistence component read method should either return a resource or should throw a ResourceNotFoundError. As you say future work might be to update the read interface to always require a resource.
In our use case we have different people implementing persistence components, with varying degrees of FHIR & developer knowledge, so even though it is wrong, there is a chance that one of them would actually fail to set a resource within the GenericResponse. If we are using the stubbed passThroughAuthz component for authorizeAndFilterReadResponse, that would return the undefined readResponse into this code and trigger the bug. I tested the code on L51 if updatedReadResponse is undefined and it is handled ok. I expect this is dealt with somewhere in Express.
Issue #, if available:
#53
Description of changes:
Add check that optional
responseexists before checking sub properties of it.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.