-
Notifications
You must be signed in to change notification settings - Fork 490
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.
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
Use JsonUtil.getJsonObject in AbstractApiBean, MakeDataCountApi #10062
Conversation
This fixes IQSS#10054. Like before, JsonException may still be thrown. Since this is a RuntimeException, I only mention it in the Javadoc.
This may become flaky if the indentation is dependent on implementation. GSON apparently keeps empty objects in one line and uses two spaces for indentation, whereas I see slightly different outputs.
It only has static methods and should not be instantiated.
Ooh, all checks have passed! 🎉 |
I like what I'm seeing here, including refactoring out Gson dependencies. Was just talking about this with @pdurbin a couple of days ago. While this served a purpose when it was introduced into Dataverse, a lot of things like this can now be handled with well vetted standards that have been developed since then. |
src/main/java/edu/harvard/iq/dataverse/engine/command/impl/GetProvJsonCommand.java
Outdated
Show resolved
Hide resolved
src/main/java/edu/harvard/iq/dataverse/api/MakeDataCountApi.java
Outdated
Show resolved
Hide resolved
JsonUtil.getJsonObject closes the Readers, but not the InputStream. It is the caller's responsibility to close the InputStream properly.
@qqmyers I think I fixed it. The javadoc for |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
This would be a good opportunity to see if we can add ProvIT to the API test suite. @jp-tosca you can run in manually like this:
For me it's failing. I'm not sure why:
Ideally we'd get this test working and add it to Here's how to test manually, if we must: https://guides.dataverse.org/en/6.0/api/native-api.html#get-provenance-json-for-an-uploaded-file We should also exercise To check API test coverage of this PR, see https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-10062/4/execution/node/3/ws/target/coverage-it/index.html You can see GetProvJsonCommand has zero coverage, for example: https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-10062/4/execution/node/3/ws/target/coverage-it/edu.harvard.iq.dataverse.engine.command.impl/index.html |
What this PR does / why we need it:
Rewrite methods to use
JsonUtil.getJsonObject
orJsonUtil.getJsonArray
, so that try-with-resources for parsing JSON happens in the same way across the updated methods.I added methods to
JsonUtil
that take anInputStream
or a file name and return the data asJsonObject
. The latter has a new name to prevent a clash of method signatures.I also removed calls to deprecated
Gson
methods and then proceeded to removeGson
completely fromJsonUtil
. Pretty-printing a JSON string now uses the methods fromJsonUtil
itself. Their output is a little different, which meant I had to change expected outcomes inJsonUtilTest#testPrettyPrint
.Which issue(s) this PR closes:
Closes #10054
Closes #10056
Special notes for your reviewer:
Like before,
JsonException
s may still be thrown.Since this is a
RuntimeException
, I only mention it in the Javadoc.Suggestions on how to test this:
Please review the changes to
JsonUtilTest
and see that unit tests pass. Also run the API tests, please.Does this PR introduce a user interface change? If mockups are available, please link/include them here:
No.
Is there a release notes update needed for this change?:
No.
Additional documentation:
n/a