From c27e910da84f1dc829cca480e8ca11408c7ba688 Mon Sep 17 00:00:00 2001 From: Steven Winship <39765413+stevenwinship@users.noreply.github.com> Date: Thu, 5 Dec 2024 15:38:42 -0500 Subject: [PATCH 1/4] remove duplicate DatasetFieldTypes --- .../iq/dataverse/util/json/JsonPrinter.java | 25 +++++++--- .../dataverse/util/json/JsonPrinterTest.java | 49 +++++++++++++++++++ 2 files changed, 68 insertions(+), 6 deletions(-) diff --git a/src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java b/src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java index 91af13c79a3..bb0c8e10c82 100644 --- a/src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java +++ b/src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java @@ -642,17 +642,19 @@ public static JsonObjectBuilder json(MetadataBlock metadataBlock, boolean printO .add("displayName", metadataBlock.getDisplayName()) .add("displayOnCreate", metadataBlock.isDisplayOnCreate()); - Set datasetFieldTypes; + List datasetFieldTypesList; if (ownerDataverse != null) { - datasetFieldTypes = new TreeSet<>(datasetFieldService.findAllInMetadataBlockAndDataverse( - metadataBlock, ownerDataverse, printOnlyDisplayedOnCreateDatasetFieldTypes)); + datasetFieldTypesList = datasetFieldService.findAllInMetadataBlockAndDataverse( + metadataBlock, ownerDataverse, printOnlyDisplayedOnCreateDatasetFieldTypes); } else { - datasetFieldTypes = printOnlyDisplayedOnCreateDatasetFieldTypes - ? new TreeSet<>(datasetFieldService.findAllDisplayedOnCreateInMetadataBlock(metadataBlock)) - : new TreeSet<>(metadataBlock.getDatasetFieldTypes()); + datasetFieldTypesList = printOnlyDisplayedOnCreateDatasetFieldTypes + ? datasetFieldService.findAllDisplayedOnCreateInMetadataBlock(metadataBlock) + : metadataBlock.getDatasetFieldTypes(); } + Set datasetFieldTypes = filterOutDuplicateDatasetFieldTypes(datasetFieldTypesList); + JsonObjectBuilder fieldsBuilder = Json.createObjectBuilder(); for (DatasetFieldType datasetFieldType : datasetFieldTypes) { fieldsBuilder.add(datasetFieldType.getName(), json(datasetFieldType, ownerDataverse)); @@ -662,6 +664,17 @@ public static JsonObjectBuilder json(MetadataBlock metadataBlock, boolean printO return jsonObjectBuilder; } + // This will remove datasetFieldTypes that are in the list but also a child of another datasetFieldType in the list + // Prevents duplicate datasetFieldType information from being returned twice + // See: https://github.com/IQSS/dataverse/issues/10472 + private static Set filterOutDuplicateDatasetFieldTypes(List datasetFieldTypesList) { + // making a copy of the list as to not damage the original when we remove items + List datasetFieldTypes = new ArrayList<>(datasetFieldTypesList); + // exclude/remove datasetFieldTypes if datasetFieldType exists as a child of another datasetFieldType + datasetFieldTypesList.forEach(dsft -> dsft.getChildDatasetFieldTypes().forEach(c -> datasetFieldTypes.remove(c))); + return new TreeSet<>(datasetFieldTypes); + } + public static JsonArrayBuilder jsonDatasetFieldTypes(List fields) { JsonArrayBuilder fieldsJson = Json.createArrayBuilder(); for (DatasetFieldType field : fields) { diff --git a/src/test/java/edu/harvard/iq/dataverse/util/json/JsonPrinterTest.java b/src/test/java/edu/harvard/iq/dataverse/util/json/JsonPrinterTest.java index 7ec8e0b25f3..1987307637c 100644 --- a/src/test/java/edu/harvard/iq/dataverse/util/json/JsonPrinterTest.java +++ b/src/test/java/edu/harvard/iq/dataverse/util/json/JsonPrinterTest.java @@ -25,6 +25,7 @@ import jakarta.json.JsonString; import edu.harvard.iq.dataverse.util.BundleUtil; +import org.assertj.core.util.Lists; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.BeforeEach; @@ -268,6 +269,54 @@ public void testDatasetContactWithPrivacy() { } + @Test + public void testDatasetFieldTypesWithChildren() { + MetadataBlock block = new MetadataBlock(); + block.setId(0L); + block.setName("citation"); + long id = 0L; + // create datasetFieldTypes + List datasetFieldTypes = new ArrayList<>(); + for (int i = 0; i < 10; i++) { + DatasetFieldType dft = new DatasetFieldType(); + dft.setId(id++); + dft.setDisplayOrder(i); + dft.setMetadataBlock(block); + dft.setFieldType(FieldType.TEXT); + dft.setName("subType" + dft.getId()); + dft.setTitle(dft.getName()); + dft.setChildDatasetFieldTypes(Lists.emptyList()); + datasetFieldTypes.add(dft); + } + // add DatasetFieldType as children to another DatasetFieldType to test the suppression of duplicate data + // adding 3 and 4 as children of 2 + datasetFieldTypes.get(3).setParentDatasetFieldType(datasetFieldTypes.get(2)); + datasetFieldTypes.get(4).setParentDatasetFieldType(datasetFieldTypes.get(2)); + datasetFieldTypes.get(2).setChildDatasetFieldTypes(List.of(datasetFieldTypes.get(3), datasetFieldTypes.get(4))); + // adding 6 as child of 9 + datasetFieldTypes.get(6).setParentDatasetFieldType(datasetFieldTypes.get(9)); + datasetFieldTypes.get(9).setChildDatasetFieldTypes(List.of(datasetFieldTypes.get(6))); + + block.setDatasetFieldTypes(datasetFieldTypes); + + DatasetFieldServiceBean nullDFServiceBean = null; + JsonPrinter.injectSettingsService(new MockSettingsSvc(), nullDFServiceBean); + + JsonObject jsonObject = JsonPrinter.json(block).build(); + assertNotNull(jsonObject); + + System.out.println("json: " + JsonUtil.prettyPrint(jsonObject.toString())); + assertEquals("subType2 subType3", jsonObject.getJsonObject("fields").getJsonObject("subType2") + .getJsonObject("childFields").getJsonObject("subType3").getString("displayName")); + assertEquals("subType2 subType4", jsonObject.getJsonObject("fields").getJsonObject("subType2") + .getJsonObject("childFields").getJsonObject("subType4").getString("displayName")); + assertEquals("subType9 subType6", jsonObject.getJsonObject("fields").getJsonObject("subType9") + .getJsonObject("childFields").getJsonObject("subType6").getString("displayName")); + assertNull(jsonObject.getJsonObject("fields").getJsonObject("subType3")); + assertNull(jsonObject.getJsonObject("fields").getJsonObject("subType4")); + assertNull(jsonObject.getJsonObject("fields").getJsonObject("subType6")); + } + @Test public void testDataversePrinter() { Dataverse dataverse = new Dataverse(); From b2e449779cc141160d15070b18eb2a3a037c848b Mon Sep 17 00:00:00 2001 From: Steven Winship <39765413+stevenwinship@users.noreply.github.com> Date: Fri, 6 Dec 2024 11:20:50 -0500 Subject: [PATCH 2/4] fix tests afetr validating new results --- .../java/edu/harvard/iq/dataverse/api/DataversesIT.java | 6 +++--- .../java/edu/harvard/iq/dataverse/api/MetadataBlocksIT.java | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java b/src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java index 76bb515beb2..d58c84a06d4 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java @@ -927,7 +927,7 @@ public void testListMetadataBlocks() { .body("data.size()", equalTo(1)) .body("data[0].name", is("citation")) .body("data[0].fields.title.displayOnCreate", equalTo(true)) - .body("data[0].fields.size()", is(28)); + .body("data[0].fields.size()", is(10)); // 28 - 18 child duplicates Response setMetadataBlocksResponse = UtilIT.setMetadataBlocks(dataverseAlias, Json.createArrayBuilder().add("citation").add("astrophysics"), apiToken); setMetadataBlocksResponse.then().assertThat().statusCode(OK.getStatusCode()); @@ -1007,11 +1007,11 @@ public void testListMetadataBlocks() { // Since the included property of notesText is set to false, we should retrieve the total number of fields minus one int citationMetadataBlockIndex = geospatialMetadataBlockIndex == 0 ? 1 : 0; listMetadataBlocksResponse.then().assertThat() - .body(String.format("data[%d].fields.size()", citationMetadataBlockIndex), equalTo(79)); + .body(String.format("data[%d].fields.size()", citationMetadataBlockIndex), equalTo(34)); // 79 minus 45 child duplicates // Since the included property of geographicCoverage is set to false, we should retrieve the total number of fields minus one listMetadataBlocksResponse.then().assertThat() - .body(String.format("data[%d].fields.size()", geospatialMetadataBlockIndex), equalTo(10)); + .body(String.format("data[%d].fields.size()", geospatialMetadataBlockIndex), equalTo(6)); // 10 - 4 child duplicates String actualGeospatialMetadataField1 = listMetadataBlocksResponse.then().extract().path(String.format("data[%d].fields.geographicCoverage.name", geospatialMetadataBlockIndex)); String actualGeospatialMetadataField2 = listMetadataBlocksResponse.then().extract().path(String.format("data[%d].fields.country.name", geospatialMetadataBlockIndex)); diff --git a/src/test/java/edu/harvard/iq/dataverse/api/MetadataBlocksIT.java b/src/test/java/edu/harvard/iq/dataverse/api/MetadataBlocksIT.java index 6e7061961f0..fe608a97ac8 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/MetadataBlocksIT.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/MetadataBlocksIT.java @@ -42,7 +42,7 @@ void testListMetadataBlocks() { // returnDatasetFieldTypes=true listMetadataBlocksResponse = UtilIT.listMetadataBlocks(false, true); - int expectedNumberOfMetadataFields = 80; + int expectedNumberOfMetadataFields = 35; // 80 - 45 child duplicates; listMetadataBlocksResponse.then().assertThat() .statusCode(OK.getStatusCode()) .body("data[0].fields", not(equalTo(null))) @@ -51,7 +51,8 @@ void testListMetadataBlocks() { // onlyDisplayedOnCreate=true and returnDatasetFieldTypes=true listMetadataBlocksResponse = UtilIT.listMetadataBlocks(true, true); - expectedNumberOfMetadataFields = 28; + listMetadataBlocksResponse.prettyPrint(); + expectedNumberOfMetadataFields = 10; // 28 - 18 child duplicates listMetadataBlocksResponse.then().assertThat() .statusCode(OK.getStatusCode()) .body("data[0].fields", not(equalTo(null))) From 9fb3dbe956fe69f0a7be17dd67cbd1dce7b42bcc Mon Sep 17 00:00:00 2001 From: Steven Winship <39765413+stevenwinship@users.noreply.github.com> Date: Fri, 6 Dec 2024 13:45:16 -0500 Subject: [PATCH 3/4] add release note --- ...0472-review-modify-jsonprinter-for-datasetfieldtype.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 doc/release-notes/10472-review-modify-jsonprinter-for-datasetfieldtype.md diff --git a/doc/release-notes/10472-review-modify-jsonprinter-for-datasetfieldtype.md b/doc/release-notes/10472-review-modify-jsonprinter-for-datasetfieldtype.md new file mode 100644 index 00000000000..f0b9c30c9cd --- /dev/null +++ b/doc/release-notes/10472-review-modify-jsonprinter-for-datasetfieldtype.md @@ -0,0 +1,8 @@ +### Json Printer Bug fix + +DatasetFieldTypes in MetadataBlock response that are also a child of another DatasetFieldType were being returned twice. The child DatasetFieldType was included in the "fields" object as well as in the "childFields" of it's parent DatasetFieldType. This fix suppresses the standalone object so only one instance of the DatasetFieldType is returned (in the "childFields" of its parent). +This fix changes the Json output of the API `/api/dataverses/{dataverseAlias}/metadatablocks` + +## Backward Incompatible Changes + +The Json response of API call `/api/dataverses/{dataverseAlias}/metadatablocks` will no longer include the DatasetFieldTypes in "fields" if they are children of another DatasetFieldType. The child DatasetFieldType will only be included in the "childFields" of it's parent DatasetFieldType. From a067793eab56bc9280ac8dd7b0628d071f48926b Mon Sep 17 00:00:00 2001 From: Steven Winship <39765413+stevenwinship@users.noreply.github.com> Date: Mon, 6 Jan 2025 09:44:32 -0500 Subject: [PATCH 4/4] fix merged test --- src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java b/src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java index deb3354b481..960e3d2e3ac 100644 --- a/src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java +++ b/src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java @@ -1012,8 +1012,9 @@ public void testListMetadataBlocks() { // Since the included property of geographicCoverage is set to false, we should retrieve the total number of fields minus one listMetadataBlocksResponse.then().assertThat() - .body(String.format("data[%d].fields.size()", geospatialMetadataBlockIndex), equalTo(6)); // 10 - 4 child duplicates + .body(String.format("data[%d].fields.size()", geospatialMetadataBlockIndex), equalTo(2)); + listMetadataBlocksResponse = UtilIT.getMetadataBlock("geospatial"); String actualGeospatialMetadataField1 = listMetadataBlocksResponse.then().extract().path(String.format("data.fields['geographicCoverage'].name")); String actualGeospatialMetadataField2 = listMetadataBlocksResponse.then().extract().path(String.format("data.fields['geographicCoverage'].childFields['country'].name")); String actualGeospatialMetadataField3 = listMetadataBlocksResponse.then().extract().path(String.format("data.fields['geographicCoverage'].childFields['city'].name"));