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

Allow HTTP content negotiation via Accept: header #217

Closed
jerstlouis opened this issue Jun 10, 2021 · 13 comments · Fixed by #272
Closed

Allow HTTP content negotiation via Accept: header #217

jerstlouis opened this issue Jun 10, 2021 · 13 comments · Fixed by #272
Labels
1.1 Next development iteration

Comments

@jerstlouis
Copy link
Member

jerstlouis commented Jun 10, 2021

Perhaps in a way similar to the solution proposed for #215, I propose that we allow content negotiation of the response via Accept: header for raw responses.

For a single output, this would allow specifying response format without having to specify a big outputs block and untangle the format from the execution request, faclitating re-usability.

For multiple outputs, it could be a way to select a particular multipart encoding.

(For document response, the Accept: format would refer to the document itself, e.g. XML instead of JSON encoding)

This would make the API a lot more in line with the OGC API web guidelines.

Similar to #215, the header or the request would override the other.

My preference actually would be for the header to override the body, since it is the request document that would likely be re-used and the header would provide a mechanism to override the request without modifying the request document itself.

@jerstlouis jerstlouis changed the title AllowHTTP content negotiation via Accept: header Allow HTTP content negotiation via Accept: header Jun 10, 2021
@fmigneault
Copy link
Contributor

To resolve the single vs multiple output issue and returning their content directly in different formats, wouldn't it be better to simply define an endpoint as follows:
GET /jobs/{jobID}/results/{outputID}

I don't like how /jobs/{jobID}/results could sometimes answer with a mapping of outputID:URL and other times with contents of that output itself according to Accept. How would we resolve the conflict with the response content-type if that output result happens to also be application/json?

@jerstlouis
Copy link
Member Author

jerstlouis commented Jun 11, 2021

@fmigneault A few things:

/jobs/{jobID}/results sometimes answers with a mapping of outputID:URL and other times with contents of that output itself

This is kind of a separate issue. Even without this Accept: proposal, this is already the case. I really dislike that myself.

How would we resolve the conflict with the response content-type if that output result happens to also be application/json?

There is no conflict, because what is returned is determined by whether the response/return is set to document/minimal or raw/representation, not the Accept: header. The Accept: header only defines how to return it.

wouldn't it be better to simply define an endpoint as follows:
GET /jobs/{jobID}/results/{outputID}

