Patch treq so public APIs don't swallow any unexpected kwargs. #251

evilham · 2019-07-29T16:45:45Z

PR following short conversation on IRC:

exarkun: ugh.  `treq.get`, etc all take **kwargs and silently ignore unrecognized arguments? :(
evilham: yup: https://github.com/twisted/treq/blob/master/src/treq/client.py#L144
only method and url are required, everything else is either used or ignored
exarkun: Bad.
evilham: maybe :-D yes, it shouldn't be too hard to patch, I agree I want the thing to explode in my face if I mistype `unbuffered` as `unbuffreed` instead of causing the whole system to swap and trash to death

^^^^ This has actually happened at some point :-) and is just awkward.

I consider this to be WIP because at the moment I have to stop and therefore can't add tests for "blow up in my face if passed unknown arguments", but since it shouldn't decrease coverage, is understandable and current tests pass, maybe if it passes review it can be fit for merging.

Notice that I had to change an internal API for this tow ork, shouldn't be too much of a problem I guess.

Up until now it was easy to, for example mistype `unbuffered=True` as `unbuffreed=True` which could have dare consequences :-). In general it sounds like complaining about unexpected data is a safer bet than just ignoring it. Also: Documented the files argument to `treq.request` which was only documented in the actual code for `treq.client.HTTPClient.request`.

altendky

Not that I have any authority, just noticed this quickly as I was glancing around here.

src/treq/client.py

requests.cookies.merge_cookies already checks and converts the cookies parameter, so there was no need for treq to do so.

Some code relies on the returned cookies object being defined properly and based off the CookieJar. This change ensures all tests pass again.

…args-request

glyph · 2019-08-05T05:25:55Z

@altendky Do you want some authority? 😉

altendky · 2019-08-05T12:59:45Z

@glyph if someone sees fit to give me authority I'll try to live up to it... here mostly I fell off the map because pycharm wouldn't let me set a breakpoint without totally trashing the trial run.

evilham · 2019-08-06T06:03:17Z

Thank you for fixing the lint @glyph, I had a very busy weekend.

~~Notice coverage decreases because previously fully tested code is now shorter :-/.~~

FWIW, I think @altendky's review was very thorough and it's the kind of review that makes the process of contributing to twisted(+ related projects) somewhat easy to do with confidence that things won't utterly break suddenly because of something missed in a rush.

altendky

Overall I'm not sure how this fits into backwards compatibility. The point of the change is exactly to break in cases where previously treq did not break. This seems good but it also hazards breaking existing code using treq. I'll go read up on treq/twisted policies here, though I expect someone else should just weigh in on the matter.

src/treq/api.py

src/treq/client.py

src/treq/api.py

glyph · 2019-08-06T17:56:29Z

This seems good but it also hazards breaking existing code using treq. I'll go read up on treq/twisted policies here, though I expect someone else should just weigh in on the matter.

the relevant policy is https://twisted.readthedocs.io/en/twisted-19.7.0/core/development/policy/compatibility-policy.html and I'll save you a moment reading through it: the thing to do here is to emit a warning for a few releases before raising an exception. so let's do that here.

src/treq/client.py

twm · 2019-08-08T05:13:09Z

(Sorry about that. Tabbed one too far.)

…EU/treq into evilham-dont-use-kwargs-request

…equest-001 Alternative approach with less kwargs muddling

altendky · 2019-08-08T18:48:26Z

src/treq/client.py

+                "Unsupported arguments: [",
+                ",".join(kwargs.keys()),
+                "]"]),
+                category=DeprecationWarning, stacklevel=2)


:] Listing the unsupported arguments is a nice touch for sure.

:[ When called directly we need stacklevel=2 but when called through treq.request() etc I think you would want stacklevel=3? I don't know how important this is but we could handle it via a silly stack traversal until we got out of treq. At least then it would be isolated here and not require passing params etc. I was curious to practice so I made up this... I tend to think it's kind of icky and this issue may not be worth addressing but... the result is nice. Hopefully the try/finally would successfully make the worst case be that you get a warning but don't know quite where from.

Py2: https://repl.it/@altendky/GrizzledWatchfulPostgres-5
Py3: https://repl.it/@altendky/GrotesqueAnnualTag-6

import inspect import warnings top_level_package = __name__.partition('.')[0] def warn(*args, **kwargs): try: if 'stacklevel' not in kwargs: stack = inspect.stack() for stack_level, frame_info in enumerate(stack, 1): module_name = inspect.getmodule(frame_info[0]).__name__ if not module_name.startswith(top_level_package + '.'): break else: stack_level = 3 kwargs['stacklevel'] = stack_level finally: kwargs.setdefault('stacklevel', 3) warnings.warn(*args, **kwargs)

Nice! I think I'd like to see this in twisted (if not python itself, twisted is a good first step).

I added it to treq._util with some "improvements", I hope you agree that makes the code more generic :-).

