#24 Recommendation: GET instead of POST for /api/v1.0/decision
Closed: Rejected 3 years ago Opened 3 years ago by bowlofeggs.

The REST API currently has users post query parameters to /api/v1.0/decision to get the results. This is not very REST-ful because the query parameters aren't becoming an object in the database; thus, we are not truly POSTing an object. What we are really doing is GETing a response based on query parameters.

This has already been discussed in this PR[1]. The query parameters will most likely be changed and we may need to store the results in the database.

[1] https://pagure.io/greenwave/pull-request/2

More importantly, the inputs for making a decision are potentially long and difficult to represent in a query string (think, a list of 20 build NVRs etc).

We figured taking the inputs as JSON in the request body of a POST will make life easier because it's not subject to query string length restriction, and we can express arbitrary structure instead of just key=value string pairs.

Hello @dcallagh,

I can't speak towards other use cases than Bodhi (and I know there are others), but Bodhi will be asking about builds one at a time as far as I know so there will only be one NVR in each call from Bodhi (unless I am mistaken about @pingou's plans ☺).

I thought Bodhi will only ask decision for each update so there would be a list of NVRs in each call. The decision should be made on each update, not on each build.

@mjia The design by @pingou so far has been to track CI status by build. The update's CI status is a composite of its component build CI statuses. @pingou, what do you think?

Since build is what is being tested, I think it makes sense to track status per build, but it also makes sense to be able to ask for an entire update in one go (ie a (potentially long) list of builds) as a single build may fail where a list of builds would pass (if the update includes two builds depending on each other).

So I kinda wonder if it makes sense to have say:

  • GET: get status for a single build
  • POST: get status for multiple builds

Yeah we could certainly add (back) a GET endpoint for the simple case where the "subject" of the decision is just one single build NVR, since that can be easily represented in the query string. If the client is passing a long list of multiple NVRs, or some other more complex "subject" (we don't have any yet but we might in future) then the client can POST it as JSON instead.

+1, that makes much more sense.

I can have a crack at this.

Metadata Update from @dcallagh:
- Issue assigned to dcallagh

3 years ago

I just realised, in light of #45 this might not be possible anymore...

Now that we are expecting the subject to be a list of dicts, where each element will contain multiple keys needed to identify the item under test (item and type, or possibly commit_hash and fedora_release if that is what we end up with for CentOS CI results)... I don't think there is any nice way we can represent thing in query string parameters for a GET request.

The best I can come up with would be to invent some key-value syntax like colon-and-comma-separated, for example:


but then we run into problems like what if one of the values needs to contain a comma or colon, and so forth.

I think we are better off sticking to the JSON representation in a POST request, and not worrying about how to cram the subject into a query string.

Stupid idea, but would having a ?data=<json serialized=""> work?

It's a little ugly but not really meant to be used by human no?

Oh, you also have to watch out for the 414 error (which I definitely hit with a super long query in PDC).

:+1: from me for sticking with POST even though the semantics are weird.

Metadata Update from @mjia:
- Issue status updated to: Closed (was: Open)

3 years ago

Metadata Update from @dcallagh:
- Issue close_status updated to: Rejected

3 years ago

Cool. Thanks for considering it, I'm convinced that the long URL problem makes the POST OK.

Login to comment on this ticket.