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

Sync / Multiple output response #415

Closed
jerstlouis opened this issue May 28, 2024 · 4 comments · Fixed by #434
Closed

Sync / Multiple output response #415

jerstlouis opened this issue May 28, 2024 · 4 comments · Fixed by #434
Assignees

Comments

@jerstlouis
Copy link
Member

jerstlouis commented May 28, 2024

Requirement 31 could be adjusted to make it more obvious that alternative content types can be supported in this case. Right now it says:

B The media type of the response SHALL be application/json
C The content of response SHALL conform to the results.yaml schema.

This is already somewhat conflicting with Permission 7 that says:

Servers MAY support other response formats or encodings (e.g. ZIP or multipart/*) that do not conform to results.yaml.

I would suggest to not make it mandatory to support results.yaml, so as to allow sync-only servers to execute, return, forget as discussed in #412, without being forced to base64-encode large binary outputs, saying instead:

B When the negotiated media type is application/json, the content of response SHALL conform to the results.yaml schema.

and make it a recommendation instead to support application/json / results.yaml content negotiation. Alternatively, if we want to keep the requirement to support application/json, the requirement could be rephrased as such to remove the apparent conflict with Permission 7:

B The implementation SHALL support an application/json response
C When the negotiated media type is application/json, the content of response SHALL conform to the results.yaml schema.

In terms of the alternative encodings, we could also include GeoPackage (application/geopackage+sqlite3) as another excellent example of a media type supporting multiple outputs.

@fmigneault
Copy link
Contributor

This used to be distinguished in 1.0 with the response=raw|document. Document was for the results.yaml, while raw allowed the multipart/related. Since the transition to Prefer header, this control became much more ambiguous in the specification, which I believe is at the root of this issue and #412.

Fundamentally, I think multipart/related makes a lot of sense, but there must be an explicit way (as in, the spec indicates the method to do it clearly) to obtain the response corresponding to what used to be response=raw|document. Using a Accept: multipart/related vs Accept: application/json could be one way.

That being said, I don't think the requirement should be loosened. The application/json is used for all other endpoints. It makes more sense to support it. It is also easier to implement base64-encode than multipart. Base64 is often used when returning 1-output value that are not clear text anyway. Inserting it within a JSON structure is not much more complicated. On the other hand, multipart needs to handle the nested Content-Type and the boundary characters correctly. At the very least, there should be 1 common way to interact with all OGC API - Processes implementations, and application/json is the best candidate since it is used everywhere else.

So, my vote goes to the rephrasing.

@jerstlouis
Copy link
Member Author

Using a Accept: multipart/related vs Accept: application/json could be one way.

That is exactly the way with the new version. I'm not a fan of multipart/related at all. It was a nightmare to implement in our server as the Executable Test Suite for 1.0 requires it. I would rather implement the zip and/or GeoPackage alternatives before implementing it.

But being forced to base64 encode large outputs is not fun either, and neither is having so support persistent storage ;)

But you're probably right that base-64, despite its ~35% overehead, is the simplest thing to implement (compared to multipart, zip or GeoPackage) for sync servers providing processes producing multiple outputs and wanting to avoid persistent storage, so I'm happy with the rephrasing.

@fmigneault
Copy link
Contributor

That is exactly the way with the new version.

Sorry if I was not clear. I, personally, am aware of this, but I feel it might not be so obvious to readers. Before, an explicit parameter (response) was mentioned. I think the specification could avoid potential confusion just by listing those explicitly for that specific case given how critical its interpretation can be.

@pvretano
Copy link
Contributor

pvretano commented Jun 9, 2024

The requirement URI is /req/core/process-execute-sync-many-json so its implied that the client and the server have negotiated a JSON response. So, I'll add an additional condition to make that explicit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

3 participants