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

[DS-3755] integration test for submission configuration endpoint #1929

Merged
merged 135 commits into from Feb 1, 2018

Conversation

@lap82
Copy link
Contributor

lap82 commented Jan 24, 2018

This PR ( built on top of #1889 ) contains:

  • base integration test
  • implementation for missed endpoint /api/config/submissiondefinitions/<:definition-name>/collections
abollini and others added 30 commits Nov 2, 2017
…erimental code to works without authorization)
@lap82

This comment has been minimized.

Copy link
Contributor Author

lap82 commented Jan 26, 2018

Thank you @tomdesair I already noted the problem, the merge with master is not completed again. Thank you for the tip!

@lap82

This comment has been minimized.

Copy link
Contributor Author

lap82 commented Jan 26, 2018

Ok, now the merge with the master branch is completed. I discovered another issue in the code base of the master and I fixed it. I have just to perform the changes requested by @tdonohue in the related PR #1889 to move this PR ready to merge.

Copy link
Member

tdonohue left a comment

Overall, this is looking good. But, after giving this a quick scan, I believe all my questions/suggestions from the previous PR (#1889) have not yet been implemented. I found the same code in this new PR.

That said, none of my comments should necessarily block this PR. But, I do feel there are minor refactors and additional comments that should be made. I also still don't much like the new class name DirectlyAddressableRestModel, but maybe that's just me.

@@ -210,7 +210,7 @@ protected String addBitstream(Context context, ItemArchive itarch, Item item, Fi
if (group != null)
{
authorizeService.removeAllPolicies(context, bs); // remove the default policy
authorizeService.createResourcePolicy(context, bs, group, null, ce.permissionsActionId, null);
authorizeService.createResourcePolicy(context, bs, group, null, ce.permissionsActionId, null, null, null, null, null);

This comment has been minimized.

Copy link
@tdonohue

tdonohue Jan 26, 2018

Member

I'm still seeing a large number of these createResourcePolicy() calls that now end in 5 null parameters. While it's not necessarily bad code, it looks very odd / unclear to me (and is extremely duplicative). So, as I mentioned in #1889, I wonder if we should have multiple createResourcePolicy() methods, with different sets of attributes passed in, rather than continually passing in nulls for the majority of the params.

So, for example, retain the existing authorizeService.createResourcePolicy(context, bs, group, null, ce.permissionsActionId, null) call in this line, and have its method code now call another createResourcePolicy() method with all 10 params, and the last 4 being set to null. This would cut down on a number of changes in the PR which simply append 4 more null attributes on every createResourcePolicy call.

Does this make sense? Or am I missing something in why we'd rather do it this way?

This comment has been minimized.

Copy link
@lap82

lap82 Jan 29, 2018

Author Contributor

Yes, I agree. I left the method createResourcePolicy() after refactoring the method to highlight the changes, no other reason.

}
@Override
public void doProcessing(Context context, Request req, InProgressSubmission wsi) {
// TODO Auto-generated method stub

This comment has been minimized.

Copy link
@tdonohue

tdonohue Jan 26, 2018

Member

I'm still seeing quite a few of these TODO Auto-generated method stub comments in what looks to be empty classes. Should we remove these classes too?

This comment has been minimized.

Copy link
@lap82

lap82 Jan 29, 2018

Author Contributor

I suggest to do not remove these classes. The unused classes will be remove on the next round (I left the classes on to simplify also diff with the history commit).

*
* @author Andrea Bollini (andrea.bollini at 4science.it)
* @author Tom Desair (tom dot desair at atmire dot com)
* @author Frederic Van Reet (frederic dot vanreet at atmire dot com)
*
*/
@RestController
@RequestMapping("/api/"+BitstreamRest.CATEGORY +"/"+ BitstreamRest.PLURAL_NAME + "/{uuid:[0-9a-fxA-FX]{8}-[0-9a-fxA-FX]{4}-[0-9a-fxA-FX]{4}-[0-9a-fxA-FX]{4}-[0-9a-fxA-FX]{12}}/content")
@RequestMapping("/api/" + BitstreamRest.CATEGORY + "/" + BitstreamRest.PLURAL_NAME
+ "/{uuid:[0-9a-fxA-FX]{8}-[0-9a-fxA-FX]{4}-[0-9a-fxA-FX]{4}-[0-9a-fxA-FX]{4}-[0-9a-fxA-FX]{12}}/content")

This comment has been minimized.

Copy link
@tdonohue

tdonohue Jan 26, 2018

Member

As noted in #1889, I'd like to see a comment that explains these very large regex. It could be an example of paths that should match here, or note that it's trying to match valid UUIDs (if that's what this is doing). It's not a very straightforward regex, so it needs comments.

This comment has been minimized.

Copy link
@abollini

abollini Jan 30, 2018

Member

yes please add a comment as suggested by @tdonohue
The requestMapping value matches valid string representation of an UUID

@PathVariable Integer id, @RequestParam(required = false) String projection) {
return findOneInternal(apiCategory, model, id, projection);
}

@RequestMapping(method = RequestMethod.GET, value = "/{id:[A-z0-9]+}")
@RequestMapping(method = RequestMethod.GET, value = "/{id:^(?!^\\d+$)(?!^[0-9a-fxA-FX]{8}-[0-9a-fxA-FX]{4}-[0-9a-fxA-FX]{4}-[0-9a-fxA-FX]{4}-[0-9a-fxA-FX]{12}$)[\\w+\\-]+$+}")

This comment has been minimized.

Copy link
@tdonohue

tdonohue Jan 26, 2018

Member

Another very long regex with no comments/examples. (There's also several more in this same class)

This comment has been minimized.

Copy link
@tomdesair

tomdesair Jan 30, 2018

Contributor

I agree that the regexes do not improve the readability of the code. In addition, I think it's the responsability of the RestRepository to parse a given String into the correct "ID format" (see the discussion in https://jira.duraspace.org/browse/DS-3785?focusedCommentId=57801&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-57801)

This comment has been minimized.

Copy link
@abollini

abollini Jan 30, 2018

Member

for now I recommend to add a comment in the javadoc to explain a bit
"The requestMapping value matches any alphanumeric string except than string composed by only digit or uuid string representation"


private <ID extends Serializable> ResourceSupport findRelEntryInternal(HttpServletRequest request, String apiCategory, String model,
String id, String rel, String relid, Pageable page, PagedResourcesAssembler assembler, String projection) {
checkModelPluralForm(apiCategory, model);
DSpaceRestRepository<RestModel, ID> repository = utils.getResourceRepository(apiCategory, model);
Class<RestModel> domainClass = repository.getDomainClass();
DSpaceRestRepository<DirectlyAddressableRestModel, ID> repository = utils.getResourceRepository(apiCategory, model);

This comment has been minimized.

Copy link
@tdonohue

tdonohue Jan 26, 2018

Member

As noted in #1889, I'm still not a fan of calling this class DirectlyAddressableRestModel, as the DirectlyAddressable portion has very little meaning to me. I'm not sure of a better name yet.

return deleteInternal(apiCategory, model, id);
}

@RequestMapping(method = RequestMethod.DELETE, value = "/{uuid:[0-9a-fxA-FX]{8}-[0-9a-fxA-FX]{4}-[0-9a-fxA-FX]{4}-[0-9a-fxA-FX]{4}-[0-9a-fxA-FX]{12}}")

This comment has been minimized.

Copy link
@tdonohue

tdonohue Jan 26, 2018

Member

Another long regex with no comment.

This comment has been minimized.

Copy link
@abollini

abollini Jan 30, 2018

Member

Same as above... maybe we can add define a constant UUID_REGEX, NO_UUID_OR_DIGIT_REGEX and put the comment on top of these constants

* @author Andrea Bollini (andrea.bollini at 4science.it)
*
*/
public interface DirectlyAddressableRestModel extends RestModel {

This comment has been minimized.

Copy link
@tdonohue

tdonohue Jan 26, 2018

Member

Here's the actual DirectlyAddressableRestModel class. I'm not sure I understand the terminology here, and why it is now "directly addressable" because it has a Category and Controller? Maybe I'm the only one though who feels the name here is obscure/unclear?

This comment has been minimized.

Copy link
@lap82

lap82 Jan 29, 2018

Author Contributor

Maybe RestAddressableModel is the name appropriate for this interface.

# The user interface you will be using for DSpace. Common usage is either xmlui or jspui
dspace.ui = xmlui
# The user interface you will be using for DSpace.
dspace.ui = dspace-spring-rest

This comment has been minimized.

Copy link
@tdonohue

tdonohue Jan 26, 2018

Member

Again, as noted in #1889, "dspace-spring-rest" isn't a "ui". We might want to consider renaming this configuration (either in this PR or a future one).

This comment has been minimized.

Copy link
@lap82

lap82 Jan 29, 2018

Author Contributor

I agree. Now I remove the dspace.ui configuration.

@lap82

This comment has been minimized.

Copy link
Contributor Author

lap82 commented Jan 26, 2018

Thanks @tdonohue to report the requested changes from #1889. In my previous comment what I would say is "I am still applying the changes proposed by Tim and then I will do the PR". Sorry for that!

@tdonohue

This comment has been minimized.

Copy link
Member

tdonohue commented Jan 26, 2018

Oh, sorry, @lap82. I had misunderstood your previous statement :) Now I understand that you still plan to apply my suggestions in the coming days. Thanks!

lap82 added 4 commits Jan 26, 2018
DS-3755 add links for the resource returned by the POST and PATCH method;
…pace.path or a dspace.rest.url could be introduced)
…bleRestModel)

DS-3755 add missing interface declaration (RestAddressableModel) for the search rest class
Copy link
Contributor

tomdesair left a comment

Overall this PR is OK. Nice job on the PATCH request implementation @lap82 👍!

I've posted a few remarks on which I would like some feedback.

I did not yet built and deployed this PR to give it a test run. I'll try to do that later today or tomorrow.

@@ -543,4 +546,25 @@ public static String getSourceVersion()
}
return toReturn;
}

