Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return JSON for all API routes #2850

Merged
merged 1 commit into from Mar 24, 2020
Merged

Conversation

perlpunk
Copy link
Contributor

@perlpunk perlpunk commented Mar 18, 2020

This commit changes the response of the job_templates_scheduling endpoint,
if Accept: application/json was requested explicitly, it will return the
job template YAML encoded in a JSON string.

With an unspecific request header (or explicitly text/yaml) it will return
YAML as before.

OpenQA::UserAgent allows overriding the Accept header now.

The command line client can deal with different combinations of response type
and output options.

Issue: https://progress.opensuse.org/issues/64496

I did not add tests for the new script/client options. Any hints on how to test that appreciated.

lib/OpenQA/UserAgent.pm Outdated Show resolved Hide resolved
script/client Outdated Show resolved Hide resolved
@perlpunk perlpunk marked this pull request as ready for review March 18, 2020 16:43
@AdamWill
Copy link
Contributor

So I filed the issue but I'm not 100% sure this is the 'right' fix. It kinda depends on the intent of the API. Is it mainly intended just to deliver the data, or is the YAML representation of the data important? If the YAML representation is important, this may not be the right fix, it may be more appropriate to either intentionally fail if asked for JSON (with an error text telling the client the data's only available as YAML) or simply return the YAML representation wrapped in JSON (a single-item JSON dict with the value as the entire YAML representation as a string, or something...)

@perlpunk
Copy link
Contributor Author

simply return the YAML representation wrapped in JSON

I thought about that, but that would mean that the output has a different meaning depending on the content-type, and I don't like that.

IMHO, the result of an API call should always be the same, regardless if it is represented as YAML or JSON.

If you want to get the exact YAML template, you need to set the Accept header to text/yaml.

If we wrapped the YAML string in quotes for JSON, then we should also wrap it in a YAML string.

@codecov
Copy link

codecov bot commented Mar 18, 2020

Codecov Report

Merging #2850 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2850      +/-   ##
==========================================
+ Coverage   93.07%   93.08%   +<.01%     
==========================================
  Files         190      190              
  Lines       11890    11897       +7     
==========================================
+ Hits        11067    11074       +7     
  Misses        823      823
Impacted Files Coverage Δ
lib/OpenQA/WebAPI/Controller/API/V1/JobTemplate.pm 93.22% <100%> (+0.23%) ⬆️
lib/OpenQA/UserAgent.pm 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c50fd7c...c9bf515. Read the comment docs.

@kalikiana
Copy link
Member

simply return the YAML representation wrapped in JSON

I thought about that, but that would mean that the output has a different meaning depending on the content-type, and I don't like that.

IMHO, the result of an API call should always be the same, regardless if it is represented as YAML or JSON.

If you want to get the exact YAML template, you need to set the Accept header to text/yaml.

If we wrapped the YAML string in quotes for JSON, then we should also wrap it in a YAML string.

With this change, from what I can see, you get the following:

  • Request text/yaml to get the verbatim YAML as before.
  • By default the result is the serialized JSON version of the YAML.
  • Request JSON to get the same as the default.

So this is a breaking change because the default is not the same. And the meaning of the new default is very different - you can't post the JSON without information loss. The client changes also reflect that - you can choose from two formats, one of which we don't support.

If you want to get the exact YAML template, you need to set the Accept header to text/yaml.

The API gives you YAML by default right now. So that would unfortunately break things.

If your goal is consistency with other routes which already return JSON the result should be JSON wrapping the verbatim YAML document, in both the single and multi case, if the request asks for JSON. And we consider the other one deprecated and stop recommending its use.

@perlpunk
Copy link
Contributor Author

perlpunk commented Mar 19, 2020

Ok, @kalikiana and I talked about this, and decided that we'll go with his suggestion to deprecate the text/yaml version of the endpoint.

For backwards compatibility we still have to support it, so the new behaviour depending on the Accept header would be:

  • text/yaml -> Return the job template YAML directly, as text/yaml (Deprecated)
  • */* -> same as for text/yaml (Default might change after a certain amount of time)
  • application/json -> return a JSON string which contains the encoded YAML template

That means that all API clients should always request JSON from now on to get consistent results.

There is one possibly breaking change necessary:

OpenQA::Client always sends application/json if no Accept header is set, and we shouldn't change that. It's a client for a REST API, and that makes sense.
But script/client therefor requests the job_template_scheduling route with application/json, and the backend will now respond with JSON, so that's not backwards compatible.
So we would need to change script/client to send Accept: */* by default, and one can influence the accept header with the new --accept option.

Since there is a new client in development, we probably are ok to do this change for the old one, and the new client will implement and document this from the beginning.

@perlpunk
Copy link
Contributor Author

@AdamWill

the YAML representation wrapped in JSON (a single-item JSON dict with the value as the entire YAML representation as a string, or something...)

Would it be ok in your opinion to just return a string with the encoded template, instead of a list or an object?
I think that has been added to be valid JSON for a while now.

@AdamWill
Copy link
Contributor

if a JSON validator says it's good, sounds fine to me :)

t/api/08-jobtemplates.t Show resolved Hide resolved
t/api/08-jobtemplates.t Outdated Show resolved Hide resolved
@okurz
Copy link
Member

okurz commented Mar 21, 2020

please squash wip commits and rebase

This commit changes the response of the job_templates_scheduling endpoint,
if Accept: application/json was requested explicitly, it will return the
job template YAML encoded in a JSON string.

With an unspecific request header (or explicitly text/yaml) it will return
YAML as before.

OpenQA::UserAgent allows overriding the Accept header now.

The command line client can deal with different combinations of response type
and output options.

Issue: https://progress.opensuse.org/issues/64496
@perlpunk perlpunk changed the title Support Accept header in API Return JSON for all API routes Mar 24, 2020
@perlpunk
Copy link
Contributor Author

squashed and rebased

@okurz okurz merged commit 54cfbef into os-autoinst:master Mar 24, 2020
@perlpunk perlpunk deleted the api-content-type branch March 24, 2020 17:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants