Artifactory overwrite message changed?

[marcz63]

Is there an existing issue for this?

• I have searched the existing issues (open and closed), and could not find an existing issue

What keywords did you use to search existing issues?

artifactory

What operating system are you using?

Linux, Windows 11

If you selected 'Other', describe your Operating System here

No response

What version of Python are you running?

Python 3.9 and 3.12

How did you install twine? Did you use your operating system's package manager or pip or something else?

pip install twine

What version of twine do you have installed (include the complete output)

twine version 6.1.0 (keyring: 25.6.0, packaging: 24.1, requests: 2.32.3, requests-toolbelt: 1.0.0, urllib3: 2.2.3, id: 1.5.0, importlib-metadata:
8.5.0)

Which package repository are you using?

pypi.org

Please describe the issue that you are experiencing

Since an Artifactory update, my twine upload --skip-existing command is now returning an error in case the artifact already exists.

With --verbose option I get a Not enough permissions to delete/overwrite all artifacts under.

I see in the code of twine that it is checking against a overwrite artifact message (note the all difference).

So I am guessing there was a change of behaviour has been made in Artifactory and twine should follow maybe?

Please list the steps required to reproduce this behaviour

1. have an Artifactory instance?
2. create a venv
3. pip install latest twine
4. twine upload --skip-existing some python package already present in artifactory! (or do it twice ;)
5. you get a return code not 0

Anything else you'd like to mention?

Thanks for your help :)

[sigmavirus24]

I'm really leaning towards making this PyPI only because the number of times they have broken this while refusing to provide a reasonable status code is way more often than standard library backwards incompatibilities (which last happened with importlib.metadata years ago).

Thoughts @woodruffw ?

[tscheepers]

Having the same issue, opened: #1252

[sigmavirus24]

@tscheepers your PR makes this so generic as to end up skipping and swallowing errors that we shouldn't. This is my point. A company unilaterally changes a message and provides us no really consistent way to detect this problem and we fall back to increasingly worse hacks.

We started supporting these indexes but they consistently break things leading to worse and worse "fixes". This feature was only added because PyPI gives us a consistent behavior and the ability to verify the upload happened at all before trying the upload.

[dnicolodi]

If the index implementation changes the returned message, why can't they change the returned status code to something specific? Returning 409 seems the most natural thing to do in this case. I guess that till twine keeps working around the issue, there will not be enough motivation for index maintainers to fix this.

[woodruffw]

I'm really leaning towards making this PyPI only because the number of times they have broken this while refusing to provide a reasonable status code is way more often than standard library backwards incompatibilities (which last happened with importlib.metadata years ago).

Thoughts @woodruffw ?

+1 from me -- the fix is straightforward on Artifactory's side, and IMO they should be incentivized to make it.

(I'm also baffled - like with pypi/warehouse#18275 - how this even landed in production in Artifactory. I can only assume they're not doing any amount of downstream or tool integration testing, despite presumably directing users towards twine for uploads.)

[sigmavirus24]

If the index implementation changes the returned message, why can't they change the returned status code to something specific?

All of the users that encounter this first are almost always users at a company with a procurement relationship with the index creators. They have the best leverage to push on this.

I vote for a 5.x release stripping these hacks with a big warning in the changelog. At this point the exploitation of the community for continued revenue on the company's part is unacceptable

[morotti]

wow, i was reviewing the code and I have to say I am surprised.
let's not blame artifactory for badly written code.

I am somewhat familiar with artifactory for managing the python indexes at my workplace and it works fine.
artifactory gives a 403 error when twines tries to upload a package that's already there.
403 permission denied because the repository does not allow to overwrite packages. It's possible to allow overwriting permissions in theory but in practice files must never be replaced in a pypi repo.
(pip is caching packages with filename and hash. pip will detect a corrupted cache and refuse to run if a package is modified, pip will never work again until you run "pip cache purge" on all the affected machines.)

