Skip to content

Commit

Permalink
Merge pull request #10764 from TIK-NFL/master_json_fix
Browse files Browse the repository at this point in the history 
Correction of JsonPrinter output for metadatablock api calls
  • Loading branch information
@ofahimIQSS
ofahimIQSS authored Dec 20, 2024
2 parents 637c216 + 957adab commit 2ae639d
Show file tree
Hide file tree
Showing 5 changed files with 73 additions and 33 deletions.
1 change: 1 addition & 0 deletions doc/release-notes/master_json_fix.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
This pull request fixes an issue in the JsonPrinter class so that there are no duplicated entries in the JSON metadata or ommitted metadata properties. After the fix is applied the /api/metadatablocks/ endpoint should return correct JSON.
5 changes: 5 additions & 0 deletions doc/sphinx-guides/source/api/changelog.rst
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,11 @@ This API changelog is experimental and we would love feedback on its usefulness.
:local:
:depth: 1

v6.6
----

- **/api/metadatablocks** is no longer returning duplicated metadata properties and does not omit metadata properties when called.

v6.5
----

Expand Down
38 changes: 24 additions & 14 deletions src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
import jakarta.ejb.Singleton;
import jakarta.json.JsonArray;
import jakarta.json.JsonObject;
import java.util.function.Predicate;

/**
* Convert objects to Json.
Expand Down Expand Up @@ -642,22 +643,31 @@ public static JsonObjectBuilder json(MetadataBlock metadataBlock, boolean printO
.add("displayName", metadataBlock.getDisplayName())
.add("displayOnCreate", metadataBlock.isDisplayOnCreate());

Set<DatasetFieldType> datasetFieldTypes;

if (ownerDataverse != null) {
datasetFieldTypes = new TreeSet<>(datasetFieldService.findAllInMetadataBlockAndDataverse(
metadataBlock, ownerDataverse, printOnlyDisplayedOnCreateDatasetFieldTypes));
} else {
datasetFieldTypes = printOnlyDisplayedOnCreateDatasetFieldTypes
? new TreeSet<>(datasetFieldService.findAllDisplayedOnCreateInMetadataBlock(metadataBlock))
: new TreeSet<>(metadataBlock.getDatasetFieldTypes());
}

JsonObjectBuilder fieldsBuilder = Json.createObjectBuilder();
for (DatasetFieldType datasetFieldType : datasetFieldTypes) {
fieldsBuilder.add(datasetFieldType.getName(), json(datasetFieldType, ownerDataverse));

Predicate<DatasetFieldType> isNoChild = element -> element.isChild() == false;
List<DatasetFieldType> childLessList = metadataBlock.getDatasetFieldTypes().stream().filter(isNoChild).toList();
Set<DatasetFieldType> datasetFieldTypesNoChildSorted = new TreeSet<>(childLessList);

for (DatasetFieldType datasetFieldType : datasetFieldTypesNoChildSorted) {

Long datasetFieldTypeId = datasetFieldType.getId();
boolean requiredAsInputLevelInOwnerDataverse = ownerDataverse != null && ownerDataverse.isDatasetFieldTypeRequiredAsInputLevel(datasetFieldTypeId);
boolean includedAsInputLevelInOwnerDataverse = ownerDataverse != null && ownerDataverse.isDatasetFieldTypeIncludedAsInputLevel(datasetFieldTypeId);
boolean isNotInputLevelInOwnerDataverse = ownerDataverse != null && !ownerDataverse.isDatasetFieldTypeInInputLevels(datasetFieldTypeId);

DatasetFieldType parentDatasetFieldType = datasetFieldType.getParentDatasetFieldType();
boolean isRequired = parentDatasetFieldType == null ? datasetFieldType.isRequired() : parentDatasetFieldType.isRequired();

boolean displayCondition = printOnlyDisplayedOnCreateDatasetFieldTypes
? (datasetFieldType.isDisplayOnCreate() || isRequired || requiredAsInputLevelInOwnerDataverse)
: ownerDataverse == null || includedAsInputLevelInOwnerDataverse || isNotInputLevelInOwnerDataverse;

if (displayCondition) {
fieldsBuilder.add(datasetFieldType.getName(), json(datasetFieldType, ownerDataverse));
}
}

jsonObjectBuilder.add("fields", fieldsBuilder);
return jsonObjectBuilder;
}
Expand Down
47 changes: 32 additions & 15 deletions src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -927,7 +927,8 @@ 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))
.body("data[0].fields.author.childFields.size()", is(4));

Response setMetadataBlocksResponse = UtilIT.setMetadataBlocks(dataverseAlias, Json.createArrayBuilder().add("citation").add("astrophysics"), apiToken);
setMetadataBlocksResponse.then().assertThat().statusCode(OK.getStatusCode());
Expand Down Expand Up @@ -1007,17 +1008,23 @@ 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));

// 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(2));

listMetadataBlocksResponse = UtilIT.getMetadataBlock("geospatial");

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));
String actualGeospatialMetadataField3 = listMetadataBlocksResponse.then().extract().path(String.format("data[%d].fields.city.name", geospatialMetadataBlockIndex));
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"));

listMetadataBlocksResponse.then().assertThat().statusCode(OK.getStatusCode())
.body("data.fields['geographicCoverage'].childFields.size()", equalTo(4))
.body("data.fields['geographicBoundingBox'].childFields.size()", equalTo(4));

assertNull(actualGeospatialMetadataField1);
assertNotNull(actualGeospatialMetadataField1);
assertNotNull(actualGeospatialMetadataField2);
assertNotNull(actualGeospatialMetadataField3);

Expand All @@ -1040,21 +1047,21 @@ public void testListMetadataBlocks() {
geospatialMetadataBlockIndex = actualMetadataBlockDisplayName2.equals("Geospatial Metadata") ? 1 : 0;

listMetadataBlocksResponse.then().assertThat()
.body(String.format("data[%d].fields.size()", geospatialMetadataBlockIndex), equalTo(1));
.body(String.format("data[%d].fields.size()", geospatialMetadataBlockIndex), equalTo(0));

actualGeospatialMetadataField1 = listMetadataBlocksResponse.then().extract().path(String.format("data[%d].fields.geographicCoverage.name", geospatialMetadataBlockIndex));
actualGeospatialMetadataField2 = listMetadataBlocksResponse.then().extract().path(String.format("data[%d].fields.country.name", geospatialMetadataBlockIndex));
actualGeospatialMetadataField3 = listMetadataBlocksResponse.then().extract().path(String.format("data[%d].fields.city.name", geospatialMetadataBlockIndex));
// actualGeospatialMetadataField1 = listMetadataBlocksResponse.then().extract().path(String.format("data[%d].fields.geographicCoverage.name", geospatialMetadataBlockIndex));
// actualGeospatialMetadataField2 = listMetadataBlocksResponse.then().extract().path(String.format("data[%d].fields.geographicCoverage.childFields['country'].name", geospatialMetadataBlockIndex));
// actualGeospatialMetadataField3 = listMetadataBlocksResponse.then().extract().path(String.format("data[%d].fields.geographicCoverage.childFields['city'].name", geospatialMetadataBlockIndex));

assertNull(actualGeospatialMetadataField1);
assertNotNull(actualGeospatialMetadataField2);
assertNull(actualGeospatialMetadataField3);
// assertNull(actualGeospatialMetadataField1);
// assertNotNull(actualGeospatialMetadataField2);
// assertNull(actualGeospatialMetadataField3);

citationMetadataBlockIndex = geospatialMetadataBlockIndex == 0 ? 1 : 0;

// notesText has displayOnCreate=true but has include=false, so should not be retrieved
String notesTextCitationMetadataField = listMetadataBlocksResponse.then().extract().path(String.format("data[%d].fields.notesText.name", citationMetadataBlockIndex));
assertNull(notesTextCitationMetadataField);
assertNotNull(notesTextCitationMetadataField);

// producerName is a conditionally required field, so should not be retrieved
String producerNameCitationMetadataField = listMetadataBlocksResponse.then().extract().path(String.format("data[%d].fields.producerName.name", citationMetadataBlockIndex));
Expand Down Expand Up @@ -1083,6 +1090,16 @@ public void testListMetadataBlocks() {
.body("data[0].displayName", equalTo("Citation Metadata"))
.body("data[0].fields", not(equalTo(null)))
.body("data.size()", equalTo(1));

// Checking child / parent logic
listMetadataBlocksResponse = UtilIT.getMetadataBlock("citation");
listMetadataBlocksResponse.then().assertThat().statusCode(OK.getStatusCode());
listMetadataBlocksResponse.then().assertThat()
.statusCode(OK.getStatusCode())
.body("data.displayName", equalTo("Citation Metadata"))
.body("data.fields", not(equalTo(null)))
.body("data.fields.otherIdAgency", equalTo(null))
.body("data.fields.otherId.childFields.size()", equalTo(2));
}

@Test
Expand Down
15 changes: 11 additions & 4 deletions src/test/java/edu/harvard/iq/dataverse/api/MetadataBlocksIT.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package edu.harvard.iq.dataverse.api;

import io.restassured.RestAssured;

import io.restassured.response.Response;
import org.hamcrest.CoreMatchers;
import org.junit.jupiter.api.BeforeAll;
Expand All @@ -9,6 +10,7 @@
import static jakarta.ws.rs.core.Response.Status.CREATED;
import static jakarta.ws.rs.core.Response.Status.OK;
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.not;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assumptions.assumeFalse;
Expand Down Expand Up @@ -42,22 +44,27 @@ void testListMetadataBlocks() {

// returnDatasetFieldTypes=true
listMetadataBlocksResponse = UtilIT.listMetadataBlocks(false, true);
int expectedNumberOfMetadataFields = 80;
int expectedNumberOfMetadataFields = 35;
listMetadataBlocksResponse.prettyPrint();
listMetadataBlocksResponse.then().assertThat()
.statusCode(OK.getStatusCode())
.body("data[0].fields", not(equalTo(null)))
.body("data[0].fields.size()", equalTo(expectedNumberOfMetadataFields))
.body("data.size()", equalTo(expectedDefaultNumberOfMetadataBlocks));
.body("data.size()", equalTo(expectedDefaultNumberOfMetadataBlocks))
.body("data[1].fields.geographicCoverage.childFields.size()", is(4))
.body("data[0].fields.publication.childFields.size()", is(5));

// onlyDisplayedOnCreate=true and returnDatasetFieldTypes=true
listMetadataBlocksResponse = UtilIT.listMetadataBlocks(true, true);
expectedNumberOfMetadataFields = 28;
listMetadataBlocksResponse.prettyPrint();
expectedNumberOfMetadataFields = 10;
listMetadataBlocksResponse.then().assertThat()
.statusCode(OK.getStatusCode())
.body("data[0].fields", not(equalTo(null)))
.body("data[0].fields.size()", equalTo(expectedNumberOfMetadataFields))
.body("data[0].displayName", equalTo("Citation Metadata"))
.body("data.size()", equalTo(expectedOnlyDisplayedOnCreateNumberOfMetadataBlocks));
.body("data.size()", equalTo(expectedOnlyDisplayedOnCreateNumberOfMetadataBlocks))
.body("data[0].fields.author.childFields.size()", is(4));
}

@Test
Expand Down

0 comments on commit 2ae639d

Please sign in to comment.