Inconsistencies in the API

[hmpf]

While testing the automated dev-setup for the frontend I wound up taking a look at the swagger-interface (runs on localhost:443) to the backend API. I have some (pedantic) comments.

In most cases you have two endpoints to work with one type of data. For instance /devices to list all, and /device/<something>. But not for groups, there you only have /groups.

Furthermore, the descriptions of several of the endpoints seems copy-pasted. Compare

• /job API for handling jobs
• /jobs API for handling jobs
• /joblock API for handling jobs

Third, why /joblocks instead of /job_locks when you have /device_<something>? I read it as "job blocks" at first.

Fourth, there is no example response body, which is very useful for somebody trying to figure out the API without having possibly dangerous access. I assume the swagger is auto-generated and that it is a hassle to extend it to do the right thing but it is very much worth it, in my experience.

Fifth, how are you planning to do versioning of the API? I assure you, you'll want to have a way to version the API.

[katsel]

I just played with the swagger interface a bit. Swagger supports running actual queries against the API by storing a JWT token (klick: Authorize > paste a valid token > klick: Authorize).

Seems like there is an issue with autentication also.

I am trying an arbitrary endpoint, let's say /devices.

Swagger generates:

Curl

curl -X GET "https://localhost/api/v1.0/devices" -H  "accept: application/json" -H  "Authorization: <JWT>"

Request URL

https://localhost/api/v1.0/devices

This results in the following error: 401 unauthorized

Response body

{
  "data": "Invalid header, JWT token missing? Bad Authorization header. Expected value 'Bearer <JWT>'",
  "status": "error"
}

The point here is, that swagger sends Authorization as a keyword in the HTTP header, where the cnaas-nms API expects the keyword Bearer. So either, swagger could be configured to use Bearer as keyword (if that's possible?), or the API could accept Authorization as well.

The swagger interface is a nice human-friendly way to get an overview of the API and play with it, it is interactive documentation. So I would definitely support looking into these issues, and also putting a reference to swagger into the written docs.

[katsel]

To sum up, I tried to generate some actionable tasks from @hmpf's accurate report.

Documentation/swagger:

• [ ] improve description on /job, /jobs and /joblock
• [ ] add example response bodies to swagger doc
• [ ] enable JWT auth in swagger; see my comment above

For API consistency:

• [ ] add /group/ID endpoint
• [ ] rename /joblock to /job_lock (or keep the former for API backwards compatibility)

Regarding the last bit you mentioned @hmpf, cnaas-nms does API versioning via the URL: /api/v1.0/…. Is there anything more that needs to be considered here?

[hmpf]

Regarding the last bit you mentioned @hmpf, cnaas-nms does API versioning via the URL: /api/v1.0/….

Yeah I'm not used to the base url being split out, found it today.

Is there anything more that needs to be considered here?

I only did a cursory skim so there's no doubt other things.

[hmpf]

I just played with the swagger interface a bit. Swagger supports running actual queries against the API by storing a JWT token (klick: Authorize > paste a valid token > klick: Authorize).

Did you try pasting in "Bearer: <jwt token>" and not just the token?

[indy-independence]

We should also differentiate PUT and PATCH methods, today only PUT is used but functions as PATCH should Maybe PUT to reflesh repos should be a POST also? And of course the discussion about trailing s in object names, probably easier if everything ends with s and maybe accepts a list of items to PUT,POST,PATCH,DELETE etc? I think all things like this are good to save for a /api/v2/ some day