Amongst other things, I noticed that if the code lived somewhere outside of the top_level_package, it'd break the loop on the first frame, also, I think it has a few usability goodies.

Since you caught the difference between request and get, I went ahead and added a test for all supported methods.

@altendky

Thanks to @altendky, who came up with the gist of treq._utils.warn, which dinamically sets the stacklevel. This is adds a handy API that could be useful in twisted and other projects, while being fully compatible with the standard library function.

altendky · 2019-08-09T01:52:25Z

src/treq/_utils.py

@@ -45,3 +48,50 @@ def default_pool(reactor, pool, persistent):
        set_global_pool(HTTPConnectionPool(reactor, persistent=True))

    return get_global_pool()
+
+
+def warn(*args, **kwargs):


The try/finally was very intentional. This is doing some goofy stuff, which maybe you tidied up a bit, but what we really don't want to have happen is have our attempt to warn someone of a future exception... turn into another exception without the warning. Speaking of which, I probably should have had an except Exception: pass to avoid any exception caused by this inspection ever.

altendky · 2019-08-09T01:53:41Z

src/treq/_utils.py

+        Defaults to 1, which would match the place where this was called from.
+    """
+    # Default to top-level package
+    namespace = kwargs.pop('namespace', __name__.partition('.')[0])


The default could be gotten by grabbing the calling stack frame's module's name, if it has one. We're already inspecting like we just don't care, why not a bit more.

In so far as it needs to support other packages using it, perhaps it should get a class. Any code that uses it can create an instance which holds the desired top level package name and then call that instance instead of having to reinspect or repass the top level package. With it only needing to be passed once it's not a hassle to anyone to just require it be specified and never guess. Then we can disregard my last comment and have a tad less inspection.

altendky · 2019-08-09T02:42:23Z

src/treq/_utils.py

+        Defaults to 1, which would match the place where this was called from.
+    """
+    # Default to top-level package
+    namespace = kwargs.pop('namespace', __name__.partition('.')[0])


In so far as it needs to support other packages using it, perhaps it should get a class. Any code that uses it can create an instance which holds the desired top level package name and then call that instance instead of having to reinspect or repass the top level package. With it only needing to be passed once it's not a hassle to anyone to just require it be specified and never guess. Then we can disregard my last comment and have a tad less inspection.

altendky · 2019-08-09T02:52:16Z

src/treq/test/test_client.py

+    def test_request_unknown_arguments(self):
+        self._check_unknown_arguments('request',
+                                      self.test_request_unknown_arguments,
+                                      'GET', 'www.http://example.com')


Is this intentionally not 'http://www.example.com'?

altendky · 2019-08-09T03:01:04Z

src/treq/_utils.py

@@ -45,3 +48,50 @@ def default_pool(reactor, pool, persistent):
        set_global_pool(HTTPConnectionPool(reactor, persistent=True))

    return get_global_pool()
+
+
+def warn(*args, **kwargs):


Do we want to use functools.wraps or wrapt or... If we do that can we have extra args? I certainly have had places I should have used them but have never really learned them.

altendky · 2019-08-09T03:01:08Z

src/treq/_utils.py

+    """
+    # Default to top-level package
+    namespace = kwargs.pop('namespace', __name__.partition('.')[0])
+    default_stacklevel = kwargs.pop('default_stacklevel', 1)


Good golly this review is scattered... I should just stop for now. You are claiming the namespace (should this be top_level_package? even your comment says that) and default_stacklevel parameters. Perhaps there should be tests verifying that they are not valid for warnings.warn(). Just in case they get added someday. Or maybe that's excessive. I'm not sure offhand how to write them without just duplicating the names which hazards the test and this function getting out of sync.

glyph · 2019-10-03T03:39:56Z

Hi @evilham !

It would be great to get this done; any chance you could:

address some of the reviews
placate coveralls
resolve the conflicts?

Thanks!

glyph · 2020-08-04T04:28:56Z

Since this branch has unresolved conflicts and has now celebrated its first birthday, I'm closing it. The bug is still real though so I've opened #287 to track that. Anyone with time to follow up on the feedback above, please feel free to resubmit.

altendky requested changes Jul 29, 2019

View reviewed changes

src/treq/client.py Outdated Show resolved Hide resolved

Remove mutable defaults from method definitions.

67a80f1

altendky requested changes Jul 30, 2019

View reviewed changes

src/treq/client.py Show resolved Hide resolved

evilham and others added 4 commits July 30, 2019 14:47

Simplify cookie handling in treq.request.

dd6dd7e

requests.cookies.merge_cookies already checks and converts the cookies parameter, so there was no need for treq to do so.

Partially revert conditionally merging CookieJars.

2073e27

Some code relies on the returned cookies object being defined properly and based off the CookieJar. This change ensures all tests pass again.

lint on py3

1551efe

Merge remote-tracking branch 'origin/master' into evilham-dont-use-kw…

d5db670

…args-request

Merge branch 'master' into evilham-dont-use-kwargs-request

630efb4

altendky requested changes Aug 6, 2019

View reviewed changes

src/treq/api.py Outdated Show resolved Hide resolved

src/treq/client.py Outdated Show resolved Hide resolved

src/treq/api.py Outdated Show resolved Hide resolved

twm reviewed Aug 8, 2019

View reviewed changes

src/treq/client.py Outdated Show resolved Hide resolved

twm closed this Aug 8, 2019

twm reopened this Aug 8, 2019

evilham added 2 commits August 8, 2019 11:45

Simplify checking for data that is not JSON.

76451ac

Merge branch 'evilham-dont-use-kwargs-request' of github.com:Camilion…

3fee25e

…EU/treq into evilham-dont-use-kwargs-request

evilham mentioned this pull request Aug 8, 2019

Public APIs should raise an exception when passed unexpected arguments #258

Closed

3 tasks

evilham changed the title ~~Patch treq so public APIs don't swallow any unexpected kwargs.~~ [stuck until 18.6 + 2 releases] Patch treq so public APIs don't swallow any unexpected kwargs. Aug 8, 2019

Add warning and test for HttpClient.request.

aaa2bca

evilham changed the title ~~[stuck until 18.6 + 2 releases] Patch treq so public APIs don't swallow any unexpected kwargs.~~ Patch treq so public APIs don't swallow any unexpected kwargs. Aug 8, 2019

evilham and others added 3 commits August 8, 2019 18:43

Fix typo in description for parameters.

9130ef0

Alternative approach with less kwargs muddling

a5fc693

Merge pull request #1 from altendky/altendky-evilham-dot-use-kwargs-r…

589a769

…equest-001 Alternative approach with less kwargs muddling

altendky reviewed Aug 8, 2019

View reviewed changes

altendky mentioned this pull request Aug 8, 2019

Automatic stack level for warnings.warn() altendky/altendpy#13

Open

evilham force-pushed the evilham-dont-use-kwargs-request branch from 800b987 to 68df904 Compare August 8, 2019 22:50

altendky reviewed Aug 9, 2019

View reviewed changes

twm and others added 2 commits January 28, 2020 22:39

Merge branch 'master' into evilham-dont-use-kwargs-request

2e642ad

Merge branch 'master' into evilham-dont-use-kwargs-request

724ef14

glyph mentioned this pull request Aug 4, 2020

public APIs should not swallow arbitrary invalid keyword arguments #287

Closed

3 tasks

glyph closed this Aug 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Patch treq so public APIs don't swallow any unexpected kwargs. #251

Patch treq so public APIs don't swallow any unexpected kwargs. #251

evilham commented Jul 29, 2019 •

edited by glyph

Loading

altendky left a comment

glyph commented Aug 5, 2019

altendky commented Aug 5, 2019

evilham commented Aug 6, 2019 •

edited

Loading

altendky left a comment

glyph commented Aug 6, 2019

twm commented Aug 8, 2019

altendky Aug 8, 2019 •

edited

Loading

evilham Aug 8, 2019

altendky Aug 9, 2019

altendky Aug 9, 2019

altendky Aug 9, 2019

altendky Aug 9, 2019

altendky Aug 9, 2019

altendky Aug 9, 2019

altendky Aug 9, 2019

glyph commented Oct 3, 2019

glyph commented Aug 4, 2020

Patch treq so public APIs don't swallow any unexpected kwargs. #251

Patch treq so public APIs don't swallow any unexpected kwargs. #251

Conversation

evilham commented Jul 29, 2019 • edited by glyph Loading

altendky left a comment

Choose a reason for hiding this comment

glyph commented Aug 5, 2019

altendky commented Aug 5, 2019

evilham commented Aug 6, 2019 • edited Loading

altendky left a comment

Choose a reason for hiding this comment

glyph commented Aug 6, 2019

twm commented Aug 8, 2019

altendky Aug 8, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glyph commented Oct 3, 2019

glyph commented Aug 4, 2020

evilham commented Jul 29, 2019 •

edited by glyph

Loading

evilham commented Aug 6, 2019 •

edited

Loading

altendky Aug 8, 2019 •

edited

Loading