Re: [Django] #37025: RemoteUserMiddleware.header does not correspond to request.META in the async path

[Django]

#37025: RemoteUserMiddleware.header does not correspond to request.META in the
async path
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage:
RemoteUserMiddleware | Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Carlton Gibson):

Thanks for the draft Jacob. I think that looks good. (And covers the
ground that we need to ''by default'' IMO — Folks needing more can
customise as necessary.)
--
Ticket URL: <https://code.djangoproject.com/ticket/37025#comment:12>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#37025: RemoteUserMiddleware.header does not correspond to request.META in the
async path
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted
RemoteUserMiddleware |

Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* stage: Unreviewed => Accepted

Comment:

Thank you Jacob, I like where this is going
--
Ticket URL: <https://code.djangoproject.com/ticket/37025#comment:13>

[Django]

#37025: RemoteUserMiddleware.header does not correspond to request.META in the
async path
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
RemoteUserMiddleware |
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Mykhailo Havelia):

Replying to [comment:3 Sarah Boyce]:
> Thank you for the ticket
>
> From reviewing the PR that implemented this, I would say our approach
was to get test parity by async-ifying the sync tests. This was then
required for the tests to pass and, therefore, seemed required. I am now
under the impression that our logic as to how we wrote the async tests
must be off. Have you looked into what changes may be required there? I
think that will help identify why we got this wrong
>
> This is a partial duplicate of #36443 and #36300

I don't think we can improve our test tooling to prevent something like
this in the future. But we do have experience supporting sync/async
middlewares with separate tests (like in the PR), and I can say with
certainty that it's a bad approach for long-term maintenance 😌. At first
it works fine, you just copy the sync tests, swap in the async
equivalents, and you're done. But over time it becomes hard to maintain,
and having separate tests doesn't actually guarantee that the sync and
async paths behave the same way.

`RemoteUserMiddleware` is the first and only pure async middleware in
Django, so we can experiment with testing tooling and approaches here, and
then apply what we learn to future pure async middlewares.

Just a thought, if it makes sense 😌
--
Ticket URL: <https://code.djangoproject.com/ticket/37025#comment:14>

[Django]

#37025: RemoteUserMiddleware.header does not correspond to request.META in the
async path
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
RemoteUserMiddleware |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* needs_better_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/37025#comment:15>

[Django]

#37025: RemoteUserMiddleware.header does not correspond to request.META in the
async path
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
RemoteUserMiddleware |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Sarah Boyce):

* needs_better_patch: 1 => 0

--
Ticket URL: <https://code.djangoproject.com/ticket/37025#comment:16>

[Django]

#37025: RemoteUserMiddleware.header does not correspond to request.META in the
async path
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
RemoteUserMiddleware |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 1

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* needs_better_patch: 0 => 1

Comment:

I think we need more discussion about whether to duplicate the middlewares
here.
--
Ticket URL: <https://code.djangoproject.com/ticket/37025#comment:17>

[Django]

#37025: RemoteUserMiddleware.header does not correspond to request.META in the
async path
-------------------------------------+-------------------------------------
Reporter: Jacob Walls | Owner: Jacob
| Walls
Type: Bug | Status: assigned
Component: contrib.auth | Version: 5.2
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
RemoteUserMiddleware |
Has patch: 1 | Needs documentation: 0

Needs tests: 0 | Patch needs improvement: 0

Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by Jacob Walls):

* needs_better_patch: 1 => 0

Comment:

Thanks Sarah for exploring that angle. I'm worried we're mixing a couple
things together here. Mykhailo found an existing bug during the review of
my PR, which I've now fixed. We shouldn't use that as a justification for
duplicating the middlewares, which is as disruptive as what Carlton winced
at in comment:5.

I've updated my [https://github.com/django/django/pull/21079 PR], and
think it's ready for another look.
--
Ticket URL: <https://code.djangoproject.com/ticket/37025#comment:18>