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

Added support for the CRUD operations on the Collection and Community… #2277

Merged
merged 7 commits into from Feb 4, 2019

Conversation

@Raf-atmire
Copy link
Contributor

commented Dec 3, 2018

In this PR we have created support for CRUD operations on both the Community and Collection endpoints for the REST api. This includes:

  • A PUT method in the RestResourceController to be used for the repository.put methods which will take care of the updating. This is build with the createAndReturn example in mind and functions virtually in the same way.
  • The creation of the createAndReturn(create), put(update) and delete(delete) methods in the CollectionRestRepository and the CommunityRestRepository
  • We've added a class DSpaceObjectUtils that takes care of the replacing of the MetadataEntries so that we have one centralised place to manage this
  • CollectionRest and CommunityRest classes have been extended to now store a String representation of the UUID of the owningCommunity so we can use this to pass it along to the DB.
  • Additional tests have been added to the CollectionRestRepositoryIT and CommunityRestRepositoryIT to keep the functionality properly tested at all times
    This PR in its entirety will cover the full CRUD for both the Collection and the Community endpoints.

Jira link: https://jira.duraspace.org/browse/DS-4091
RestContract PR link: DSpace/Rest7Contract#33

@KevinVdV KevinVdV added the REST API v7 label Dec 3, 2018
@KevinVdV KevinVdV added this to the 7.0 milestone Dec 3, 2018
… REST endpoints - send parentCommunity as request parameter
@samuelcambien samuelcambien force-pushed the atmire:rest_comm_coll_management branch from 6b27f4a to 5af1eee Dec 18, 2018
… REST endpoints - remove owningCommunity field from CommunityRest and CollectionRest
@samuelcambien samuelcambien force-pushed the atmire:rest_comm_coll_management branch from 5af1eee to fb1a8b8 Dec 19, 2018
@KevinVdV

This comment has been minimized.

Copy link
Member

commented Dec 19, 2018

The changes that were asked in the rest contract (DSpace/Rest7Contract#33) were implemented in this PR.

Copy link
Member

left a comment

I did a quick review of this PR today and found some areas that could use minor improvement. I also noted a few areas where usage of log4j could be improved ... remember that we are now using log4j v2, so the way in which we initialize the logger is improved & we can now make use of parameterized log messages. While these are not required for log4j v2, it's worth starting to develop new habits around logging. See also this dspace-devel email thread: https://groups.google.com/d/msg/dspace-devel/Cwaex50_CHg/LWsYiQhwBgAJ

@Raf-atmire

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2019

@tdonohue All the feedback should be addressed and resolved. Could you please take another look when you find the time? Thanks!

@tdonohue

This comment has been minimized.

Copy link
Member

commented Jan 11, 2019

@Raf-atmire : I've rereviewed. Thanks for the JavaDocs updates. There are still some outstanding requests to enhance/improve some RuntimeExceptions (I've given some examples for how that can be done). I've also noted a few areas (see above inline notes) where I'm worried about possible NullPointerExceptions.

@Raf-atmire

This comment has been minimized.

Copy link
Contributor Author

commented Jan 16, 2019

@tdonohue I've altered the RuntimeExceptions and added a null check for the CollectionRestRepository. I've written a short answer about the CommunityRestRepository parent null check as well. Thank you for the review and could you give this another look when you find the time? Thanks!

Copy link
Member

left a comment

👍 Gave this another review today. It looks great now, thanks for your updates @Raf-atmire!

Copy link
Member

left a comment

Code looks fine via inspection (I haven't run it), only few remarks about the security checks that can be enforced in a separate PR if needed (please open JIRA issue for them in such case)

@@ -139,4 +160,107 @@ public CollectionResource wrapResource(CollectionRest collection, String... rels
return new CollectionResource(collection, utils, rels);
}

@Override
@PreAuthorize("hasAuthority('ADMIN')")

This comment has been minimized.

Copy link
@abollini

abollini Jan 31, 2019

Member

Should we check for ADD on the parent community instead than general admin rights?

This comment has been minimized.

Copy link
@tdonohue

tdonohue Jan 31, 2019

Member

I suspect we need to log a bug ticket to refactor createAndReturn().

As createAndReturn only takes in a Context object, I'm not sure it can use the more appropriate @PreAuthorize("hasPermission(#id, 'COMMUNITY', 'ADD')") annotation...because it doesn't have a place to grab the parent Community ID from.

So, while I agree with @abollini, I think this particular line needs to stay as-is, until we can refactor createAndReturn to allow for fine grained permission checks (I'll create a ticket for it)

This comment has been minimized.

Copy link
@tdonohue
@@ -60,18 +74,35 @@ public CommunityRestRepository() {
protected CommunityRest createAndReturn(Context context) throws AuthorizeException {

This comment has been minimized.

Copy link
@abollini

abollini Jan 31, 2019

Member

Should we check for ADD on the parent instead than general admin rights?

This comment has been minimized.

Copy link
@tdonohue

tdonohue Jan 31, 2019

Member

As I noted for CollectionRestRepository, I don't think we can fix this line until createAndReturn() is refactored to work better with Spring Security annotations.

@tdonohue

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

@Raf-atmire : Do you have any objections to fixing the Annotations above noted by @abollini ? I've analyzed them and noted which ones it seems like you can fix easily right now (and which will likely need to wait for later).

If you can fix them quickly (and Travis approves), ping me and I'll give a quick final review & merge immediately.

@abollini

This comment has been minimized.

Copy link
Member

commented Jan 31, 2019

If you change the annotations in this PR please consider to fix the IT tests as well including at least one test where a community admin perform such actions instead than a site administrator otherwise add a note about such test in the JIRA ticket created by @tdonohue

@Raf-atmire

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2019

@tdonohue I agree with the Annotation changes and I've implemented them as well as adding two ITs for both the Collection and Community Rest Repositories to verify their behavior

@tdonohue

This comment has been minimized.

Copy link
Member

commented Feb 4, 2019

@Raf-atmire : thanks for the quick changes! I've re-reviewed and this looks great. Since @abollini previously noted the "code looks fine", and his suggestions were all implemented, I'm going to consider that his approval as well. Merging shortly.

@tdonohue tdonohue merged commit 3e5c942 into DSpace:master Feb 4, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 29.792%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.