search

CZ-NIC / pyoidc

A complete OpenID Connect implementation in Python
Other
711 stars 258 forks source link

Client.request_and_return no longer works with NoneResponse #840

Open ubik2 opened 1 year ago

ubik2 commented 1 year ago

In 1.5.0, client.request_and_return doesn't work with NoneResponse anymore. As a workaround, you can pass None as the response, but that prevents more generic protocol registration from working. This broke in PR 817, where src/oic/oauth2/__init__.py lost the handling of the empty body on line 752 where it returned the status code instead.

ubik2 commented 1 year ago

Another approach to this is to change the Client.parse_response to handle the NoneResponse specially (acceptable for only_extras() to be true, and also ok for resp to evaluate to False). I dislike NoneResponse being special, but this also lets us still run the store_response and resp.verify.

schlenk commented 1 year ago

Can you elaborate for which usecase you need the old behaviour please?

I see the issue, yes, the return type changed if you do not care for the response message and just want the response HTTP code. The only internal users of that functionality got changed to just handle the case directly, because it ends up being just http_request() + some minimal http status code handling. Seems i missed the docstring of the request_and_return() method that spells out the special handling for no expected responses.

If there is some convincing usecase for this usage pattern, we might simply add a convenience method that does it, but currently i do not see a real benefit over just calling http_request() directly and simply reading the status code yourself.

ubik2 commented 1 year ago

For my use case, I have a user management endpoint to which I submit a POST to delete a user, and get back an empty response.

I can easily work around this by passing None as the response type, so it's not a real problem for me.

It's easy to imagine an expected response that could be empty, but could also have fields where you want to take advantage of the verification when all the fields are optional, or storing the response, but I don't actually need that functionality myself.

Feel free to close this, if you don't think it needs to be handled by the library.

schlenk commented 1 year ago

Ok, thank you.

I'll have to think about it a bit. The mix of Message and http status code as return type kind of makes the type annotations for that code really ugly and hard to reason about.

ubik2 commented 1 year ago

I did appreciate having the types cleaned up so much. Overall it's much nicer now, but I did want you to know about the compatibility issue with code that worked with 1.4. I'll use the workaround in my calling code for now.