Prefer python_multipart import over multipart #3710

musicinmybrain · 2024-10-28T17:07:30Z

See also releases 0.0.13 through 0.0.16 at
https://github.com/Kludex/python-multipart/releases.

This prefers the new python_multipart import name for https://pypi.org/project/python-multipart/ introduced in release 0.0.13 to resolve conflicts with https://pypi.org/project/multipart/. A fallback to the original multipart import name remains for compatibility with older versions.

Starlette itself now does something similar:

encode/starlette@0.41.0...0.41.2

I tried tox -e linters, tox -e py3.13-starlette-latest, tox -e py3.13-starlette-0.36, and tox -e py3.13-starlette-0.32 locally. I don’t expect any trouble with the rest of the test matrix.

musicinmybrain · 2024-10-28T17:09:02Z

This avoids a PendingDeprecationError in the tests when using the multipart import name with python-multipart 0.0.13 or later.

szokeasaurusrex

Thanks for the contribution! This mostly looks good.

Two things though:

Would you be able to add a test to ensure we don't get the PendingDeprecationWarning?
Also small nit, I think it might be better to import python_multipart as multipart. This would minimize the diff, since we would not need to update existing usages of multipart.

Let me know what you think; I can also take over the PR from here if you prefer.

szokeasaurusrex · 2024-10-29T09:15:34Z

sentry_sdk/integrations/starlette.py

+        # python-multipart 0.0.13 and later
+        import python_multipart  # type: ignore
+    except ImportError:
+        # python-multipart 0.0.12 and later


Did you mean "earlier" here?

Suggested change

# python-multipart 0.0.12 and later

# python-multipart 0.0.12 and earlier

Good catch. Thanks. I will fix this.

codecov · 2024-10-29T09:27:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 84.27%. Comparing base (d48dc46) to head (479c951).
Report is 2 commits behind head on master.

✅ All tests successful. No failed tests found.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3710      +/-   ##
==========================================
- Coverage   84.28%   84.27%   -0.02%     
==========================================
  Files         133      133              
  Lines       14257    14260       +3     
  Branches     2405     2405              
==========================================
+ Hits        12017    12018       +1     
+ Misses       1490     1489       -1     
- Partials      750      753       +3

Files with missing lines	Coverage Δ
sentry_sdk/integrations/starlette.py	`84.81% <100.00%> (+0.13%)`	⬆️

... and 3 files with indirect coverage changes

musicinmybrain · 2024-10-29T10:43:02Z

* Would you be able to add a test to ensure we don't get the `PendingDeprecationWarning`?

I am not sure how to add a proper test since the PendingDeprecationWarning arises when multipart is imported, which happens when sentry_sdk/integrations/starlette.py is imported:

sentry-python/tests/integrations/starlette/test_starlette.py

Lines 16 to 19 in d48dc46

    
           from sentry_sdk.integrations.starlette import ( 
        
               StarletteIntegration, 
        
               StarletteRequestExtractor, 
        
           )

And of course, a second import would use caches, and the warning would not be triggered again.

Maybe I can wrap the import with catch_warnings() and at least get PendingDeprecationWarning to cause an error in test collection, even if I can’t add a named test.

* Also small nit, I think it might be better to `import python_multipart as multipart`. This would minimize the diff, since we would not need to update existing usages of `multipart`.

Sure, that’s fine. I defaulted to using the new import name throughout because I thought it would be least confusing in the future when everyone has forgotten the old name, but there is no harm in doing it as you suggested.

I’m going to go ahead and force-push with the other changes while I think about testing.

…tarlette integrations

See: Kludex/python-multipart#16 See also releases 0.0.13 through 0.0.16 at https://github.com/Kludex/python-multipart/releases.

musicinmybrain · 2024-10-29T11:00:31Z

Added a commit that causes an error to occur in the tests when there is a PendingDeprecationWarning or DeprecationWarning from the Starlette integrations.

With just the first commit, tox -e py3.13-starlette-latest gives:

