Correction of JsonPrinter output for metadatablock api calls #10764

ffritze · 2024-08-09T08:42:17Z

What this PR does / why we need it:
This fixes the issue that some metadata properties are ommitted or duplicated in the JsonPrinter output if someone is calling the api/metadablock endpoint.

Which issue(s) this PR closes:

Closes #

Special notes for your reviewer:

Suggestions on how to test this:
All tests are passed.

Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No user interface changes it only affects the api json output.

Is there a release notes update needed for this change?:

Additional documentation:
The fix filters the metadata properties out of a list which are chield fields. Only parent properties are in the list which are then cast to an ordered set. The order logic is not changed. The addition of child properties is not changed because it is called correctly in another json() method in the JsonPrinter class.

Merge v6.1 into master

Merge 6.2 into master

merge develop into master for 6.3

pdurbin · 2024-08-12T14:45:30Z

See also discussion at https://dataverse.zulipchat.com/#narrow/stream/378866-troubleshooting/topic/metadatablocks.20api/near/457293229

@GPortas I requested a review from you because you last worked on this code in PR #10642 to fix #10637.

I have to admit I haven't looked deeply into this!

pdurbin

Some more feedback.

pdurbin · 2024-08-20T14:06:50Z

doc/sphinx-guides/source/api/changelog.rst

+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.


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.

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

pdurbin · 2024-08-20T14:08:31Z

src/main/java/edu/harvard/iq/dataverse/util/json/JsonPrinter.java

@@ -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;
    }


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

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?

@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:

Use mvn -Pct package to build the application image.

Use docker-compose -f docker-compose-dev.yml up to run the containerized environment

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.

GPortas

Integration tests should be updated. Possibly new test cases should be added to verify that the child field exclusion logic works as expected.

Please read my comment above.

…er_json_fix

ffritze · 2024-08-29T06:32:02Z

@pdurbin @GPortas The branch was updated. The integration test is now passing.

…er_json_fix

ffritze · 2024-11-20T11:56:54Z

@stevenwinship I am not quite sure if there is an open task for me regarding this pull request. Is there still something to do from my side?

stevenwinship · 2024-12-16T13:59:29Z

@ffritze
Please make the above change and get the tests to pass

pdurbin · 2024-12-16T14:20:18Z

@ffritze I made a PR for you:

10764 updates TIK-NFL/dataverse#24

Please take a look.

10764 updates

coveralls · 2024-12-17T12:08:28Z

coverage: 22.571%. remained the same
when pulling 957adab on TIK-NFL:master_json_fix
into e3b5795 on IQSS:develop.

ffritze · 2024-12-17T13:01:15Z

@pdurbin Thanks for the pull request. I merged into my branch and now the "continuous integration" test is failing. Does that matter?

pdurbin · 2024-12-17T14:37:28Z

@ffritze yes, it matters. 😄

Here are the latest test failures:

https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-10764/5/testReport/edu.harvard.iq.dataverse.api/DataversesIT/testListMetadataBlocks/ is showing this:

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

https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-10764/5/testReport/edu.harvard.iq.dataverse.api/MetadataBlocksIT/testListMetadataBlocks/ is showing this:

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

@GPortas gave some advice on running DataversesIT here: #10764 (comment) . Can you please give that a try? And if it works, try running the MetadataBlocksIT tests. If you need help running these tests, please feel free to start a thread in #dev over at https://dataverse.zulipchat.com . Thanks!

ffritze · 2024-12-18T09:54:56Z

@pdurbin I have fixed both of the two tests. When I run it locally, all tests are passing. Could you please provide me with another error log regarding the jenkins fail. Thanks in advance.

pdurbin · 2024-12-18T15:39:01Z

@ffritze thanks for adding those tests!

https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-10764/8/consoleFull failed due to an unrelated S3 bucket error:

failure during "create localstack bucket for testing" step gdcc/dataverse-ansible#387

I just kicked off another run: https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-10764/9/

landreev and others added 4 commits December 12, 2023 15:34

Merge pull request IQSS#10178 from IQSS/develop

f5d1299

Merge v6.1 into master

Merge pull request IQSS#10453 from IQSS/develop

a218417

Merge 6.2 into master

Merge pull request IQSS#10664 from IQSS/develop

8c99a74

merge develop into master for 6.3

bugfix: create correct json output for metadatablock api call

d197373

pdurbin requested a review from GPortas August 12, 2024 14:43

