Implement login endpoint #46 #47
Conversation
| */ | ||
| @POST | ||
| @Path("/session") | ||
| public String login(@QueryParam("facilityName") String facilityName, @FormParam("json") String jsonString) throws BadRequestException { |
There was a problem hiding this comment.
I feel like this endpoint is not simple/user-friendly enough for a DLS user, mainly for 2 reasons.
- They should not have to specify that the facilityName is "DLS"
- We should not pass on the horrible task of creating the ICAT json string required for the credentials.
Ideally a DLS user just wants to submit their username and password and get a session ID back.
So probably what we need is 2 mandatory parameters for username and password, and the facility name as an optional parameter that if not provided is either picked up from a new property (facility.default?) or just assumed to be the first facility in the facility.list property. The plugin could also be an optional parameter that if not specified is picked up from a new property (login.plugin.default?)
There was a problem hiding this comment.
I'm happy to simplify this along those lines. In the first instance was trying to keep things consistent with how ICAT server accepts login requests (the JSON string) and how all the other download-api endpoints require the facilityName as a PathParam, QueryParam or FormParam.
Have replaced the json string with username, password, plugin. facility_name and plugin are optional. The latter just uses a new default value in the properties. The former will default to the only facility set in cases where there is only one facility set. I assumed this would be the case for Diamond (i.e. that API is only running for them and not multiple facilities) but if not I can relax this to just always use the first if facility_name is null. Note that this removed explicit null checks that existed before, but didn't notice any unexpected behaviour. It will also allow other endpoints created as part of this feature to benefit from not needing to specify the Facility.
|
Implemented explicit Local and CI test failures were being caused by (I suspect) Downloads generated from the new tests from #43 and/or #44 not being cleared up after the tests finished, which then caused tests from #45 to fail as these other Downloads were being processed instead. Added similar clean up in a |
| testDownload.setTransport("http"); | ||
| downloadRepository.save(testDownload); | ||
| downloadId = testDownload.getId(); | ||
| System.out.println("userstatus:" + downloadId); |
There was a problem hiding this comment.
Is this a typo/copy paste? (It prints "userstatus:" but then adds the download ID)
There was a problem hiding this comment.
The string is intentional (UserResourceTest + testSetDownloadStatus -> "userstatus") but it's a debug statement left in from when I was trying to work out which Downloads were being generated by one test and then triggering failures in another - will remove it.
Should be independent of other changes
Closes #46