From f2e382a1324481aace2cb078a9eb2a74b68b4eba Mon Sep 17 00:00:00 2001 From: James Agnew Date: Wed, 24 Apr 2019 14:10:21 -0400 Subject: [PATCH] Swap JSON Patch provider --- hapi-fhir-jpaserver-base/pom.xml | 14 +- .../jpa/util/jsonpatch/CopyOperation.java | 81 -------- .../jpa/util/jsonpatch/JsonPatchUtils.java | 90 +++------ .../dstu3/ResourceProviderDstu3Test.java | 125 ------------ .../jpa/provider/r4/PatchProviderR4Test.java | 190 ++++++++++++++++++ .../util/jsonpatch/JsonPatchUtilsTest.java | 81 ++++++++ pom.xml | 10 +- src/changes/changes.xml | 7 + 8 files changed, 321 insertions(+), 277 deletions(-) delete mode 100644 hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/jsonpatch/CopyOperation.java create mode 100644 hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/PatchProviderR4Test.java create mode 100644 hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/util/jsonpatch/JsonPatchUtilsTest.java diff --git a/hapi-fhir-jpaserver-base/pom.xml b/hapi-fhir-jpaserver-base/pom.xml index a31a9b236e5..947ea4de6e8 100644 --- a/hapi-fhir-jpaserver-base/pom.xml +++ b/hapi-fhir-jpaserver-base/pom.xml @@ -147,6 +147,10 @@ com.fasterxml.jackson.core jackson-annotations + + com.fasterxml.jackson.core + jackson-databind + com.helger @@ -182,14 +186,8 @@ - net.riotopsys - json_patch - - - com.google.code.gson - gson - - + com.github.java-json-tools + json-patch com.github.dnault diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/jsonpatch/CopyOperation.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/jsonpatch/CopyOperation.java deleted file mode 100644 index 1a2aaeb8b17..00000000000 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/jsonpatch/CopyOperation.java +++ /dev/null @@ -1,81 +0,0 @@ -package ca.uhn.fhir.jpa.util.jsonpatch; - -/* - * #%L - * HAPI FHIR JPA Server - * %% - * Copyright (C) 2014 - 2019 University Health Network - * %% - * Licensed under the Apache License, Version 2.0 (the "License"); - * you may not use this file except in compliance with the License. - * You may obtain a copy of the License at - * - * http://www.apache.org/licenses/LICENSE-2.0 - * - * Unless required by applicable law or agreed to in writing, software - * distributed under the License is distributed on an "AS IS" BASIS, - * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - * See the License for the specific language governing permissions and - * limitations under the License. - * #L% - */ - -import com.google.gson.JsonArray; -import com.google.gson.JsonElement; -import net.riotopsys.json_patch.JsonPath; -import net.riotopsys.json_patch.operation.AbsOperation; - -import java.util.ArrayList; -import java.util.Iterator; -import java.util.List; - -public class CopyOperation extends AbsOperation { - - public JsonPath mySourcePath; - - public CopyOperation(JsonPath theTargetPath, JsonPath theSourcePath) { - mySourcePath = theSourcePath; - this.path = theTargetPath; - } - - @Override - public String getOperationName() { - return "copy"; - } - - @Override - public JsonElement apply(JsonElement original) { - JsonElement result = duplicate(original); - - JsonElement item = path.head().navigate(result); - JsonElement data = mySourcePath.head().navigate(original); - - if (item.isJsonObject()) { - item.getAsJsonObject().add(path.tail(), data); - } else if (item.isJsonArray()) { - - JsonArray array = item.getAsJsonArray(); - - int index = (path.tail().equals("-")) ? array.size() : Integer.valueOf(path.tail()); - - List temp = new ArrayList(); - - Iterator iter = array.iterator(); - while (iter.hasNext()) { - JsonElement stuff = iter.next(); - iter.remove(); - temp.add(stuff); - } - - temp.add(index, data); - - for (JsonElement stuff : temp) { - array.add(stuff); - } - - } - - return result; - } - -} diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/jsonpatch/JsonPatchUtils.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/jsonpatch/JsonPatchUtils.java index 9fb4a61d25e..8a1c0a2e64d 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/jsonpatch/JsonPatchUtils.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/util/jsonpatch/JsonPatchUtils.java @@ -20,73 +20,47 @@ package ca.uhn.fhir.jpa.util.jsonpatch; * #L% */ +import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; +import com.fasterxml.jackson.core.JsonFactory; +import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.github.fge.jsonpatch.JsonPatch; +import com.github.fge.jsonpatch.JsonPatchException; import org.hl7.fhir.instance.model.api.IBaseResource; -import com.google.gson.Gson; -import com.google.gson.JsonArray; -import com.google.gson.JsonElement; -import com.google.gson.JsonObject; - -import ca.uhn.fhir.context.FhirContext; -import ca.uhn.fhir.parser.JsonParser; -import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; -import net.riotopsys.json_patch.JsonPatch; -import net.riotopsys.json_patch.JsonPath; -import net.riotopsys.json_patch.operation.AddOperation; -import net.riotopsys.json_patch.operation.MoveOperation; -import net.riotopsys.json_patch.operation.RemoveOperation; -import net.riotopsys.json_patch.operation.ReplaceOperation; +import java.io.IOException; +import java.io.StringReader; public class JsonPatchUtils { public static T apply(FhirContext theCtx, T theResourceToUpdate, String thePatchBody) { - JsonPatch parsedPatch = new JsonPatch(); - // Parse the patch - Gson gson = JsonParser.newGson(); - JsonElement jsonElement = gson.fromJson(thePatchBody, JsonElement.class); - JsonArray array = jsonElement.getAsJsonArray(); - for (JsonElement nextElement : array) { - JsonObject nextElementAsObject = (JsonObject) nextElement; - - String opName = nextElementAsObject.get("op").getAsString(); - if ("add".equals(opName)) { - AddOperation op = new AddOperation(toPath(nextElementAsObject), nextElementAsObject.get("value")); - parsedPatch.add(op); - } else if ("remove".equals(opName)) { - RemoveOperation op = new RemoveOperation(toPath(nextElementAsObject)); - parsedPatch.add(op); - } else if ("replace".equals(opName)) { - ReplaceOperation op = new ReplaceOperation(toPath(nextElementAsObject), nextElementAsObject.get("value")); - parsedPatch.add(op); - } else if ("copy".equals(opName)) { - CopyOperation op = new CopyOperation(toPath(nextElementAsObject), toFromPath(nextElementAsObject)); - parsedPatch.add(op); - } else if ("move".equals(opName)) { - MoveOperation op = new MoveOperation(toPath(nextElementAsObject), toFromPath(nextElementAsObject)); - parsedPatch.add(op); - } else { - throw new InvalidRequestException("Invalid JSON PATCH operation: " + opName); - } - + ObjectMapper mapper = new ObjectMapper(); + mapper.configure(JsonParser.Feature.INCLUDE_SOURCE_IN_LOCATION, false); + + JsonFactory factory = mapper.getFactory(); + + final JsonPatch patch; + try { + com.fasterxml.jackson.core.JsonParser parser = factory.createParser(thePatchBody); + JsonNode jsonPatchNode = mapper.readTree(parser); + patch = JsonPatch.fromJson(jsonPatchNode); + + JsonNode originalJsonDocument = mapper.readTree(theCtx.newJsonParser().encodeResourceToString(theResourceToUpdate)); + JsonNode after = patch.apply(originalJsonDocument); + + @SuppressWarnings("unchecked") + Class clazz = (Class) theResourceToUpdate.getClass(); + + T retVal = theCtx.newJsonParser().parseResource(clazz, mapper.writeValueAsString(after)); + return retVal; + + } catch (IOException | JsonPatchException theE) { + throw new InvalidRequestException(theE.getMessage()); } - - @SuppressWarnings("unchecked") - Class clazz = (Class) theResourceToUpdate.getClass(); - - JsonElement originalJsonDocument = gson.fromJson(theCtx.newJsonParser().encodeResourceToString(theResourceToUpdate), JsonElement.class); - JsonElement target = parsedPatch.apply(originalJsonDocument); - T retVal = theCtx.newJsonParser().parseResource(clazz, gson.toJson(target)); - - return retVal; - } - private static JsonPath toFromPath(JsonObject nextElementAsObject) { - return new JsonPath(nextElementAsObject.get("from").getAsString()); - } - - private static JsonPath toPath(JsonObject nextElementAsObject) { - return new JsonPath(nextElementAsObject.get("path").getAsString()); } } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderDstu3Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderDstu3Test.java index 3254d13cd1e..b62e8e0e7a5 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderDstu3Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderDstu3Test.java @@ -2198,131 +2198,6 @@ public class ResourceProviderDstu3Test extends BaseResourceProviderDstu3Test { } - @Test - public void testPatchUsingJsonPatch() throws Exception { - String methodName = "testPatchUsingJsonPatch"; - IIdType pid1; - { - Patient patient = new Patient(); - patient.setActive(true); - patient.addIdentifier().setSystem("urn:system").setValue("0"); - patient.addName().setFamily(methodName).addGiven("Joe"); - pid1 = myPatientDao.create(patient, mySrd).getId().toUnqualifiedVersionless(); - } - - HttpPatch patch = new HttpPatch(ourServerBase + "/Patient/" + pid1.getIdPart()); - patch.setEntity(new StringEntity("[ { \"op\":\"replace\", \"path\":\"/active\", \"value\":false } ]", ContentType.parse(Constants.CT_JSON_PATCH + Constants.CHARSET_UTF8_CTSUFFIX))); - patch.addHeader(Constants.HEADER_PREFER, Constants.HEADER_PREFER_RETURN + "=" + Constants.HEADER_PREFER_RETURN_OPERATION_OUTCOME); - - CloseableHttpResponse response = ourHttpClient.execute(patch); - try { - assertEquals(200, response.getStatusLine().getStatusCode()); - String responseString = IOUtils.toString(response.getEntity().getContent(), StandardCharsets.UTF_8); - assertThat(responseString, containsString("")); - } finally { - response.close(); - } - - Patient newPt = ourClient.read().resource(Patient.class).withId(pid1.getIdPart()).execute(); - assertEquals("1", newPt.getIdElement().getVersionIdPart()); - assertEquals(true, newPt.getActive()); - } - - @Test - public void testPatchUsingJsonPatchWithContentionCheckGood() throws Exception { - String methodName = "testPatchUsingJsonPatchWithContentionCheckGood"; - IIdType pid1; - { - Patient patient = new Patient(); - patient.setActive(true); - patient.addIdentifier().setSystem("urn:system").setValue("0"); - patient.addName().setFamily(methodName).addGiven("Joe"); - pid1 = myPatientDao.create(patient, mySrd).getId().toUnqualifiedVersionless(); - } - - HttpPatch patch = new HttpPatch(ourServerBase + "/Patient/" + pid1.getIdPart()); - patch.setEntity(new StringEntity("[ { \"op\":\"replace\", \"path\":\"/active\", \"value\":false } ]", ContentType.parse(Constants.CT_JSON_PATCH + Constants.CHARSET_UTF8_CTSUFFIX))); - patch.addHeader("If-Match", "W/\"1\""); - patch.addHeader(Constants.HEADER_PREFER, Constants.HEADER_PREFER_RETURN + "=" + Constants.HEADER_PREFER_RETURN_OPERATION_OUTCOME); - - CloseableHttpResponse response = ourHttpClient.execute(patch); - try { - assertEquals(200, response.getStatusLine().getStatusCode()); - String responseString = IOUtils.toString(response.getEntity().getContent(), StandardCharsets.UTF_8); - assertThat(responseString, containsString("false"; - patch.addHeader(Constants.HEADER_PREFER, Constants.HEADER_PREFER_RETURN + "=" + Constants.HEADER_PREFER_RETURN_OPERATION_OUTCOME); - patch.setEntity(new StringEntity(patchString, ContentType.parse(Constants.CT_XML_PATCH + Constants.CHARSET_UTF8_CTSUFFIX))); - - CloseableHttpResponse response = ourHttpClient.execute(patch); - try { - assertEquals(200, response.getStatusLine().getStatusCode()); - String responseString = IOUtils.toString(response.getEntity().getContent(), StandardCharsets.UTF_8); - assertThat(responseString, containsString("")); + } finally { + response.close(); + } + + Patient newPt = ourClient.read().resource(Patient.class).withId(pid1.getIdPart()).execute(); + assertEquals("1", newPt.getIdElement().getVersionIdPart()); + assertEquals(true, newPt.getActive()); + } + + @Test + public void testPatchUsingJsonPatchWithContentionCheckGood() throws Exception { + String methodName = "testPatchUsingJsonPatchWithContentionCheckGood"; + IIdType pid1; + { + Patient patient = new Patient(); + patient.setActive(true); + patient.addIdentifier().setSystem("urn:system").setValue("0"); + patient.addName().setFamily(methodName).addGiven("Joe"); + pid1 = myPatientDao.create(patient, mySrd).getId().toUnqualifiedVersionless(); + } + + HttpPatch patch = new HttpPatch(ourServerBase + "/Patient/" + pid1.getIdPart()); + patch.setEntity(new StringEntity("[ { \"op\":\"replace\", \"path\":\"/active\", \"value\":false } ]", ContentType.parse(Constants.CT_JSON_PATCH + Constants.CHARSET_UTF8_CTSUFFIX))); + patch.addHeader("If-Match", "W/\"1\""); + patch.addHeader(Constants.HEADER_PREFER, Constants.HEADER_PREFER_RETURN + "=" + Constants.HEADER_PREFER_RETURN_OPERATION_OUTCOME); + + CloseableHttpResponse response = ourHttpClient.execute(patch); + try { + assertEquals(200, response.getStatusLine().getStatusCode()); + String responseString = IOUtils.toString(response.getEntity().getContent(), StandardCharsets.UTF_8); + assertThat(responseString, containsString("false"; + patch.addHeader(Constants.HEADER_PREFER, Constants.HEADER_PREFER_RETURN + "=" + Constants.HEADER_PREFER_RETURN_OPERATION_OUTCOME); + patch.setEntity(new StringEntity(patchString, ContentType.parse(Constants.CT_XML_PATCH + Constants.CHARSET_UTF8_CTSUFFIX))); + + CloseableHttpResponse response = ourHttpClient.execute(patch); + try { + assertEquals(200, response.getStatusLine().getStatusCode()); + String responseString = IOUtils.toString(response.getEntity().getContent(), StandardCharsets.UTF_8); + assertThat(responseString, containsString("xml-patch 0.3.0 + + com.github.java-json-tools + json-patch + 1.10 + com.google.errorprone error_prone_core @@ -840,11 +845,6 @@ mysql-connector-java 8.0.12 - - net.riotopsys - json_patch - 0.0.0 - net.sf.json-lib json-lib diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 62b1cfdb530..5b9e7f6898c 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -164,6 +164,13 @@ channel is destroyed (because its subscription is deleted) then the remove() method will be called on that channel. + + The JSON Patch provider has been switched to use the provider from the + Java JSON Tools + ]]> + project, as it is much more robust and fault tolerant. +