I actually would very much like something like this... And you got me thinking a lot...
I am not sure the SWG is still open to discuss this, but after a lot of thinking and long spurrious extended code sprint ad-hoc exchange with @gfenoy, here is how we think it really should all work:

  • Drop the document/raw response distinction altogether from execution requests
  • Drop the reference/value transmission distinction altogether from execution requests
  • /jobs/{jobID}/results always includes all outputs (specified in the request, or all if omitting outputs)
  • /jobs/{jobID}/results/{resultId} is always the raw individual output, by value (resultId = outputId)
  • Content negotiation using Accept: header is supported at:
    • /jobs/{jobId}/results,
    • /jobs/{jobId}/results/{resultId},
    • for synchronous execution requests (whether single or multiple outputs).
  • The server SHALL support the request's output media type at /jobs/{jobId}/results/{resultId}, but MAY support additional ones (e.g. transcoding to a different supported output type after execution)
  • The server SHALL support all advertised media types for single output sync execution, where the Accept: media type would override the request/default
  • The server SHALL support application/json at /results and for multi-outputs sync execution, but MAY support additional ones (e.g. zip or multipart/*)
  • Requesting /jobs/{jobID}/results as JSON returns a document containing the map of outputId to values (or references)
  • For /jobs/{jobId}/results requested as JSON, if no return Prefer: header is used, the server SHOULD return large outputs as a reference
  • For /jobs/{jobId}/results requested as JSON, if return=minimal is used, the server SHOULD return all outputs which are not boolean, number or simple string (i.e. no contentMediaType) as a reference
  • For /jobs/{jobId}/results requested as JSON, if return=representation is used, the server SHOULD return all outputs inline (self-contained document).
  • Any inlined binary outputs still have to be base64-encoded
  • For a synchronous execution returning multiple outputs, the same rules as for /jobs/{jobId}/results apply
  • For a synchronous execution returning a single output, the same rules as for /jobs/{jobId}/results/{resultId} apply
  • Job status response MAY include a link to "all results" (/jobs/{jobId}/results) and/or to individual outputs at /jobs/{jobID}/results/{resultId} (but those resources would always required and available at those fixed paths anyways) -- Note that redirection could be used to point to a reource hosted somewhere else.

This would properly address #204, obsolete #215, and I think in general is a huge improvement in following the OGC API Web Guildelines Principle #2, #9 & #10. The table of possibilities is greatly reduced, without losing any real flexibility. It makes things much easier to implement on the server, and the specifications a lot easier to understand.

@pvretano
Copy link
Contributor

You guys realize that this means that any hope of getting the specification out in the next few months is out the door ... right? If we apply these changes it will mean touching almost every file in the specification and then we need to give implementors time to implement the changes and verify the specification. I'm not saying I don't like this (my server actually implements a /jobs/{jobId}/results/{resultId} endpoint) but the implications are enormous!

@jerstlouis
Copy link
Member Author

jerstlouis commented Jun 11, 2021

@pvretano Let's say we have a PR ready for Monday, 2 server implementations and a 1 client implementation, we can still discuss on Monday, present the document at the Members Meeting on Tuesday, and have an electronic vote ending 3 weeks from then?
I believe this would give enough time for everyone in the workshop this week at least to update their implementations and validate the changes.

Really, the changes are about avoiding corner/impractical cases which make things harder to implement, so I feel it will have minimum impact on implementations (other than requiring the .../results/{resultId} end-point for jobs), and will actually make it much easier for implementations to conform.

@pvretano
Copy link
Contributor

@jerstlouis as I said I am not opposed to the proposal BUT if we move ahead with it then I would make a motion to withdraw the adoption vote until further notice. We cannot keep drifting in this manner without ever reaching an endpoint and we cannot keep making such big changes in a rushed manner. This change is big and would require a lot of documentation changes, a lot of review of those changes, implementations would need to updated to test the changes and to verify the standard. For an adoption vote, three implementations would be required at a minumum.

@jerstlouis
Copy link
Member Author

jerstlouis commented Jun 11, 2021

For an adoption vote, three implementations would be required at a minumum.

How complete/well tested those 3 implementations have to be though, especially considering we don't yet have an executable test suite ready? Can the standard be fully approved before the ETS is ready and 3 implementations pass it?

We made huge progress this week I think, but I don't think any single imlementation fully conforms to the current specifications, and I feel the proposal actually moves that goal of having 3 fully conforming implementations closer as it reduces the number of execution combinations to support (without losing flexibility from a client's perspective).

@pvretano
Copy link
Contributor

@jerstlouis the P&P says full track standards need "evidence" of implementation. What we just did with the workshop is evidence of implementation so we would probably need to run another workshop once the specification has been updated and at least 3 implementations are ready for testing.

@jerstlouis
Copy link
Member Author

jerstlouis commented Jun 11, 2021

@pvretano I don't think these changes affect anything that was demonstrated this week in TIEs between implementations from different participants.

Honestly, I personally feel like we should do more of this with more TIEs before the standard is fully approved regardless of any changes (the July code sprint being the perfect opportunity).

@sptillma
Copy link
Contributor

@jerstlouis I am with Peter on this. I use to have a boss that said you have to draw a line in the sand because developers will never finish coding - they always think they can make it better. We are not going to continue all of this last minute proposals. You are welcome to create the issue (which you have) and we can see if there is a supported motion to discuss it. But until a motion is put forth to discuss it (in a quorum meeting), it cannot even be discussed on the formal board like this. I would even say it borders on breaking the Code of Conduct within the OGC. On Monday, nothing prevents you from making a motion to see if others support you. But at this point I don't support opening the door for discussion and I will force a vote. This last minute stuff has to stop at some point.

@jerstlouis
Copy link
Member Author

@sptillma I apologize if I am breaking the Code of Conduct.
I understood GitHub repository issues as the place to have the technical exchanges to discuss what could be, the results of which can inform the SWG about whether it should be.
Sorry if the discussion moved away from the technical focus.

@sptillma
Copy link
Contributor

@jerstlouis It is the place to have technical discussions, but not in regards to the current draft based on the motion to freeze functionality was passed.

@fmigneault
Copy link
Contributor

I really like overall the description in #217 (comment)

Only issue I can think of is that the server is still allowed to ignore Prefer, whether return=minimal|representation is provided or not. It would be important to properly define what is considered "large" if this is the default:

if no return Prefer: header is used, the server SHOULD return large outputs as a reference

Otherwise, anything else looks fine to me.
All that being said, I agree that some milestone should probably be reached before introduction all those changes.

@jerstlouis
Copy link
Member Author

jerstlouis commented Jun 11, 2021

@fmigneault We will meet on Monday morning 8:00 AM Eastern and hopefully discuss whether we can discuss this proposal. It would be great if you can join us.

It would be important to properly define what is considered "large" if this is the default:

The recommendation could include a certain threshold, say 10kb.

Like anything with the Prefer: header these would have to be recommendations.
On the plus side, I think it makes it easier for server implementations (making it possible to avoid wasted bandwidth with base64 encoding, or allow for sync execution that may immediately discard results). For clients, it just means having to support base64-decoding and following links, but only for multi-outputs sync requests or when using /results instead of /results/{resultid} (i.e. there is still always a way out in not having to support either).

@bpross-52n bpross-52n added the 1.1 Next development iteration label Jun 18, 2021
pvretano added a commit to pvretano/ogcapi-processes that referenced this issue Nov 28, 2021
@pvretano pvretano linked a pull request Nov 28, 2021 that will close this issue
pvretano added a commit that referenced this issue Jan 23, 2022
Initial check-in to implemented changes for Issue #217.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1.1 Next development iteration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants