Skip to content

Comments

Support for deleting files using native API#9383

Merged
kcondon merged 28 commits intoIQSS:developfrom
ErykKul:3913_delete_file_endpoint
Mar 28, 2023
Merged

Support for deleting files using native API#9383
kcondon merged 28 commits intoIQSS:developfrom
ErykKul:3913_delete_file_endpoint

Conversation

@ErykKul
Copy link
Collaborator

@ErykKul ErykKul commented Feb 10, 2023

What this PR does / why we need it:
Support for deleting files using native API.

Which issue(s) this PR closes:

Closes #3913

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

@pdurbin pdurbin added Feature: API Feature: File Upload & Handling Size: 3 A percentage of a sprint. 2.1 hours. labels Feb 10, 2023
@pdurbin pdurbin self-assigned this Mar 9, 2023
Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Hi @ErykKul thanks for the pull request! People have wanted this for a long time!

I was going to test this but hit a snag when merging the latest from develop. Can you please try and check out my comments about this and other things? Thanks!

@ErykKul
Copy link
Collaborator Author

ErykKul commented Mar 13, 2023

@pdurbin thank you for your review. I have implemented the changes. I am not sure if the integration test that I wrote works correctly, as I could not run any integration tests. Maybe it has something to do with my testing on Payara 6, I do not know yet. Probably something to do with the configuration on my machine. I will try to fix it and verify the test.

@ErykKul
Copy link
Collaborator Author

ErykKul commented Mar 14, 2023

Integration test is working now.

@pdurbin
Copy link
Member

pdurbin commented Mar 14, 2023

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Looks good. Approved. I fixed up the code and docs and added more tests. Thanks, @ErykKul !

@pdurbin pdurbin removed their assignment Mar 15, 2023
@kcondon kcondon self-assigned this Mar 20, 2023
@kcondon
Copy link
Contributor

kcondon commented Mar 23, 2023

@ErykKul I am out today but your updated test would test the failing case. Thanks!

@ErykKul ErykKul force-pushed the 3913_delete_file_endpoint branch from a7a7608 to 69120b8 Compare March 24, 2023 11:22
@ErykKul
Copy link
Collaborator Author

ErykKul commented Mar 24, 2023

I thought I have found what the problem was, but I still can't run the integration tests. I am not sure if it only at my machine, or if there is a more general problem with the tests (the resulting aoi docker image does not work properly and many tests fail).

@pdurbin
Copy link
Member

pdurbin commented Mar 24, 2023

@ErykKul here's the latest from https://jenkins.dataverse.org/job/IQSS-Dataverse-Develop-PR/job/PR-9383/18/testReport/edu.harvard.iq.dataverse.api/FilesIT/testDeleteFile/

java.lang.AssertionError: 
JSON path data.files[0] doesn't match.
Expected: null
  Actual: {restricted=false, directoryLabel=data/subdir1, description=my description3, dataFile={filename=orcid_16x16.png, storageIdentifier=file://187143e3d0f-e782829cb196, checksum={type=SHA-1, value=ada69beeb122b099475efdaf88133c82bd0890b2}, description=my description3, id=314, filesize=1261, categories=[Data], creationDate=2023-03-24, contentType=image/png, rootDataFileId=-1}, label=orcid_16x16.png, categories=[Data], version=1, datasetVersionId=129}

	at edu.harvard.iq.dataverse.api.FilesIT.testDeleteFile(FilesIT.java:1979)

I haven't tried running the IT tests in docker-aio for quite some months. In the Containerization Working Group, we've talked about the possibility of using the new Dockerized Dataverse environment (PR #9439) for this, to allow developers like yourself (or me!) run the integration tests within Docker. Would that be of interest?

@pdurbin
Copy link
Member

pdurbin commented Mar 28, 2023

@ErykKul do you need a hand with this? How can I help?

@ErykKul
Copy link
Collaborator Author

ErykKul commented Mar 28, 2023

@pdurbin
I think that running the integration tests with the Dockerized Dataverse is a great idea. Maybe we could use docker for that as well? For example: https://hub.docker.com/_/docker. Right now, the aio-docker makes some assumptions about the environment that are not true for every system (I still could not run the integration tests lately...). I am not sure about this, but it looks to me that it is no longer possible to build aio-docker using latest Rocky Linux image, like specified in the docker file? I get errors about not sufficient entropy (it also takes very long time to start the Dataverse inside the aio-docker container). Also, the "ps" command is missing, preventing the solr starting properly (procps package is not installed by default). Finally, when I get everything running, then I get errors from the sword API... I am not sure how the continuous integration runs the integration tests, I was not successful in doing that (I did get them running when trying out payara 6, but I had upgraded many packages to do that, upgraded the jdk version to 17, changed some scripts, etc.).

I have tried to fix the integration test, but since I was not able to run it locally, I am waiting for the continuous integration to finish.

@ErykKul
Copy link
Collaborator Author

ErykKul commented Mar 28, 2023

@pdurbin, @kcondon
I have solved the entropy problem on my machine by installing the rng-tools. Now the tests run, so I was able to fix it.

@pdurbin
Copy link
Member

pdurbin commented Mar 28, 2023

@ErykKul thanks for the feedback on testing in Docker. I passed it along at https://dataverse.zulipchat.com/#narrow/stream/375812-containers/topic/running.20API.20tests if you'd like to continue talking there.

It's strange, the API test you wrote seems to run just fine for me in Dockerized Dataverse. To get set up I'm following the steps laid out at http://preview.guides.gdcc.io/en/develop/container/dev-usage.html

mvn -Pct clean package
mvn -Pct docker:run
scripts/dev/docker-final-setup.sh

Importantly, in that "final setup" script we run setup-all.sh with the --insecure flag which allows for API testing. (See also https://guides.dataverse.org/en/5.13/developers/testing.html#getting-set-up-to-run-rest-assured-tests )

Anyway, below is the failure I'm seeing. While I'm on this branch I'll see if I can fix it quick:

[ERROR]   FilesIT.testDeleteFile:1997 JSON path data.files[0].dataFile.filename doesn't match.
Expected: orcid_16x16.png
  Actual: cc0.png

Here's the full test run: testrun.txt

To run just your "delete file" test I used this:

mvn test -Dtest=FilesIT#testDeleteFile

@ErykKul
Copy link
Collaborator Author

ErykKul commented Mar 28, 2023

@pdurbin
"filename doesn't match" was fixed in my last commit - I was testing the wrong version of the dataset (1.0 i.s.o. draft).

@pdurbin
Copy link
Member

pdurbin commented Mar 28, 2023

@ErykKul great. Looking good! I also added a way to check the filename with some weird REST Assured syntax. I think we're good now. Back to QA! Thanks!

Copy link
Member

@pdurbin pdurbin left a comment

Choose a reason for hiding this comment

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

Ready for more QA.

@kcondon kcondon self-assigned this Mar 28, 2023
@kcondon kcondon merged commit bdd6fb6 into IQSS:develop Mar 28, 2023
@pdurbin pdurbin added this to the 5.14 milestone Apr 27, 2023
@cmbz cmbz added FY26 Sprint 14 FY26 Sprint 14 (2025-12-31 - 2026-01-14) and removed FY26 Sprint 14 FY26 Sprint 14 (2025-12-31 - 2026-01-14) labels Jan 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

Is a delete file endpoint available?

5 participants