Skip to content

[FIX] webservice: deprecate implicit content_only=True default - #144

Open
HasanAlsaafen wants to merge 1 commit into
OCA:18.0from
HasanAlsaafen:fix-content-only-default-142
Open

[FIX] webservice: deprecate implicit content_only=True default#144
HasanAlsaafen wants to merge 1 commit into
OCA:18.0from
HasanAlsaafen:fix-content-only-default-142

Conversation

@HasanAlsaafen

Copy link
Copy Markdown

Fixes #142

Problem

BaseRestRequestsAdapter._request() defaults content_only to True,
returning raw bytes instead of the expected requests.Response object.
This violates the Principle of Least Astonishment, as most HTTP client
libraries return a Response object by default.

Additionally, BackendApplicationOAuth2RestRequestsAdapter._request()
ignored content_only entirely, always returning .content, which was
inconsistent with the base adapter.

Solution

  • content_only now defaults to None. When not explicitly set, a
    deprecation warning is logged and the current behavior (True) is
    preserved for backward compatibility.
  • Fixed the OAuth2 backend application adapter to respect content_only
    consistently with the base adapter.
  • Added tests covering: default behavior with warning, explicit
    content_only=True, and explicit content_only=False.

Testing

All 25 existing tests pass alongside the 3 new tests added.

- 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
@OCA-git-bot

Copy link
Copy Markdown
Contributor

Hi @etobella,
some modules you are maintaining are being modified, check this out!

@Ricardoalso Ricardoalso left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@HasanAlsaafen

Copy link
Copy Markdown
Author

Hey @Ricardoalso
Thanks for raising this, it's a valid concern.

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.

@simahawk

Copy link
Copy Markdown
Contributor

@HasanAlsaafen sounds like a good approach. Can you pls rewrite the commit msg as [FIX] webservice: deprecate implicit content_only=True default.
Regarding other modules: I don't know any other module in the OCA that directly relies on webservice.call but you could search for modules depending on it.
Last but not least. pls check the CI 🙏

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Open Concern about webservice module defaulting content_only to True in "base.requests"

5 participants