Skip to content

Commit

Permalink
Remove duplicate creation of bundle from request details (#134)
Browse files Browse the repository at this point in the history 
* optimising_on_bundle_creation

* using preconditions to assert

* Added test cases
  • Loading branch information
@anchita-g
anchita-g committed Mar 28, 2023
1 parent 23091c3 commit 5ab10af
Show file tree
Hide file tree
Showing 6 changed files with 59 additions and 50 deletions.
3 changes: 2 additions & 1 deletion plugins/src/main/java/com/google/fhir/gateway/plugin/ListAccessChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -315,7 +315,8 @@ private AccessDecision processBundle(RequestDetailsReader requestDetails) throws
@Nullable
private BundlePatients createBundlePatients(RequestDetailsReader requestDetails)
throws IOException {
BundlePatients patientsInBundleUnfiltered = patientFinder.findPatientsInBundle(requestDetails);
Bundle requestBundle = FhirUtil.parseRequestToBundle(fhirContext, requestDetails);
BundlePatients patientsInBundleUnfiltered = patientFinder.findPatientsInBundle(requestBundle);

if (patientsInBundleUnfiltered == null) {
return null;
Expand Down
30 changes: 3 additions & 27 deletions plugins/src/main/java/com/google/fhir/gateway/plugin/PatientAccessChecker.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,6 @@
package com.google.fhir.gateway.plugin;

import ca.uhn.fhir.context.FhirContext;
import ca.uhn.fhir.parser.IParser;
import ca.uhn.fhir.rest.api.RequestTypeEnum;
import com.auth0.jwt.interfaces.DecodedJWT;
import com.google.common.annotations.VisibleForTesting;
Expand All @@ -37,12 +36,9 @@
import com.google.fhir.gateway.plugin.SmartFhirScope.Principal;
import java.net.URI;
import java.net.URISyntaxException;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.util.Arrays;
import java.util.Set;
import javax.inject.Named;
import org.hl7.fhir.instance.model.api.IBaseResource;
import org.hl7.fhir.instance.model.api.IIdType;
import org.hl7.fhir.r4.model.Bundle;
import org.hl7.fhir.r4.model.Bundle.BundleEntryComponent;
Expand Down Expand Up @@ -208,7 +204,8 @@ private AccessDecision checkPatientAccessInUpdate(RequestDetailsReader requestDe
}

private AccessDecision processBundle(RequestDetailsReader requestDetails) {
BundlePatients patientsInBundle = patientFinder.findPatientsInBundle(requestDetails);
Bundle requestBundle = FhirUtil.parseRequestToBundle(fhirContext, requestDetails);
BundlePatients patientsInBundle = patientFinder.findPatientsInBundle(requestBundle);

if (patientsInBundle == null
|| patientsInBundle.areTherePatientToCreate()
Expand All @@ -226,13 +223,7 @@ private AccessDecision processBundle(RequestDetailsReader requestDetails) {
return NoOpAccessDecision.accessDenied();
}
}
// TODO: there is duplication in processing the request Bundle here. It is already processed in
// PatientFinder
// Will follow up with a PR to solve this :https://github.com/google/fhir-gateway/issues/104
Bundle requestBundle = createBundleFromRequest(requestDetails);
if (requestBundle == null) {
return NoOpAccessDecision.accessDenied();
}

for (BundleEntryComponent entryComponent : requestBundle.getEntry()) {
if (!doesBundleElementHavePermission(entryComponent)) {
return NoOpAccessDecision.accessDenied();
Expand All @@ -241,21 +232,6 @@ private AccessDecision processBundle(RequestDetailsReader requestDetails) {
return NoOpAccessDecision.accessGranted();
}

private Bundle createBundleFromRequest(RequestDetailsReader request) {
byte[] requestContentBytes = request.loadRequestContents();
Charset charset = request.getCharset();
if (charset == null) {
charset = StandardCharsets.UTF_8;
}
String requestContent = new String(requestContentBytes, charset);
IParser jsonParser = fhirContext.newJsonParser();
IBaseResource resource = jsonParser.parseResource(requestContent);
if (!(resource instanceof Bundle)) {
return null;
}
return (Bundle) resource;
}

private boolean doesReferenceElementHavePermission(
IIdType referenceElement, Permission permission) {
if (referenceElement.getResourceType() != null && referenceElement.hasIdPart()) {
Expand Down
21 changes: 21 additions & 0 deletions server/src/main/java/com/google/fhir/gateway/FhirUtil.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,8 @@
import com.google.common.base.Preconditions;
import com.google.fhir.gateway.interfaces.RequestDetailsReader;
import java.io.IOException;
import java.nio.charset.Charset;
import java.nio.charset.StandardCharsets;
import java.util.regex.Pattern;
import javax.annotation.Nullable;
import org.apache.http.HttpResponse;
Expand Down Expand Up @@ -59,6 +61,25 @@ public static Bundle parseResponseToBundle(FhirContext fhirContext, HttpResponse
return (Bundle) resource;
}

public static IBaseResource createResourceFromRequest(
FhirContext fhirContext, RequestDetailsReader request) {
byte[] requestContentBytes = request.loadRequestContents();
Charset charset = request.getCharset();
if (charset == null) {
charset = StandardCharsets.UTF_8;
}
String requestContent = new String(requestContentBytes, charset);
IParser jsonParser = fhirContext.newJsonParser();
return jsonParser.parseResource(requestContent);
}

public static Bundle parseRequestToBundle(FhirContext fhirContext, RequestDetailsReader request) {
IBaseResource resource = createResourceFromRequest(fhirContext, request);
Preconditions.checkArgument(
FhirUtil.isSameResourceType(resource.fhirType(), ResourceType.Bundle));
return (Bundle) resource;
}

public static boolean isValidId(String id) {
return ID_PATTERN.matcher(id).matches();
}
Expand Down
22 changes: 2 additions & 20 deletions server/src/main/java/com/google/fhir/gateway/PatientFinderImp.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -211,17 +211,6 @@ public String findPatientFromParams(RequestDetailsReader requestDetails) {
return patientId;
}

private IBaseResource createResourceFromRequest(RequestDetailsReader request) {
byte[] requestContentBytes = request.loadRequestContents();
Charset charset = request.getCharset();
if (charset == null) {
charset = StandardCharsets.UTF_8;
}
String requestContent = new String(requestContentBytes, charset);
IParser jsonParser = fhirContext.newJsonParser();
return jsonParser.parseResource(requestContent);
}

private JsonArray createJsonArrayFromRequest(RequestDetailsReader request) {
byte[] requestContentBytes = request.loadRequestContents();
Charset charset = request.getCharset();
Expand Down Expand Up @@ -253,14 +242,7 @@ private Set<String> parseReferencesForPatientIds(IBaseResource resource) {
}

@Override
public BundlePatients findPatientsInBundle(RequestDetailsReader request) {
IBaseResource resource = createResourceFromRequest(request);
if (!(resource instanceof Bundle)) {
ExceptionUtil.throwRuntimeExceptionAndLog(
logger, "The provided resource is not a Bundle!", InvalidRequestException.class);
}
Bundle bundle = (Bundle) resource;

public BundlePatients findPatientsInBundle(Bundle bundle) {
if (bundle.getType() != BundleType.TRANSACTION) {
// Currently, support only for transaction bundles; see:
// https://github.com/google/fhir-access-proxy/issues/67
Expand Down Expand Up @@ -460,7 +442,7 @@ private void addPatientReference(Resource resource, BundlePatientsBuilder builde

@Override
public Set<String> findPatientsInResource(RequestDetailsReader request) {
IBaseResource resource = createResourceFromRequest(request);
IBaseResource resource = FhirUtil.createResourceFromRequest(fhirContext, request);
if (!resource.fhirType().equals(request.getResourceName())) {
ExceptionUtil.throwRuntimeExceptionAndLog(
logger,
Expand Down
5 changes: 3 additions & 2 deletions server/src/main/java/com/google/fhir/gateway/interfaces/PatientFinder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
import com.google.fhir.gateway.BundlePatients;
import java.util.Set;
import org.hl7.fhir.r4.model.Bundle;

public interface PatientFinder {
/**
Expand All @@ -34,12 +35,12 @@ public interface PatientFinder {
/**
* Find all patients referenced or updated in a Bundle.
*
* @param request that is expected to have a Bundle content.
* @param bundle bundle request to find patient references in.
* @return the {@link BundlePatients} that wraps all found patients.
* @throws InvalidRequestException for various reasons when unexpected content is encountered.
* Callers are expected to deny access when this happens.
*/
BundlePatients findPatientsInBundle(RequestDetailsReader request);
BundlePatients findPatientsInBundle(Bundle bundle);

/**
* Finds all patients in the content of a request.
Expand Down
28 changes: 28 additions & 0 deletions server/src/test/java/com/google/fhir/gateway/FhirUtilTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,14 +17,23 @@

import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.equalTo;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

import ca.uhn.fhir.context.FhirContext;
import com.google.common.io.Resources;
import com.google.fhir.gateway.interfaces.RequestDetailsReader;
import java.io.IOException;
import java.net.URL;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.junit.MockitoJUnitRunner;

@RunWith(MockitoJUnitRunner.class)
public class FhirUtilTest {

private static final FhirContext fhirContext = FhirContext.forR4();

@Test
public void isValidIdPass() {
assertThat(FhirUtil.isValidId("simple-id"), equalTo(true));
Expand Down Expand Up @@ -72,4 +81,23 @@ public void isValidIdTooLong() {
public void isValidIdNull() {
assertThat(FhirUtil.isValidId(""), equalTo(false));
}

@Test
public void canParseValidBundle() throws IOException {
URL bundleUrl = Resources.getResource("patient_id_search.json");
byte[] bundleBytes = Resources.toByteArray(bundleUrl);
RequestDetailsReader requestMock = mock(RequestDetailsReader.class);
when(requestMock.loadRequestContents()).thenReturn(bundleBytes);
assertThat(
FhirUtil.parseRequestToBundle(fhirContext, requestMock).getEntry().size(), equalTo(10));
}

@Test(expected = IllegalArgumentException.class)
public void throwExceptionOnNonBundleResource() throws IOException {
URL bundleUrl = Resources.getResource("test_patient.json");
byte[] bundleBytes = Resources.toByteArray(bundleUrl);
RequestDetailsReader requestMock = mock(RequestDetailsReader.class);
when(requestMock.loadRequestContents()).thenReturn(bundleBytes);
FhirUtil.parseRequestToBundle(fhirContext, requestMock).getEntry().size();
}
}

0 comments on commit 5ab10af

Please sign in to comment.