ffritze added 2 commits August 14, 2024 08:11

added docu for the fix

db1c59c

corrected a word

9c480a3

pdurbin reviewed Aug 20, 2024

View reviewed changes

GPortas reviewed Aug 26, 2024

View reviewed changes

ffritze added 2 commits August 27, 2024 09:04

Merge branch 'develop' of https://github.com/IQSS/dataverse into mast…

65eee38

…er_json_fix

added integration test: checking child / parent logic

7884bdb

ffritze force-pushed the master_json_fix branch from edf12ec to 7884bdb Compare August 27, 2024 08:23

integration test fix

7605345

pdurbin added the Size: 3 A percentage of a sprint. 2.1 hours. label Sep 3, 2024

pdurbin added the Type: Bug a defect label Oct 9, 2024

cmbz added FY25 Sprint 8 FY25 Sprint 8 (2024-10-09 - 2024-10-23) FY25 Sprint 9 FY25 Sprint 9 (2024-10-23 - 2024-11-06) labels Oct 23, 2024

Merge branch 'develop' of https://github.com/IQSS/dataverse into mast…

886c8ad

…er_json_fix

sekmiller assigned sekmiller and unassigned sekmiller Nov 1, 2024

cmbz added the FY25 Sprint 10 FY25 Sprint 10 (2024-11-06 - 2024-11-20) label Nov 7, 2024

stevenwinship assigned stevenwinship and unassigned stevenwinship Nov 13, 2024

stevenwinship added the Status: Needs Input Applied to issues in need of input from someone currently unavailable label Nov 13, 2024

stevenwinship assigned stevenwinship and ffritze Nov 13, 2024

pdurbin added 2 commits December 16, 2024 09:13

Merge branch 'develop' into master_json_fix IQSS#10764

01b6762

make changelog for 6.6, reword IQSS#10764

3fd62f6

pdurbin mentioned this pull request Dec 16, 2024

10764 updates TIK-NFL/dataverse#24

Merged

Merge pull request #24 from IQSS/10764-updates

cd3f0ef

10764 updates

pdurbin removed the Status: Needs Input Applied to issues in need of input from someone currently unavailable label Dec 17, 2024

ffritze added 2 commits December 18, 2024 09:43

fix the tests: DataversesIT and MetadataBlocksIT

b2e271a

removed the tab

957adab

pdurbin assigned pdurbin and unassigned stevenwinship Dec 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correction of JsonPrinter output for metadatablock api calls #10764

Correction of JsonPrinter output for metadatablock api calls #10764

ffritze commented Aug 9, 2024

pdurbin commented Aug 12, 2024

pdurbin left a comment

pdurbin Aug 20, 2024

stevenwinship Nov 13, 2024

pdurbin Aug 20, 2024

ffritze Aug 26, 2024

GPortas Aug 26, 2024

GPortas left a comment

ffritze commented Aug 29, 2024

ffritze commented Nov 20, 2024

stevenwinship commented Dec 16, 2024

pdurbin commented Dec 16, 2024

coveralls commented Dec 17, 2024 •

edited

Loading

ffritze commented Dec 17, 2024

pdurbin commented Dec 17, 2024

ffritze commented Dec 18, 2024

pdurbin commented Dec 18, 2024

	- /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.

Correction of JsonPrinter output for metadatablock api calls #10764

Are you sure you want to change the base?

Correction of JsonPrinter output for metadatablock api calls #10764

Conversation

ffritze commented Aug 9, 2024

pdurbin commented Aug 12, 2024

pdurbin left a comment

Choose a reason for hiding this comment

pdurbin Aug 20, 2024

Choose a reason for hiding this comment

stevenwinship Nov 13, 2024

Choose a reason for hiding this comment

pdurbin Aug 20, 2024

Choose a reason for hiding this comment

ffritze Aug 26, 2024

Choose a reason for hiding this comment

GPortas Aug 26, 2024

Choose a reason for hiding this comment

GPortas left a comment

Choose a reason for hiding this comment

ffritze commented Aug 29, 2024

ffritze commented Nov 20, 2024

stevenwinship commented Dec 16, 2024

pdurbin commented Dec 16, 2024

coveralls commented Dec 17, 2024 • edited Loading

ffritze commented Dec 17, 2024

pdurbin commented Dec 17, 2024

ffritze commented Dec 18, 2024

pdurbin commented Dec 18, 2024

coveralls commented Dec 17, 2024 •

edited

Loading