[FIX] webservice: deprecate implicit content_only=True default - #144
HasanAlsaafen wants to merge 1 commit into
Conversation
- Add explicit deprecation warning when content_only is not set - Fix inconsistency in OAuth2 adapters that ignored content_only entirely - Add tests covering default behavior, explicit True, and explicit False Fixes OCA#142
|
Hi @etobella, |
There was a problem hiding this comment.
Hey @HasanAlsaafen, thanks for this PR related to #142.
I originally opened the issue to gather external input on how to handle this situation, if we decide to address it. The challenge with making changes like the ones introduced in this commit is that they may disrupt the expected behavior of existing modules or code that depend on the current default, potentially breaking running instances.
My bad, I misread the code and the default stays request.content. That's fine but we do not solve the main issue concerning my issue #142 🥲 . The main issue being returning something else than a Response when the original documentation states :
All of Requests’ functionality can be accessed by these 7 methods. They all return an instance of the Response object.
https://requests.readthedocs.io/en/latest/api/#main-interface
|
Hey @Ricardoalso Before deciding how to proceed, it might help to first get a clearer picture of the actual impact: are there known modules in the OCA ecosystem that rely on the current default behavior, or would this mainly affect external/custom code outside OCA? If we can narrow down who is likely affected, it'll be easier to choose between an opt-in flag, a deprecation cycle, or a straight breaking change with a migration note. Let me know if there's a preferred way to check this (e.g. searching OCA repos for usages), and I can help gather that data before we settle on an approach. |
|
@HasanAlsaafen sounds like a good approach. Can you pls rewrite the commit msg as |
|
@Hasan2005-CS thanks for your contrib. In the end I had to choose a slightly different approach because of the core split. |
`_request()` defaulted `content_only` to `True`, silently returning raw bytes instead of the `requests.Response` object callers expect. The OAuth2 backend adapter ignored the flag entirely, always returning bytes (fixes OCA#142). OCA#144 fixes the OAuth2 inconsistency but keeps `content_only` as a kwarg (default `None`, warns, still defaults to `True`) - perpetuating the actual problem: the return type shouldn't be a per-call switch, especially when raw content is just `.response.content` away. Here, `_request()` always returns the full response; the kwarg is dropped (a stray one is popped and warned about, not left to crash inside `requests.request()`). Backward compat is handled once, at the database level: the `18.0.2.0.1` upgrade script sets a `webservice.request_content_only` system parameter on existing databases only, preserving old behavior for calling code that relied on the implicit default. Delete the parameter to switch to the new behavior. The compat switch lives in `webservice`, not `webservice_core`: the latter is new and unreleased, so its `_request()` stays simple with no compat baggage.
`_request()` defaulted `content_only` to `True`, silently returning raw bytes instead of the `requests.Response` object callers expect. The OAuth2 backend adapter ignored the flag entirely, always returning bytes (fixes OCA#142). OCA#144 fixes the OAuth2 inconsistency but keeps `content_only` as a kwarg (default `None`, warns, still defaults to `True`) - perpetuating the actual problem: the return type shouldn't be a per-call switch, especially when raw content is just `.response.content` away. Here, `_request()` always returns the full response; the kwarg is dropped (a stray one is popped and warned about, not left to crash inside `requests.request()`). Backward compat is handled once, at the database level: the `18.0.2.0.1` upgrade script sets a `webservice.request_content_only` system parameter on existing databases only, preserving old behavior for calling code that relied on the implicit default. Delete the parameter to switch to the new behavior. The compat switch lives in `webservice`, not `webservice_core`: the latter is new and unreleased, so its `_request()` stays simple with no compat baggage.
`_request()` defaulted `content_only` to `True`, silently returning raw bytes instead of the `requests.Response` object callers expect. The OAuth2 backend adapter ignored the flag entirely, always returning bytes (fixes OCA#142). OCA#144 fixes the OAuth2 inconsistency but keeps `content_only` as a kwarg (default `None`, warns, still defaults to `True`) - perpetuating the actual problem: the return type shouldn't be a per-call switch, especially when raw content is just `.response.content` away. Here, `_request()` always returns the full response; the kwarg is dropped (a stray one is popped and warned about, not left to crash inside `requests.request()`). Backward compat is handled once, at the database level: the `18.0.2.0.1` upgrade script sets a `webservice.request_content_only` system parameter on existing databases only, preserving old behavior for calling code that relied on the implicit default. Delete the parameter to switch to the new behavior. The compat switch lives in `webservice`, not `webservice_core`: the latter is new and unreleased, so its `_request()` stays simple with no compat baggage.
5 participants
Fixes #142
Problem
BaseRestRequestsAdapter._request()defaultscontent_onlytoTrue,returning raw
bytesinstead of the expectedrequests.Responseobject.This violates the Principle of Least Astonishment, as most HTTP client
libraries return a Response object by default.
Additionally,
BackendApplicationOAuth2RestRequestsAdapter._request()ignored
content_onlyentirely, always returning.content, which wasinconsistent with the base adapter.
Solution
content_onlynow defaults toNone. When not explicitly set, adeprecation warning is logged and the current behavior (
True) ispreserved for backward compatibility.
content_onlyconsistently with the base adapter.
content_only=True, and explicitcontent_only=False.Testing
All 25 existing tests pass alongside the 3 new tests added.