======================================================== ERRORS =========================================================
____________________________ ERROR collecting tests/integrations/starlette/test_starlette.py ____________________________
tests/integrations/starlette/test_starlette.py:22: in <module>
    from sentry_sdk.integrations.starlette import (
sentry_sdk/integrations/starlette.py:68: in <module>
    import multipart  # type: ignore
.tox/py3.13-starlette-latest/lib/python3.13/site-packages/multipart/__init__.py:19: in <module>
    warnings.warn("Please use `import python_multipart` instead.", PendingDeprecationWarning, stacklevel=2)
E   PendingDeprecationWarning: Please use `import python_multipart` instead.
---------------------------- generated xml file: /home/ben/src/forks/sentry-python/.junitxml ----------------------------
================================================ short test summary info ================================================
ERROR tests/integrations/starlette/test_starlette.py - PendingDeprecationWarning: Please use `import python_multipart` instead.
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!! Interrupted: 1 error during collection !!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
=================================================== 1 error in 0.32s ====================================================

With both commits:

=================================================== warnings summary ====================================================
tests/integrations/starlette/test_starlette.py::test_starletterequestextractor_json
tests/integrations/starlette/test_starlette.py::test_starletterequestextractor_form
tests/integrations/starlette/test_starlette.py::test_starletterequestextractor_body_consumed_twice
tests/integrations/starlette/test_starlette.py::test_starletterequestextractor_extract_request_info_too_big
tests/integrations/starlette/test_starlette.py::test_starletterequestextractor_extract_request_info
tests/integrations/starlette/test_starlette.py::test_starletterequestextractor_extract_request_info_no_pii
  /usr/lib64/python3.13/asyncio/events.py:89: RuntimeWarning: coroutine '_mock_receive' was never awaited
    self._context.run(self._callback, *self._args)
  Enable tracemalloc to get traceback where the object was allocated.
  See https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings for more info.

tests/integrations/starlette/test_starlette.py::test_template_tracing_meta
  /home/ben/src/forks/sentry-python/.tox/py3.13-starlette-latest/lib/python3.13/site-packages/starlette/templating.py:161: DeprecationWarning: The `name` is not the first parameter anymore. The first parameter should be the `Request` instance.
  Replace `TemplateResponse(name, {"request": request})` by `TemplateResponse(request, name)`.
    warnings.warn(

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
---------------------------- generated xml file: /home/ben/src/forks/sentry-python/.junitxml ----------------------------
================================================== slowest 5 durations ==================================================
0.19s call     tests/integrations/starlette/test_starlette.py::test_transaction_style[/message-url-/message-route]
0.13s call     tests/integrations/starlette/test_starlette.py::test_transaction_http_method_custom
0.11s call     tests/integrations/starlette/test_starlette.py::test_catch_exceptions[/some_url-ZeroDivisionError-division by zero]
0.11s call     tests/integrations/starlette/test_starlette.py::test_configurable_status_codes_deprecated[failed_request_status_codes7-403-True]
0.11s call     tests/integrations/starlette/test_starlette.py::test_user_information_transaction
============================================ 67 passed, 7 warnings in 5.12s =============================================
<sys>:0: ResourceWarning: unclosed file <_io.BufferedReader name='/home/ben/src/forks/sentry-python/tests/integrations/starlette/photo.jpg'>

szokeasaurusrex · 2024-10-29T11:59:12Z

@musicinmybrain I don't think we should test in this way, with the no warnings assertion outside of a test function. If it is non-trivial to add a test, let's just skip adding a test here.

szokeasaurusrex

Removed the test code, looks good now! Thanks again for the contribution.

szokeasaurusrex reviewed Oct 29, 2024

View reviewed changes

musicinmybrain force-pushed the multipart-rename branch 3 times, most recently from a923bf8 to 55ef83c Compare October 29, 2024 10:44

musicinmybrain added 2 commits October 29, 2024 06:59

In tests, error on deprecation warnings about python_multipart from s…

279d16f

…tarlette integrations

Prefer python_multipart import over multipart

56550e2

See: Kludex/python-multipart#16 See also releases 0.0.13 through 0.0.16 at https://github.com/Kludex/python-multipart/releases.

musicinmybrain force-pushed the multipart-rename branch from 55ef83c to 56550e2 Compare October 29, 2024 11:00

remove testing code

479c951

szokeasaurusrex approved these changes Oct 29, 2024

View reviewed changes

szokeasaurusrex added the Trigger: tests using secrets PR code is safe; run CI label Oct 29, 2024

szokeasaurusrex self-assigned this Oct 29, 2024

szokeasaurusrex enabled auto-merge (squash) October 29, 2024 12:02

szokeasaurusrex disabled auto-merge October 29, 2024 12:02

szokeasaurusrex enabled auto-merge (squash) October 29, 2024 12:02

szokeasaurusrex merged commit 000c8e6 into getsentry:master Oct 29, 2024
137 of 139 checks passed

philpep mentioned this pull request Oct 31, 2024

Compatibility with python-multipart in Starlette integration #3692

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefer python_multipart import over multipart #3710

Prefer python_multipart import over multipart #3710

musicinmybrain commented Oct 28, 2024

musicinmybrain commented Oct 28, 2024

szokeasaurusrex left a comment

szokeasaurusrex Oct 29, 2024

musicinmybrain Oct 29, 2024

codecov bot commented Oct 29, 2024 •

edited

Loading

musicinmybrain commented Oct 29, 2024

musicinmybrain commented Oct 29, 2024

szokeasaurusrex commented Oct 29, 2024

szokeasaurusrex left a comment

	# python-multipart 0.0.12 and later
	# python-multipart 0.0.12 and earlier

Prefer python_multipart import over multipart #3710

Prefer python_multipart import over multipart #3710

Conversation

musicinmybrain commented Oct 28, 2024

musicinmybrain commented Oct 28, 2024

szokeasaurusrex left a comment

Choose a reason for hiding this comment

szokeasaurusrex Oct 29, 2024

Choose a reason for hiding this comment

musicinmybrain Oct 29, 2024

Choose a reason for hiding this comment

codecov bot commented Oct 29, 2024 • edited Loading

Codecov Report

musicinmybrain commented Oct 29, 2024

musicinmybrain commented Oct 29, 2024

szokeasaurusrex commented Oct 29, 2024

szokeasaurusrex left a comment

Choose a reason for hiding this comment

codecov bot commented Oct 29, 2024 •

edited

Loading