public static List<String> diff(Collection fromCollection, Collection toCollection) throws DCInputsReaderException {

This comment has been minimized.

Copy link
@tomdesair

tomdesair Jan 30, 2018

Contributor

Since this method belongs to a general Util class, I think we should give this method a better name. "diff" can mean a lot of things:

  • Difference in metadata?
  • Difference in items?

If I understood the method correctly, we could name this method differenceInSubmissionFields.

addMetadata(context, dso, schema, element, qualifier, rr.getLanguage(), rr.getValue(),
rr.getAuthority(), rr.getConfidence());
}
if (idx == to && to>from) {

This comment has been minimized.

Copy link
@tomdesair

tomdesair Jan 30, 2018

Contributor

I think that if-tests if (idx == to && to<from) and if (idx == to && to>from) can be collapsed to if (idx == to) because the code inside the if-test is the same.

This comment has been minimized.

Copy link
@lap82

lap82 Jan 30, 2018

Author Contributor

No, because the operation move the resource, the order of the add metadata is important to save it to the right place.

This comment has been minimized.

Copy link
@tomdesair

tomdesair Feb 1, 2018

Contributor

Suppose idx == to is true, then

if to < from, we'll execute addMetadata(context, dso, schema, element, qualifier, moved.getLanguage(), moved.getValue(), moved.getAuthority(), moved.getConfidence())

if to > from, we'll execute addMetadata(context, dso, schema, element, qualifier, moved.getLanguage(), moved.getValue(), moved.getAuthority(), moved.getConfidence());

So it looks like that in both cases, we're doing the same thing. So I don't understand why the distinction is really necessary. But if you are really sure, you can leave this.

This comment has been minimized.

Copy link
@lap82

lap82 Feb 1, 2018

Author Contributor

The clue is that we have to move an element in the right position related to the element of the iteration. If idx == to then we have to insert the element. So the question is the "from" element goes before or after the current element of the iteration? Depends on the "to" index related to the "from" index.


private Map<String, UploadConfiguration> map;

public Map<String, UploadConfiguration> getMap() {

This comment has been minimized.

Copy link
@tomdesair

tomdesair Jan 30, 2018

Contributor

It might be useful to document what this map contains. What are the keys?

<label>Title</label>
<input-type>onebox</input-type>
<hint>Enter the name of the file.</hint>
<required>You must enter a main title for this item.</required>

This comment has been minimized.

Copy link
@tomdesair

tomdesair Jan 30, 2018

Contributor

I suppose this is about a file. So maybe we should rephrase this to "You must enter a name for this file."

@PathVariable Integer id, @RequestParam(required = false) String projection) {
return findOneInternal(apiCategory, model, id, projection);
}

@RequestMapping(method = RequestMethod.GET, value = "/{id:[A-z0-9]+}")
@RequestMapping(method = RequestMethod.GET, value = "/{id:^(?!^\\d+$)(?!^[0-9a-fxA-FX]{8}-[0-9a-fxA-FX]{4}-[0-9a-fxA-FX]{4}-[0-9a-fxA-FX]{4}-[0-9a-fxA-FX]{12}$)[\\w+\\-]+$+}")

This comment has been minimized.

Copy link
@tomdesair

tomdesair Jan 30, 2018

Contributor

I agree that the regexes do not improve the readability of the code. In addition, I think it's the responsability of the RestRepository to parse a given String into the correct "ID format" (see the discussion in https://jira.duraspace.org/browse/DS-3785?focusedCommentId=57801&page=com.atlassian.jira.plugin.system.issuetabpanels%3Acomment-tabpanel#comment-57801)

public UploadBitstreamRest buildUploadBitstream(ConfigurationService configurationService, Bitstream source) throws SQLException {
UploadBitstreamRest data = new UploadBitstreamRest();

for (MetadataValue md : source.getMetadata()) {

This comment has been minimized.

Copy link
@tomdesair

tomdesair Jan 30, 2018

Contributor

This feels like code that should go to a UploadBitstreamRestConverter class similar to how all other REST models are built in a converter class.

This comment has been minimized.

Copy link
@lap82

lap82 Jan 31, 2018

Author Contributor

I agreed with @tomdesair but investigated in depth I left this code outside a "converter" because at moment there are code that it is not initialized as spring bean (see org.dspace.app.rest.submit.step.UploadStep). The converter will be available after the submission reader enhancement (i.e. the element into item-submission.xml will defined as spring bean)

* @author Luigi Andrea Pascarelli (luigiandrea.pascarelli at 4science.it)
*
*/
public class MetadataValidation extends AbstractValidation {

This comment has been minimized.

Copy link
@tomdesair

tomdesair Jan 30, 2018

Contributor

Would it be possible to move this class to dspace-api? It looks like "metadata validation" is business logic and is not directly related to the REST API.

This comment has been minimized.

Copy link
@lap82

lap82 Jan 31, 2018

Author Contributor

At moment this class contains logic to transport error message to the client. Usually metadata validation in DSpace is a prerogative of the UI and the batch for example assume that the inserted values are pre validated.

xsi:schemaLocation="http://www.springframework.org/schema/beans
http://www.springframework.org/schema/beans/spring-beans-2.5.xsd">

<bean name="metadataValidation" class="org.dspace.app.rest.submit.step.validation.MetadataValidation" scope="prototype">

This comment has been minimized.

Copy link
@tomdesair

tomdesair Jan 30, 2018

Contributor

Can we move these beans (or the entire XML) to dspace/config/spring/rest? Then they end up in the config folder of the dspace install directory which makes them easier to modify afterwards.

This comment has been minimized.

Copy link
@lap82

lap82 Jan 31, 2018

Author Contributor

So, I suggest to postpone it. In the next round I try to move the xml because it depends by the classpath for launch the batch script in DSpace and dependency for the other webapps to the new REST API (the dspace-rest jar need to compare in the lib of dspace installation and in the other webapp classpath (oai, sword, etc)

xsi:schemaLocation="http://www.springframework.org/schema/beans
http://www.springframework.org/schema/beans/spring-beans-2.5.xsd">

<bean id="patchConfigurationService" class="org.dspace.app.rest.submit.PatchConfigurationService">

This comment has been minimized.

Copy link
@tomdesair

tomdesair Jan 30, 2018

Contributor

Can we move these beans (or the entire XML) to dspace/config/spring/rest? Then they end up in the config folder of the dspace install directory which makes them easier to modify afterwards.

This comment has been minimized.

Copy link
@lap82

lap82 Jan 31, 2018

Author Contributor

As above.

<label>Title</label>
<input-type>onebox</input-type>
<hint>Enter the name of the file.</hint>
<required>You must enter a main title for this item.</required>

This comment has been minimized.

Copy link
@tomdesair

tomdesair Jan 30, 2018

Contributor

I think this should be updated to "You must enter a name for this file".

Copy link
Member

abollini left a comment

I suggest to immediately apply the small improvements suggested by all the reviewers to make the code more readable and create issues for the thing that need to be postponed.

@lap82 lap82 removed the work in progress label Jan 31, 2018
@lap82

This comment has been minimized.

Copy link
Contributor Author

lap82 commented Jan 31, 2018

I just ended with the suggestions. I hope that this go well. I tried to do a simple javadoc, also because I have some limitation with the english :)
Need to create other ticket to manage Exception as reported by @abollini, please @tomdesair open a separated ticket and feel free to assign me. This PR is ready to merge!

Copy link
Member

tdonohue left a comment

👍 This now looks good to me (and all my previous concerns/questions have been addressed). Thanks for the hard work, @lap82! I think this is ready to merge, provided that someone creates the various follow-up tickets mentioned in comments above.

Copy link
Contributor

tomdesair left a comment

It seems that my concerns will be addressed in new PRs and tickets. So for now, I'm OK with this. Thanks @lap82 (and @abollini) for the great effort!

addMetadata(context, dso, schema, element, qualifier, rr.getLanguage(), rr.getValue(),
rr.getAuthority(), rr.getConfidence());
}
if (idx == to && to>from) {

This comment has been minimized.

Copy link
@tomdesair

tomdesair Feb 1, 2018

Contributor

Suppose idx == to is true, then

if to < from, we'll execute addMetadata(context, dso, schema, element, qualifier, moved.getLanguage(), moved.getValue(), moved.getAuthority(), moved.getConfidence())

if to > from, we'll execute addMetadata(context, dso, schema, element, qualifier, moved.getLanguage(), moved.getValue(), moved.getAuthority(), moved.getConfidence());

So it looks like that in both cases, we're doing the same thing. So I don't understand why the distinction is really necessary. But if you are really sure, you can leave this.

@lap82 lap82 merged commit e2fdc75 into DSpace:master Feb 1, 2018
14 checks passed
14 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+1.04%) to 25.489%
Details
security/snyk - dspace-api/pom.xml No dependency changes
Details
security/snyk - dspace-oai/pom.xml No dependency changes
Details
security/snyk - dspace-rdf/pom.xml No dependency changes
Details
security/snyk - dspace-rest/pom.xml No dependency changes
Details
security/snyk - dspace-services/pom.xml No dependency changes
Details
security/snyk - dspace-solr/pom.xml No dependency changes
Details
security/snyk - dspace-spring-rest/pom.xml No dependency changes
Details
security/snyk - dspace-sword/pom.xml No dependency changes
Details
security/snyk - dspace-swordv2/pom.xml No dependency changes
Details
security/snyk - dspace/modules/pom.xml No dependency changes
Details
security/snyk - dspace/pom.xml No dependency changes
Details
security/snyk - pom.xml No dependency changes
Details
@abollini abollini deleted the 4Science:DS-3755 branch Feb 1, 2018
@abollini

This comment has been minimized.

Copy link
Member

abollini commented Feb 1, 2018

Created JIRA issues for the postponed activities:
https://jira.duraspace.org/browse/DS-3825
https://jira.duraspace.org/browse/DS-3826

@tomdesair @tdonohue I hope to haven't forget anything, I will appreciate a double check here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.