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

Client.request_and_return no longer works with NoneResponse #840

Open
ubik2 opened this issue Dec 17, 2022 · 5 comments
Open

Client.request_and_return no longer works with NoneResponse #840

ubik2 opened this issue Dec 17, 2022 · 5 comments

Comments

@ubik2
Copy link

ubik2 commented Dec 17, 2022

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
Copy link
Author

ubik2 commented Dec 17, 2022

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
Copy link
Collaborator

schlenk commented Dec 17, 2022

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
Copy link
Author

ubik2 commented Dec 18, 2022

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
Copy link
Collaborator

schlenk commented Dec 18, 2022

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
Copy link
Author

ubik2 commented Dec 21, 2022

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.

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

No branches or pull requests

2 participants