Conversation
tdonohue
added
bug
1 APPROVAL
|
@abollini : Adding you as a reviewer of this REST Contract update, since you had recommended this approach in a Dev Mtg. |
paulo-graca
left a comment
paulo-graca
left a comment
There was a problem hiding this comment.
Thank you @J4bbi for this. I've just left you two comments, please feel free to consider them. To me, this PR should be still considered in this Release.
configuration.md
Outdated
| * 404 Not found - if the property doesn't exist or isn't configured to be exposed | ||
|
|
||
| ## Search methods | ||
| ### top |
There was a problem hiding this comment.
Not sure why the Top here
configuration.md
Outdated
|
|
||
| ## Search methods | ||
| ### top | ||
| **/api/config/properties/search/exposed** |
There was a problem hiding this comment.
I've seen this in other endpoints, to be consistent it could be - isExposed, like:
/api/config/properties/search/isExposed
I let to you to decide, both approaches are ok with me.
There was a problem hiding this comment.
Changed to isExposed as suggested
J4bbi
force-pushed
the
feature/exposed_config_endpoint
branch
from
94f9b1b to
f47335d
Compare
| ``` | ||
|
|
||
| * 200 OK - if the operation succeeded | ||
| * 404 Not found - if no property is configured to be exposed No newline at end of file |
There was a problem hiding this comment.
this should be an empty list but still 200.
The endpoint must support pagination parameters according to our general rules https://github.com/DSpace/RestContract?tab=readme-ov-file#pagination
| For example: | ||
|
|
||
| ```json | ||
| [{ |
There was a problem hiding this comment.
we need to follow the usual HAL format for resource collections having an embedded with the pagination information
| * 404 Not found - if the property doesn't exist or isn't configured to be exposed | ||
|
|
||
| ## Search methods | ||
| **/api/config/properties/search/isExposed** |
There was a problem hiding this comment.
I'm not completely sure about the path isExposed but I'm not against it. Just adding this comment to share my thought. It was looking strange to me at a first look but I haven't found any direction in our naming convention https://github.com/DSpace/RestContract?tab=readme-ov-file#on-the-naming-of-endpoints
I found that we have a lot of diversity in the way that search methods have been named so far
- some start with findBy...
- other start with byXXX
- allmost all seems to use the CamelCase convention (that we decide to avoid for the resource collection endpoints)
4 participants
Fixes DSpace/DSpace#9056 as discussed in DevMtgs 14th and 21st September 2023.
This PR adds an endpoint to the configuration endpoint to more efficiently share configuration with the frontend.
Current approach
Add the /api/config/properties/search/exposed endpoint, as per the top communities search endpoint.
Pros
API endpoint url is clear.
Cons
Endpoint url is not consistent with the single case, /api/config/properties/<:property>.
Alternative approach
I am a little undecided about whether I feel /api/config/properties would be a better solution and just implementing the missing findAll method.
Pros
There is consistency in the naming of the endpoints. /api/config/properties/<:property> for a single configuration and /api/config/properties/ for every single one.
Cons
It might not be apparent that this endpoint is limited to whitelisted, exposed configuration.