DS-4495 Restricted endpoints are sometimes the only HAL link path to public endpoints #2766
Conversation
…links to standard nested endpoint, plus IT
abollini
added
the
interface: REST API v7+
There was a problem hiding this comment.
Overall, this looks fine to me. I'm OK with this route, but had a question inline. I haven't tested this yet though, as I'd like feedback from the Angular team (@artlowel perhaps) on whether this solves the issues they saw in DS-4495.
| discoverableEndpointsService | ||
| .register(ResourcePolicyRestRepository.class , Arrays.asList(new Link("/api/" | ||
| + ResourcePolicyRest.CATEGORY + "/" + ResourcePolicyRest.NAME + "/search", | ||
| ResourcePolicyRest.NAME + "-search"))); |
There was a problem hiding this comment.
I'm assuming there's no way to do this in the RestRepository classes themselves? The code here looks reasonable, but it just seems slightly odd to me that we have to do this in a separate class instead of finding a way to register these endpoints in the class that creates them (for example, registering this endpoint from within the ResourcePolicyRestRespository which creates/generates this /search subpath).
That said, if this is the easiest route, I'm OK with this. We'll just need to remember to modify this class whenever a new nested link needs to be visible at the root level.
There was a problem hiding this comment.
This approach will work for the UI. We will need some work to migrate to these endpoints, but as they're hardcoded right now, merging this PR shouldn't break anything.
I agree with @tdonohue's point about the location though. Preferably each Repository would register its own links rather than having them all in a single file
|
@artlowel @tdonohue the endpoint can be registered in each Repository if liked. The reason why I have suggested to do that in a separated central class it to makes easier to understand / locate which endpoints have this special need. I would prefer to monitor these special cases, putting them in the different repository would imply a bit of code duplication (the after afterPropertiesSet, not an huge issue of course) but more relevant to me will make harder to monitor that developers not start to add addition unnecessary endpoint registration to the root. Also the testing would become a bit more expensive if distributed over different classes. Anyway, let me know if you still want to split that over the different repositories |
|
@abollini : I'd prefer splitting this PR apart into the separate What I'm seeing is that (almost) anytime the All in all, I think this will be much easier for reviewers to check future PRs if this code moves to the RestRepository class. Otherwise, I'm worried we'll accidentally recreate this same problem down the line if we forget to register public subpaths anytime we return a 405 at the root endpoint. |
References
https://jira.lyrasis.org/browse/DS-4495
Description
A class has been added that allows to add the root endpoints the links to standard nested endpoint that are not discoverable due to limitation to access some resource collection endpoint via GET
Checklist
This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!
400 Bad Request,401 Unauthorized,403 Forbidden,404 Not Found, etc)pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.