All REST exposed config endpoint #9140
Conversation
J4bbi
added
new feature
interface: REST API v7+
There was a problem hiding this comment.
Thank you again @J4bbi for this contribution! I can see, when the Angular interface addresses it, we will notice improvements. And also thank you for fixing and improving the documentation!
I've just one single request that is to add java docs. The other suggestion I left you in a comment in the RestContract PR, I let you decide i you agree or not, it's just a matter of consistency.
@tdonohue I think we still should consider this PR in the current release.
| * @param pageable pagination information | ||
| * @return all configuration properties as an array | ||
| */ | ||
| @SearchRestMethod(name = "exposed") |
There was a problem hiding this comment.
I've just left a comment on the RestContract. If you consider it, then this name should also be changed.
There was a problem hiding this comment.
actually exposed is more close to other existing search method name (top for communities) but I'm ok with both isExposed than exposed as long as the rest implementation and the rest contract match
| import org.junit.Test; | ||
|
|
||
| /** | ||
| * Integration Tests against the /api/config/properties/[property] endpoint | ||
| * Integration Tests against the /api/config/properties/[property] and | ||
| * /api/config/properties/search/exposed endpoints |
| import org.hamcrest.Matcher; | ||
| import org.hamcrest.Matchers; | ||
|
|
||
| public class ConfigurationMatcher { |
There was a problem hiding this comment.
this class misses java docs
EDIT: I know that we decide not to have mandatory java comments on tests, but it helps to understand.
|
@paulo-graca and @J4bbi : Just to clarify, I'd be OK with this getting into 7.6.1. However, it is lower priority to me as it's not used (yet) by the User Interface -- so it's the start to a performance improvement, but isn't used yet. I apologize but I haven't had any chance to test/review this as I'm still testing other bug fixes for the 7.6.1 release. However, if this gets +1 votes, we can still consider it for the 7.6.1 release (due next week). If it misses 7.6.1, then we should aim to get this added into both 7.6.2 (date uncertain) and 8.0 (due in April) |
|
I can confirm that this PR is working and exposing properties |
|
@paulo-graca , many thanks for your review and my apologies for the late reply I have had to prioritise other work. I just wanted to confirm that I will respond to this as soon as possible but not sure when exactly. (same applies to DSpace/RestContract#240) |
| * @param pageable pagination information | ||
| * @return all configuration properties as an array | ||
| */ | ||
| @SearchRestMethod(name = "exposed") |
There was a problem hiding this comment.
actually exposed is more close to other existing search method name (top for communities) but I'm ok with both isExposed than exposed as long as the rest implementation and the rest contract match
| ConfigurationMatcher.matchConfiguration("configuration.exposed.array.value") | ||
| ))) | ||
| .andExpect(status().isOk()); | ||
| } |
There was a problem hiding this comment.
can we add a check also using the admin token? the goal is to have a test that would protect us against future refactoring. In future we could allow admins to retrieve additional properties but this search method must exposed just the public values according to the rest contract
References
Add references/links to any related issues or PRs. These may include:
240Description
This PR adds a new REST endpoint that exposes all configuration variables that have been whitelisted (in
config/modules/rest.cfg). It includes an integration test.Instructions for Reviewers
Go to
server/api/config/properties/search/exposedand verify that all config is listed.I have not tested pagination for this endpoint as I am assuming it is handled by
pageable.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!
pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.