Follow repo perm redirect

[sztomi]

This PR supersedes #1310 and fixes #858

In the documentation, I took the liberty to change the "foo.bar" domain to that of TestPyPI, because it's a common URL that people might want to add.

Pull Request Check List

• Added tests for changed code.
• Updated documentation for changed code.

[Caligatio]

Wouldn't it make more sense to somehow hook URL usage versus config? Many people don't use the config command and this also means you would potentially catch a redirect once during the lifetime of a project.

[sztomi]

@Caligatio What do they use instead? My original PR did follow the redirect at the time it was returned from the server, but there no really good options there. Either:

• POST the data to the endpoint, and if there is a redirect, follow it manually and POST again (meaning double upload time)
• Issue a HEAD request before the POST, and follow any redirects (that's what I implemented in my original PR) - this results in an extra request for everyone, for each upload.

[sztomi]

(clicked the wrong button)

[Caligatio]

I'm a bit torn whether the below ideas are over-engineering the situation or not. However, I would definitely have a follow_redirects=True on the POST. The current code depends on, essentially, one redirect-change happening in the lifetime of the repository otherwise we'll have another silent error. Now, in terms of possible over-engineering ideas:

• Check the redirect on repository instantiation (happens per poetry run)?
• Check the redirect on every POST (i.e. do a HEAD everytime)?
• Check the redirect on the first usage of a repository?

To be clear here, I'm not a maintainer so my ideas are just ideas, not requests.

EDIT: I have #1325 which needs to hook every HTTP request with custom certs. I have no idea if it will be actually accepted but, the more you can use already existing session objects, the more compatible things would be.

[sztomi]

However, I would definitely have a follow_redirects=True on the POST

Unfortunately, that doesn't help. It sends a 301 redirect, which allows changing a POST to a GET, and that is exactly what requests does. The end result is that the silent upload failure still happens (because a GET request to the same endpoint succeeds).

Check the redirect on repository instantiation (happens per poetry run)?

I don't know enough about the poetry internals to judge this, but if you can point out where it happens, I'm happy to investigate.

Check the redirect on every POST (i.e. do a HEAD everytime)?

That's basically my original proposal, but @brycedrennan was concerned about the performance impact.

Check the redirect on the first usage of a repository?

This is not substantially different from the current PR (unless I'm misunderstanding something).

To be clear here, I'm not a maintainer so my ideas are just ideas, not requests

Ideas and discussion are very much appreciated, maintainer or not :)

[Caligatio]

I confused source and upload repositories in my mind when I gave above options; sorry about that. There are actually fewer places that are possibilities:

• When someone configs an upload repository (like you have)
  • Pros: It can write the updated value back to the config
  • Cons: The update only happens at the time of the original config call
• In the Publisher class
  • Pros: The updated value gets updated per publish, no matter how many files
  • Cons:
    • I don't believe you can write the value back to the config
    • This involves making a new requests session (see previous comment about certs)
• In the Uploader class
  • Pros: The updated value gets updated per publish, no matter how many files
  • Cons: I don't believe you can write the value back to the config

I don't think there is a perfect solution here. In the Uploader seems best in terms of always working but then you don't get to perma-write the value back to the config. Maybe throw a warning in Uploader if an updated URL is detected? The idea was brought up in #1310. I could maybe make the argument that a warning and forcing the user to update their own config is clearer than auto updating a config :)

[sztomi]

I don't believe you can write the value back to the config

Why not though?

I think the ideal solution would be all of the following:

• Perform a normal POST
• Check for a redirect
  • If there is a redirect:
    • Redo POST to the redirect location
    • Save redirect location as URL if the redirect is permanent
• Check for a permanent redirect at config-save time

The last point is essentially this PR. I think both changes would improve the behavior independently and it is worth doing both.

[Caligatio]

It appears that the way configs were handled changed drastically between master and develop. It would have been entirely possible to write the values back in master as a handle was kept to each of the configs and they closely resembled files. In develop, the concept of the config was abstracted away and the individual config files are merged into the abstracted config; there is no longer a handle to the underlying files.

[brycedrennan]

@sztomi Thanks for your work on this and the first iteration of the PR.

I'm a big fan of just providing a meaningful error message that will direct the user to fix their config. This avoids the need for extra requests or automatic configuration updates.

What do you think? Perhaps your original PR can be re-purposed to that?

[sztomi]

I think this current PR would improve the situation independently, so I would keep this in addition to what you suggest. I suppose a clear error message is good though.

[sdispater]

Thanks for taking the time to make this PR!

However, I don't the config command should have any logic in it, it's just a way to edit the configuration file, and especially not make a POST request to the configured url. So I am closing this.

The work should be to improve the publish command and catch upload errors and display them properly.

[sztomi]

thanks @sdispater - do you think that publish should fix the configuration automatically when a permanent redirect is detected?

[brycedrennan]

For my part I think it’s safer to just provide a meaningful error. The user can then fix the configuration themselves.

[github-actions]

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