aiohttp/web.py: fixing middleware resolution order accross multiple applications

[Anvil]

What do these changes do?

This should fix #1853

Are there changes in behavior for the user?

Yes, likely.

Related issue number

#1853

Checklist

• I think the code is well written
• Unit tests for the changes exist
• Documentation reflects the changes
• If you provide code modification, please add yourself to CONTRIBUTORS.txt
  • The format is <Name> <Surname>.
  • Please keep alphabetical order, the file is sorted by names.
• Add a new entry to CHANGES.rst
  • Choose any open position to avoid merge conflicts with other PRs.
  • Add a link to the issue you are fixing (if any) using #issue_number format at the end of changelog message. Use Pull Request number if there are no issues for PR or PR covers the issue only partially.

[Anvil]

I honestly don't know if the right way is what I've done, or changing the match_info.add_app method behavior, since I don't know if there would be unwanted any side effect.

But in any case, you've got the point: the list match_info.apps list should be reversed.

[fafhrd91]

That's probably regression in 2.x version.

@asvetlov could you look?

[codecov-io]

Codecov Report

Merging #1854 into 2.0 will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##              2.0    #1854   +/-   ##
=======================================
  Coverage   96.71%   96.71%           
=======================================
  Files          37       37           
  Lines        7433     7433           
  Branches     1294     1294           
=======================================
  Hits         7189     7189           
  Misses        114      114           
  Partials      130      130

Impacted Files	Coverage Δ
aiohttp/web.py	97.4% <100%> (ø)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a17d55e...68d351d. Read the comment docs.

[asvetlov]

I'll have a time for it tomorrow evening.

[asvetlov]

@Anvil could you add a unit test?

[Anvil]

The test is actually already written, but with a wrong assumption. I'll fix that.

[Anvil]

See, Application inherits from MutableMapping, which leads to app == subapp1 == subapp2 being True in test_subapp_middlewares, because all their keys and matching values are equal. Same way as dict(foo=1) == dict(foo=1) is also True.

[Anvil]

FTR, I think many tests are broken because of this assumption.

[fafhrd91]

@asvetlov ping

[asvetlov]

See #1866 (comment)

[Anvil]

Do you want me to revert the changes on the test so that we can close this MR and the corresponding issue ?

[asvetlov]

Yes. Let's fix application comparison (and middleware ordering along with other potential issues).

[fafhrd91]

@asvetlov what is the status on this PR? whet is next?

[Anvil]

I've rolled back the commits on the unit tests, yesterday. So, IMHO, it's good to merge. And I'd be interested in a bugfix release in the next few weeks, to be honest. ;)

[fafhrd91]

@Anvil could you rebase against master?

[Anvil]

Done. FWIW, this bug is a blocker here. We'd like to see it fixed in 2.0 series, if possible.

[fafhrd91]

I don't think this is good fix for 2.0 This change changes behavior, even if it is broken behavior
I am planing to publish 2.1 this weekend.

@asvetlov @kxepal any comment?

[kxepal]

I think this is indeed a breaking change, even if it's fixes the issue. It is indeed about behaviour change.

The change itself looks reasonable and good for me. I would only be a bit more clear and correct about it in change log.

The Fix sub-application middlewares resolution order means resolution order was broken in some way. It actually wasn't, but it was different and it's now reversed. Small addition about what exactly changed, why and how upgrade to new behaviour could solve a lot of questions and save people from WTF times. I hope.

[fafhrd91]

thanks!

I am planing to release 2.1 this weekend

[Anvil]

Thanks for the merge.

[lock]

This thread has been automatically locked since there has not been
any recent activity after it was closed. Please open a new issue for
related bugs.

If you feel like there's important points made in this discussion,
please include those exceprts into that new issue.