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

Check for content types accepted by client #81

Open
uniqueg opened this issue Sep 17, 2020 · 16 comments
Open

Check for content types accepted by client #81

uniqueg opened this issue Sep 17, 2020 · 16 comments
Assignees
Labels
priority: medium Medium priority status: blocked Something prevents progress type: feature New feature or request workload: days Likely takes days to resolve
Milestone

Comments

@uniqueg
Copy link
Member

uniqueg commented Sep 17, 2020

API endpoints can serve different content types, e.g., application/json, text/plain etc. Clients can ask a service to respond with one or more specific accepted content types.

It would be nice if FOCA implemented a solution (e.g., a decorator) that compares the client's desired content type(s) against the ones that the service offers for the respective endpoint (as defined in the API specs). An appropriate error response should be returned if a client requests a content type that the endpoint cannot deliver. If the client does not explicitly ask for a specific content type (i.e., if the Accept: <content-type> header is missing), the default response should be given.

Other important points to think about:

@uniqueg uniqueg added priority: medium Medium priority type: feature New feature or request workload: days Likely takes days to resolve labels Sep 17, 2020
@vipulchhabra99
Copy link
Contributor

vipulchhabra99 commented Jan 31, 2021

Can I take up this issue next?

@anuragxxd
Copy link
Member

@vipulchhabra99 are you working on this? Or may I please take the issue.

@uniqueg
Copy link
Member Author

uniqueg commented Feb 17, 2021

Oh, I didn't see @vipulchhabra99's comment, sorry about that. From my side it's okay for either of you to take up that issue

@anuragxxd
Copy link
Member

I am working on it.:)

@vipulchhabra99
Copy link
Contributor

It's fine @uniqueg, I haven't started working on it.

@anuragxxd
Copy link
Member

@uniqueg In this we have to take user input for each specific route or the whole app. I think it's for the whole app.

@uniqueg
Copy link
Member Author

uniqueg commented Feb 19, 2021

I'm not sure I understand your question, but the goal here would be to create a generic solution for a relatively common problem - content negotiation between client and server. If the client requests the response to be of one or more specified content types (via the Accept: <content-type> header), the server SHOULD return the response in one of the requested content types, if it can, or else return an appropriate error response, probably 406 Not Acceptable.

To implement that, I had thought of defining a decorator that handles the content negotation and that could be added to the controllers of every specific route. But I can see that at least in most cases (I currently can't think of examples that would not want to have content negotation activated, but cannot rule out that there are not) it would be useful to have this for all routes. But I'm not sure how to implement that, given that FOCA doesn't have direct access to the routes. If you find a way (e.g., via Connexion) to do that globally, for the entire app, that would be awesome! In that case, I think we should go for such a solution and only implement a route-based solution if anybody actually asks for that.

As a starting point, please have a look at the issues linked in the issue description to find out what Connexion already has supported, what the limitations are etc. There may be other relevant issues in Connexion as well. Please post any findings, with relevant links (ideally to definite information such as code, specs etc) in this issue so that we can review them together before deciding on an implementation strategy.

@uniqueg uniqueg added this to the v1.0.0 milestone Feb 21, 2021
@uniqueg uniqueg assigned kushagra189 and unassigned anuragxxd Mar 15, 2021
@sarthakgupta072
Copy link
Member

In order to test if the connexion validation works with Accept: <content-type> header or not, we tried running DRS-filer. DRS-filer always sends an application/json response. In the request when Accept: application/zip was sent, the server was not throwing an error and giving a 200 - ok response.

Thus connexion validation is not working as expected.

This class is the place where response validation happens in connexion.

One solution is to create a similar class in foca and override it something like this

app = connexion.FlaskApp(__name__)
app.add_api('api.yaml', ..., validator_map=validator_map)

Ref: https://connexion.readthedocs.io/en/latest/response.html#custom-validator

This will ensure validation at an app-level rather than on an individual API endpoint level. Had some trouble understanding the code in the class, but if this is the right approach, we will dig deeper into this.

@uniqueg
Copy link
Member Author

uniqueg commented Apr 5, 2021

Thanks @sarthakgupta072. I think content negotiation/validation should always be performed, so it's fine to implement it at an app level.

But to see how (or rather where) to best implement it, I tnink a couple more tests would be useful to decide how to proceed:

For requests to a test endpoint, please set up those test cases:

  1. The Accept header contains at least one content type that is listed in the specs for that endpoint
  2. The Accept header does not contain any of the content types in the specs for that endpoint (desired behaviror: 406 Not Acceptable)
  3. The Accept header is not set (desired behaviror: 406 Not Acceptable)

The second set of tests are also for requests, but only if the request actually sends some data along (e.g., in a POST request):
4. The value of the Content-Type header is listed among the accepted content types in the specs for that endpoint
5. The value of the Content-Type header is not listed among the accepted content types in the specs for that endpoint (desired behavior: 415 Unsupported Media Type)
6. The Content-Type header is not set (desired behavior: 415 Unsupported Media Type)

For responses from a test endpoint, please set up those test cases (note that any errors here are the developer's fault, not the client's):

  1. The value of the Content-Type header is listed in the specs for that endpoint
  2. The value of the Content-Type header is not listed in the specs for that endpoint (should fail with a 500 Internal Server Error)
  3. The Content-Type header is not set (should fail with a 500 Internal Server Error)

Some more considerations:

  • For robustness and to check if Connexion has any special policies in place for application/json, try all test cases with content type application/json as well as at least one other content type.
  • To check if Connexion actually does some content type introspection or just passes on whatever passes on the content with whatever content type is declared, implement also a test case each where a (1) request or (2) response content type is declared (e.g., application/json) but content of another type (e.g., application/zip) is actually sent to / returned by the endpoint.

These tests also pretty much dictate how to implement content negotiation (and in which order):

  1. Check whether the client asks for a response content type that the server can actually provide
  2. Check whether any media sent along with the request, if any, is of a content type that the server can process
  3. Check whether the content type set for the response is compliant with the specs

If we have all of these tests in place, we know if and to what extent Connexion itself is handling any kind of content negotation / validation, and, based on that, we can decide whether we prefer to patch existing Connexion code or just write the FOCA code on top of Connexion. We can then also use those tests for test-driven development of the actual logic (which should actually be rather simple compared to coming up with all those tests). What's important eventually is that a list of content types accepted by the client and supported by the endpoint specs are passed to the endpoint so that the developer can decide which content type to deliver. The order of the content types in the Accept header should be preserved for that, as the usual behavior, I would assume, is that an endpoint returns content of the first accepted and supported type. However, as there may be other considerations in certain cases, it is important to retain other accepted and supported content types as well and thus give the developer a chance to implement more specific logic in the endpoint's controller.

Does this approach make sense to you?

@kushagra189
Copy link
Contributor

@uniqueg we actually tried only the basic test case. This approach definitely seems like worth trying. We'll get back to you with our further findings by the earliest.

@uniqueg uniqueg removed this from the v1.0.0 milestone May 2, 2021
@uniqueg
Copy link
Member Author

uniqueg commented May 2, 2021

@sarthakgupta072: any update on this? Just in case - I have removed the v1.0.0 milestone, as I think we should go forward with releasing that version even if that features doesn't make it in that release. Of course it's still a "nice-to-have" and there's still some time to get it in (writing the documentation and implementing/testing FOCA in at least one or two of the apps that don't currently fully use it will take a bit of time still)

@uniqueg uniqueg added this to the v1.0.0 milestone Aug 24, 2021
@uniqueg uniqueg moved this to Todo access control in FOCA release Dec 2, 2023
@uniqueg uniqueg moved this from Todo access control to Todo other in FOCA release Dec 2, 2023
@Rahuljagwani
Copy link
Contributor

Hi @uniqueg, I was looking into connexion documentation. I found out that in their latest version, they are providing support for various content types which we can use directly to solve this issue.(link)
We need to migrate to Connexion v3 for this.

Also @alohamora let me know when you are done with connexion migration to v3, I will implement this after that, or if you have not started working on migration, even I can start working on that. Do let me know what you prefer :)

@uniqueg
Copy link
Member Author

uniqueg commented Jan 22, 2024

Fantastic - thanks a lot @Rahuljagwani - great find!

Yes, @alohamora, please tell @Rahuljagwani if he should go ahead with Connexion v3 migration.

@uniqueg
Copy link
Member Author

uniqueg commented Jan 22, 2024

Also, @kushagra189 - please have a look here as well, as Connexion v3 migration also came up in our discussion. Might make sense to migrate first and then you could implement the type stuff from #144, #201 and #200? And @Rahuljagwani could take care of this one. What do you think?

@uniqueg
Copy link
Member Author

uniqueg commented Feb 5, 2024

@Rahuljagwani @kushagra189 says it's okay, so please go ahead

@uniqueg
Copy link
Member Author

uniqueg commented May 20, 2024

Currently blocked by #188

@uniqueg uniqueg added the status: blocked Something prevents progress label May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: medium Medium priority status: blocked Something prevents progress type: feature New feature or request workload: days Likely takes days to resolve
Projects
Status: Todo other
Development

No branches or pull requests

6 participants