twine should never have reached the 403 error. twine shouldn't have tried to upload the package because the package already existed.
the issue is with twine.
the problem is that "twine --skip-existing" does not work at all. it's not an artifactory issue.

looking at the twine source code:

1. there is a check to see whether the package is present... but the function package_is_uploaded() doesn't work, it's always returning False because there is no API to check that a package already exists. it's horribly broken code!
2. then twine uploads the file anyway.
3. then there is another check skip_upload() to inspect the error response after the fact and try to guess whether the error could have been because the package already existed. it's a giant hack. it's not skipping anything, the upload already happened and failed!

source code
https://github.com/pypa/twine/blob/cac723964ba9183fe897c15de19a21bf5c08db10/twine/commands/upload.py#L217C12-L217C23

I can think of a few ways to fix it:

option 1: Remove all this code that's horribly broken and remove that flag --skip-existing that never worked.

option 2: Fix twine to actually check that the package exists. (might be difficult, the comments say there are no API to do that).

option 3: Rewrite this broken function to fail when it doesn't work. it can only work on pypi

# bad code
def package_is_uploaded(
    self, package: package_file.PackageFile, bypass_cache: bool = False
) -> bool:
    # NOTE(sigmavirus24): Not all indices are PyPI and pypi.io doesn't
    # have a similar interface for finding the package versions.
    if not self.url.startswith((LEGACY_PYPI, WAREHOUSE, OLD_WAREHOUSE)):
        return False

# correct code
def package_is_uploaded(
    self, package: package_file.PackageFile, bypass_cache: bool = False
) -> bool:
    # NOTE(sigmavirus24): Not all indices are PyPI and pypi.io doesn't
    # have a similar interface for finding the package versions.
    if not self.url.startswith((LEGACY_PYPI, WAREHOUSE, OLD_WAREHOUSE)):
        raise Exception("cannot verify whether a package exists, the API for --skip-existing is only supported on pypi.org")

[sigmavirus24]

@morotti thank you for confirming the idea to forbid the use of --skip-existing on things that aren't PyPI

[woodruffw]

the problem is that "twine --skip-existing" does not work at all. it's not an artifactory issue.

It works just fine on PyPI, and it works well enough on other indices that don't change their responses often. The issue here is that Artifactory changed their response, meaning that the hack that's in place there no longer applies.

twine should never have reached the 403 error. twine shouldn't have tried to upload the package because the package already existed.

twine cannot know that it should never have reached that 403, since different indices use different 4xx codes for signalling here.

In this case I agree with making --skip-existing only work for PyPI/TestPyPI, like Ian originally said in #1251 (comment). So option 3 is what we were already going to do here.

(TL;DR: The entire situation here is extremely unfortunate, and stems ultimately from the fact that there's no standard upload API. That will eventually be addressed, but in the mean time twine is stuck in a hard position between accreting brittle hacks and not supporting various "key" features like --skip-existing when the target index isn't PyPI/TestPyPI. Removing the brittle hacks makes the most sense to me, especially if third-party index providers aren't regression-testing their changes.)

[sigmavirus24]

twine cannot know that it should never have reached that 403, since different indices use different 4xx codes for signalling here.

Worse yet. Artifactory didn't return 403 before but returned something else entirely. To @dnicolodi's point, they have already changed the status code once. Behaving the same as anything that actually targets interoperability with PyPI can't be that hard can it? It's not as if PyPI is closed source, or undocumented, or as if they couldn't test tools like twine continuously against their own product.

PyPI (old old pre-warehouse PyPI) returned 400. I argued with Donald about returning 409 instead in Warehouse and we landed on making the transition from Cheeseshop to Warehouse as seamless as possible. If basically one person can maintain backwards compatibility with almost little thought, why can't Artifactory manage the same? I don't like PyPI's 400 personally but I respect it. Artifactory and other indexes seems to be at whatever whim they want this week/month/quarter. Maybe this drives up shareholder value somehow? I don't know. Maybe they're waiting to offer some additional license set to upload to their PyPI endpoints with the same features. It would not surprise me.