3591: list facet API fix #3630
3591: list facet API fix #3630
Conversation
| public Response listFacets( @PathParam("identifier") String dvIdtf ) { | ||
| return allowCors(response( req -> ok( | ||
| execCommand(new ListFacetsCommand(req, findDataverseOrDie(dvIdtf)) ) |
There was a problem hiding this comment.
The removal of the ListFacetsCommand makes me wonder if the permissions at https://github.com/IQSS/dataverse/blob/v4.6/src/main/java/edu/harvard/iq/dataverse/engine/command/impl/ListFacetsCommand.java#L37 should be included. It looks like they checked if the dataverse is published or not.
There was a problem hiding this comment.
Good point - I'll switch it to ListFacetsCommand (assuming that's not the source of the problem).
| * (not publicly documented) API endpoint for assigning facets to a dataverse. | ||
| * `curl -X POST -H "X-Dataverse-key: $ADMIN_KEY" http://localhost:8088/api/dataverses/$dv/facets --upload-file foo.json`; where foo.json contains a list of datasetField names, | ||
| * works as expected (judging by the UI). | ||
| * This triggers a 500 when '-d @foo.json' is used. |
There was a problem hiding this comment.
It's probably worth spending a few minutes looking into this 500 error.
There was a problem hiding this comment.
This looks to me like the usual case where some API's expect form data, and some expect upload-file within the form data - it may make more sense note this for API consistency and defer until there's a systematic check of what endpoints should be getting.
| @@ -342,15 +343,35 @@ public Response setMetadataRoot( @PathParam("identifier")String dvIdtf, String b | |||
|
|
|||
| @GET | |||
| @Path("{identifier}/facets/") | |||
| /** | |||
| * return list of facets for the dataverse with alias `dvIdtf` | |||
| */ | |||
| public Response listFacets( @PathParam("identifier") String dvIdtf ) { | |||
There was a problem hiding this comment.
It would be nice to add at least one test to https://github.com/IQSS/dataverse/blob/v4.6/src/test/java/edu/harvard/iq/dataverse/api/DataversesIT.java to make sure this API doesn't stop working again. (I assume it worked once upon a time.)
There was a problem hiding this comment.
I agree that it'd be good to have (I assume it worked once upon a time as well); but the logic of setting up those tests wasn't too clear to me on initial reading (I'm also not sure if those are RestAssured tests that I'm not setup to run locally or not).
There was a problem hiding this comment.
You're off the hook on REST Assured tests until I document how to set up a dev environment to run them, which I'd like to to as part of #3431. The short of it is that you have to run setup-all.sh --insecure like in https://github.com/IQSS/dataverse/blob/v4.6/scripts/deploy/phoenix.dataverse.org/post#L3
|
switched back to using command pattern; and added some javadoc for the command (4190f6b is the latest). |
|
Ready for QA |
RFI Checklist
Before submitting the pull request, fill out sections (1.) Related Issues and (2.) Pull Request Checklist.
1. Related Issues
2. Pull Request Checklist
3. Review Checklist
After the pull request has been submitted, fill out this section.