Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Regression in 1.1.0 #49

Closed
martinamps opened this issue Jun 3, 2021 · 7 comments
Closed

Regression in 1.1.0 #49

martinamps opened this issue Jun 3, 2021 · 7 comments

Comments

@martinamps
Copy link

martinamps commented Jun 3, 2021

Authentication has begun failing for us with multiple query strings because the 1.1.0 release (which is not tagged in GitHub) has introduced this commit from 5 years ago which appears to be a regression. The 1.0.1 branch did not include it

Any ideas how this happened / how it can be prevented moving forward?

Simple repro test:


qs_items = {'foo': ['bar', 'baz'], 'qux': ['3','2','1']}
qs_strings = []
for name in sorted(qs_items):
    vals = qs_items[name]
    for val in vals:
        qs_strings.append('='.join([name, val]))
    qs1 = '&'.join(qs_strings)

qs_strings = []
for name, vals in qs_items.items():
    for val in vals:
        qs_strings.append('='.join([name, val]))
    qs2 = '&'.join(sorted(qs_strings))

print(qs1)
print(qs2)

assert qs1 == qs2
foo=bar&foo=baz&qux=3&qux=2&qux=1
foo=bar&foo=baz&qux=1&qux=2&qux=3
Traceback (most recent call last):
  File "main.py", line 19, in <module>
    assert qs1 == qs2
AssertionError

EDIT: Ah I see though the commit is 5 years old, the merge just landed more recently.

@tedder
Copy link
Owner

tedder commented Jun 3, 2021

Martin- thanks for teasing out the root cause of the problem. I've been too casual with the release process, so I just cut it from main.

I want to compare the pip version against my local server that I uploaded from and see why there's a difference. I'll definitely add a unit test, thank you very much for giving breadcrumbs on that.

(feel free to PR some of it if you want)

@martinamps
Copy link
Author

Apologies I think I was mistaken about the release process bug - the git blame showing me the commit was 5 years old was misleading given it was merged in March. I think this will do it #50

@martinamps martinamps changed the title Regression in 1.1.0 code + release process Regression in 1.1.0 Jun 3, 2021
@tedder
Copy link
Owner

tedder commented Jun 3, 2021

Gotcha, I need to sort out how that affects #21, I think.

@tedder
Copy link
Owner

tedder commented Jun 3, 2021

I'm thinking it isn't about reverting #21 (in PR #23), it's about the need to sort the inner loop too, so "for val in vals:" needs sorting.

tedder added a commit that referenced this issue Jun 4, 2021
tedder added a commit that referenced this issue Jun 4, 2021
closes #50. A simple revert doesn't actually fix the regression, as the previous version would then fail the "hyphen in key" issue. This solves both.

Thanks to @martinamps for finding and documenting this in #49.
@tedder
Copy link
Owner

tedder commented Jun 4, 2021

Okay. I closed #50, which traded one bug for another. I also added unit tests for both- and in fact, you can see that the unit test for your problem isn't fixed until 1d01f46.

Do you have an environment where you can test this before I release it as v1.1.1? I mean, something like this:

pip install git+git://github.com/tedder/requests-aws4auth.git@1d01f46950d9290773939bf17ff73d67f8cfa308

@martinamps
Copy link
Author

@tedder thanks for the quick turnaround, can confirm this works :)

@tedder
Copy link
Owner

tedder commented Jun 4, 2021

great! I just uploaded it to pypi.

@tedder tedder closed this as completed Jun 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants