Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Correction of JsonPrinter output for metadatablock api calls #10764

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from
+73 −33
Open

Correction of JsonPrinter output for metadatablock api calls #10764

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
f5d1299
Merge pull request #10178 from IQSS/develop
landreev Dec 12, 2023
a218417
Merge pull request #10453 from IQSS/develop
landreev Apr 1, 2024
8c99a74
Merge pull request #10664 from IQSS/develop
pdurbin Jul 3, 2024
d197373
bugfix: create correct json output for metadatablock api call
ffritze Aug 9, 2024
db1c59c
added docu for the fix
ffritze Aug 14, 2024
9c480a3
corrected a word
ffritze Aug 14, 2024
65eee38
Merge branch 'develop' of https://github.com/IQSS/dataverse into mast…
ffritze Aug 27, 2024
7884bdb
added integration test: checking child / parent logic
ffritze Aug 27, 2024
7605345
integration test fix
ffritze Aug 28, 2024
886c8ad
Merge branch 'develop' of https://github.com/IQSS/dataverse into mast…
ffritze Oct 25, 2024
01b6762
Merge branch 'develop' into master_json_fix #10764
pdurbin Dec 16, 2024
3fd62f6
make changelog for 6.6, reword #10764
pdurbin Dec 16, 2024
cd3f0ef
Merge pull request #24 from IQSS/10764-updates
ffritze Dec 17, 2024
b2e271a
fix the tests: DataversesIT and MetadataBlocksIT
ffritze Dec 18, 2024
957adab
removed the tab
ffritze Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions doc/release-notes/master_json_fix.md
View file
Open in desktop
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
Open in desktop
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.4
----

- /api/metadatablocks is now returning no duplicated metadata properties and does not ommit metadata properties when called. The JsonPrinter class output is fixed.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- /api/metadatablocks is now returning no duplicated metadata properties and does not ommit metadata properties when called. The JsonPrinter class output is fixed.
- **/api/metadatablocks** is now returning no duplicated metadata properties and does not omit metadata properties when called.

I'm suggesting that we remove the reference to JsonPrinter because API users don't need to worry about the underlying implementation.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ffritze
The requested change was not made. Also this now has to be added under V6.5 in the changeling.rst


v6.3
----

Expand Down
14 changes: 9 additions & 5 deletions src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@
import jakarta.ejb.Singleton;
import jakarta.json.JsonArray;
import jakarta.json.JsonObject;
import java.math.BigDecimal;
import java.util.function.Predicate;

/**
* Convert objects to Json.
Expand Down Expand Up @@ -639,9 +639,13 @@ public static JsonObjectBuilder json(MetadataBlock metadataBlock, boolean printO
jsonObjectBuilder.add("displayOnCreate", metadataBlock.isDisplayOnCreate());

JsonObjectBuilder fieldsBuilder = Json.createObjectBuilder();
Set<DatasetFieldType> datasetFieldTypes = new TreeSet<>(metadataBlock.getDatasetFieldTypes());

for (DatasetFieldType datasetFieldType : datasetFieldTypes) {

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);
Expand All @@ -658,7 +662,7 @@ public static JsonObjectBuilder json(MetadataBlock metadataBlock, boolean printO
fieldsBuilder.add(datasetFieldType.getName(), json(datasetFieldType, ownerDataverse));
}
}

jsonObjectBuilder.add("fields", fieldsBuilder);
return jsonObjectBuilder;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm just going to put this comment here at the bottom but @ffritze can you please get the following test passing?

edu.harvard.iq.dataverse.api.DataversesIT.testListMetadataBlocks is failing with this:

1 expectation failed.
JSON path data[1].fields.size() doesn't match.
Expected: <10>
  Actual: <2>

I also mentioned this here: https://dataverse.zulipchat.com/#narrow/stream/378866-troubleshooting/topic/metadatablocks.20api/near/460096243

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I start mvn clean package from my pull request branch which has dataverse 6.3 as basis there are no failing tests:
[INFO] Tests run: 7, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.020 s -- in edu.harvard.iq.dataverse.feedback.FeedbackUtilTest
[INFO]
[INFO] Results:
[INFO]
[WARNING] Tests run: 1595, Failures: 0, Errors: 0, Skipped: 36
[INFO]
[INFO]
[INFO] --- jacoco:0.8.11:report (jacoco-after-unit) @ dataverse ---
[INFO] Loading execution data file /home/ff/github/dataverse/target/coverage-reports/jacoco-unit.exec
[INFO] Analyzed bundle 'dataverse' with 1044 classes
[INFO]
[INFO] --- war:3.3.2:war (default-war) @ dataverse ---
[INFO] Packaging webapp
[INFO] Assembling webapp [dataverse] in [/home/ff/github/dataverse/target/dataverse]
[INFO] Processing war project
[INFO] Copying webapp resources [/home/ff/github/dataverse/src/main/webapp]
[INFO] Building war: /home/ff/github/dataverse/target/dataverse-6.3.war
[INFO] Packaging classes
[INFO] Building jar: /home/ff/github/dataverse/target/dataverse-6.3-classes.jar
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:02 min
[INFO] Finished at: 2024-08-26T08:06:19+02:00
[INFO] ------------------------------------------------------------------------

That's why I am not quite sure if the failing test has something to do with my pull request. Should I switch to develop and try to fix the test anyway?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ffritze

Those tests executed at build time are unit tests. @pdurbin is referring to the integration tests, and in particular the test testListMetadataBlocks, present in: DataversesIT.java.

To run integration tests, you need to have the environment running locally. You can do this by using the containerized dev environment.

Steps:

  1. Use mvn -Pct package to build the application image.
  2. Use docker-compose -f docker-compose-dev.yml up to run the containerized environment
  3. When all containers are up and running, execute: mvn test -Dtest=DataversesIT and all the IT tests from DataversesIT will be executed.

I recommend that you update this branch with develop, as the metadata blocks endpoint was recently modified, and I'm not sure if you have the latest changes.

The test may fail because of the changes you have made. There are assertions in the test that check the number of fields returned, and your changes likely reduced this number by removing duplicate child fields that were previously listed (repeated) as parents.

Expand Down