fix: NullPointerExceptions in /api/mydata/retrieve #9581
Conversation
|
@vera I'd say we're having trouble deciding whether or not to make the MyData API official: 😅 Thanks for the pull request! There are some tests in DataRetrieverApiIT but it hasn't been added to tests/integration-tests.txt (not sure why) which means they aren't being executed on Jenkins. Are you set up to run IT (integration) tests? If so, do you want to try running those? You could also simply DataRetrieverApiIT to tests/integration-tests.txt to see what happens! 😄 |
|
FWIW: From dataverse/src/main/java/edu/harvard/iq/dataverse/ThumbnailServiceWrapper.java Lines 173 to 180 in 85bde07 |
|
@pdurbin I tried running the integration tests as described here: https://guides.dataverse.org/en/latest/developers/testing.html Unfortunately I seem to need some help to get this working... Running this command Running I am running Dataverse on localhost:8080 using the new dev docker container btw. @qqmyers Interesting. I didn't manipulate the database or the index, but I did use |
Ah, sorry, I mean if you add it there, then Jenkins will run it. In those docs you found we say this:
Just above there are example of only running one or two of those classes at a time: https://guides.dataverse.org/en/latest/developers/testing.html#writing-api-tests-with-rest-assured |
vera
force-pushed
the
fix-api-mydata-retrieve-npe
branch
from
542ffd2 to
37f6fc1
Compare
|
I see, thanks. Running it with |
|
@vera awesome, thanks. I know you can't see this because a firewall is blocking your IP address but the test you added is passing: https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-9581/3/testReport/edu.harvard.iq.dataverse.api/DataRetrieverApiIT/testRetrieveMyDataAsJsonString/
|
|
I'm seeing a failure at https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-9581/4/testReport/edu.harvard.iq.dataverse.api/DataRetrieverApiIT/testRetrieveMyDataAsJsonString/ and I just kicked off this job to verify it: https://github.com/gdcc/api-test-runner/actions/runs/6853346220 |
|
Looks like the pretty printing change we discussed at standup (which hasn't been merged yet) - because the test here checks the text instead of the json, the fact that the our prettyprint adds a \n that doesn't happen with gson would cause a failure like this. Perhaps the test here, which was just turned on, was written prior to an earlier prettyprint change. |
|
@vera @johannes-darms ok, as long as you're logged into GitHub, you should be able to see the error here: https://github.com/gdcc/api-test-runner/actions/runs/6853346220 Like Jim says it seems to be about pretty printing. |
|
@vera can you please resolve the merge conflicts and let us know if you need any help with the pretty printing change? |
…rieve-npe # Conflicts: # tests/integration-tests.txt
|
@pdurbin done, I hope the pretty printing fix is OK |
|
@vera thanks! Jenkins tests are still running. I'll keep an eye on them! Update: tests passed - https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-9581/6/testReport/ |
|
If you are still interested in this PR, can you please merge and resolve any merge conflicts with the latest from develop? If so, we can prioritize reviewing and QAing the changes. If we don’t hear from you by May 22, 2024, we’ll go ahead and close this PR (it can always be reopened after that date, if there is still interest). |
|
@scolapasta If I'm not mistaken we are waiting for the merge/review to happen. |
|
@johannes-darms there are merge conflicts in tests/integration-tests.txt that @scolapasta is asking you to resolve. |
# Conflicts: # tests/integration-tests.txt
|
@scolapasta resolved :) |
What this PR does / why we need it:
While using the API endpoint
api/v1/mydata/retrieve(which btw is not documented, I hope that doesn't mean it's not supposed to be used? :)) I encountered two NullPointerExceptions which this PR fixes:Which issue(s) this PR closes:
none
Special notes for your reviewer:
Suggestions on how to test this:
Unfortunately, I am not sure precisely in what situation the entity is null, since I encountered these exceptions while just playing around with the API
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: