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.

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

#1612 - Add or Replace File via API #3415

Closed
wants to merge 92 commits into from
Closed

Conversation

raprasad
Copy link
Contributor

@raprasad raprasad commented Oct 19, 2016

RFI Checklist

Before submitting the pull request, fill out sections (1.) Related Issues and (2.) Pull Request Checklist.

1. Related Issues

#1612 Allow file uploads for non-zip files through the API


2. Pull Request Checklist

  • Functionality completed as described in FRD
  • Dependencies, risks, assumptions in FRD addressed
  • Unit tests completed
  • REST Assured tests added or modified: DatasetsIT, FilesIT
  • Deployment requirements identified (e.g., SQL scripts, indexing)
  • Documentation completed
  • All code checkins completed

3. Review Checklist

After the pull request has been submitted, fill out this section.

  • Code review completed or waived
  • Testing requirements completed
  • Usability testing completed or waived
  • Support testing completed or waived
  • Merged with develop branch and resolved conflicts

Connects to #1612

raprasad and others added 30 commits September 21, 2016 12:55
…ual method. Added direct method for checking a single file--that's for #2290
…heck that the state of the Dataset is correct, etc
…n -- e.g. why is it not going through DataFileServiceBean.save
@raprasad
Copy link
Contributor Author

fyi: found a bug with ADD.

REPLACE is retricted to a single file--even if a single '.zip' is "unzipped into multiple files.
ADD should not have this restriction.

Fix:

  • Allow add of single file (e.g. zip) to result in multiple files in dataset
  • Return appropriate JSON listing all files returned

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 8.618% when pulling 78f3267 on 1612-api-add-file into 8ad3431 on develop.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 8.618% when pulling b2dafc6 on 1612-api-add-file into 8ad3431 on develop.

…ve status http error codes to add/replace api calls
@coveralls
Copy link

Coverage Status

Coverage increased (+0.01%) to 8.616% when pulling 078fbd4 on 1612-api-add-file into 8ad3431 on develop.

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.

I'm wondering if it's possible to stick to the Java EE Standard (JAX-RS).

@@ -1,8 +1,18 @@
package edu.harvard.iq.dataverse.api;

import javax.ws.rs.ApplicationPath;
import javax.ws.rs.core.Application;
import org.glassfish.jersey.media.multipart.MultiPartFeature;
import org.glassfish.jersey.server.ResourceConfig;
Copy link
Member

Choose a reason for hiding this comment

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

@scolapasta are we consciously switching from a JAVA EE standard (JAX-RS) to an specific implementation (Jersey)?

@raprasad
Copy link
Contributor Author

raprasad commented Oct 26, 2016

Post code review checklist

  • For file add/replace remove ability to add DataFile tags
  • Use native JSON. e.g. Add new attributes to the native JSON, exclude dataset and dataverse information
  • For add file API endpoint, we now have the dataset object--pass directly to AddReplaceHelper instead of the id
    • Redo business logic
    • Redo tests
  • Tech debt: Update existing findDatasetOrDie method to use error messages in bundles
    • Rename existing bundle errors
    • Update add/replace business logic to use renamed bundle errors
    • Update API tests to use renamed bundle errors
  • Move removeDuplicatesNullsEmptyStrings to a utility class
    • Which one?
  • For OptionalFileParams, rename tags to categories in code/methods
    • Rename business logic methods
    • Update tests
    • There's an inconsistency where the UI/user facing logic has the word tags but internal code uses categories
      • Group decision to keep categories in the code
  • AddReplaceHelper logic
    • Consolidate calls to UpdateDatasetCommand in step_070_run_update_dataset_command and step_080_run_update_dataset_command_for_replace
  • OptionalFileParams.java. addCategoryByName already checks for existing categories, remove it from this class
  • leftover code in API endpoint where testFileInputStream used instead of fileInputStream
  • Use code coverage reports to see how much of the new code is being exercised by unit tests and attempt to raise the amount of code coverage of new code, if possible. See Set testing expectations in Developer Guide and document tools (Jacoco, Coveralls, REST Assured) #3431 for details on how to measure code coverage.
  • Favor using the pattern of checking if a command can be executed rather than checking a specific permission. Here's an example from StatementManagerImpl.java for checking if a command can be executed: if (!permissionService.isUserAllowedOn(user, new GetDraftDatasetVersionCommand(dvReq, dataset), dataset))
  • Rework addFileToDataset in Datasets.java to use command pattern and look more like the createPrivateUrl method in that class? Just like how there’s a CreatePrivateUrlCommand we could add a AddFileToDatasetCommand. This might help AddReplaceFileHelper.java get smaller and make the code generally more testable.
  • Add REST Assured test for to add a tiny file (using the new native add, of course) to try to reproduce File Upload: Uploading a tiny text file causes a bufferunderflow log error but works. #1719.
  • Add REST Assured test to see if the message seen when trying to upload a duplicate file matches what is shown in the GUI. Hopefully this means we will be able to put File Upload: Improve message when user tries to upload duplicate file. #2467 through QA.
  • File add via native API must enforced the :MaxFileUploadSizeInBytes limit that is enforced by the GUI and by SWORD. This doc should be updated as well: http://guides.dataverse.org/en/4.5.1/installation/config.html#maxfileuploadsizeinbytes

Include in this ticket?

  • Rename the ingestService.addFiles(..) method to ingestService.saveFiles(..)
    • retest: DatasetPage UI, EditDataFiles UI, and MediaResourceManagerImpl.java

Important Prov. note related #3238

  • The add/replace API endpoints currently have a "pathway" to add additional JSON variables. e.g. If you're adding string, numeric, boolean or other JSON based data
  • However, the API endpoints will need the ability to add a second file--a prov file.

@raprasad
Copy link
Contributor Author

raprasad commented Oct 31, 2016

This is frozen. Dev has switched to https://github.com/IQSS/dataverse/tree/2290-file-replace

@raprasad raprasad closed this Oct 31, 2016
@poikilotherm poikilotherm deleted the 1612-api-add-file branch July 1, 2020 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants