specification: make is_same_package_as() commutative and …

[radoering]

always return False if source_types do not match

Fixes an issue where p1.is_same_package_as(p2) returns True, but p2.is_same_package_as(p1) return False.

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

[radoering]

Even though the current behavior is clearly incorrect (because not commutative), this change might be too risky without further investigation.

It's very likely, that in some cases the behavior as proposed in this PR is desired, but in other cases a less strict behavior à la "package is the same if name is the same" is required (see python-poetry/poetry#5617 for example). These cases have to be identified and fixed before fixing is_same_package_as().

[radoering]

@abn Since you had some concerns about this change influencing the behavior of the installer, I investigated that a bit. This should be the relevant part in the code: https://github.com/python-poetry/poetry/blob/80b03ece039ec72e86d87295ea557a4282891bd9/src/poetry/puzzle/transaction.py#L45-L51

It's obvious that there is a special handling if PyPI dependencies (no source_type) and legacy dependencies are involved. Given the version is equal, I investigated the relevant combinations:

practical use case	installed_package	result_package	update
yes*	pypi	legacy	no (because of the source_type check in the linked code)
no**	legacy	pypi	no (because of the bug fixed in this PR)
yes	pypi	direct	yes
yes	direct	pypi	no (because of the bug fixed in this PR)
no**	legacy	direct	yes
yes	direct	legacy	yes

* Special case is necessary because when installing a package with source_type=="legacy", the installed package has no source_type anymore.
** Only theoretical use cases because installed_package cannot have source_type legacy.

I assume the missing update when switching from direct to pypi is a real bug. The missing update when switching between pypi and legacy may be desired (though I'm not sure if it is sensible). If it is desired it should be handled directly in the linked spot and not partially relying on a weird behavior of is_same_package_as().

By the way, due to is_same_package_as() not being commutative, Package.__eq__() is not commutative! __eq__ not being commutative is kind of 💩 and may provoke even stranger bugs. I kind of feel like we should fix this better sooner than later even if it's risky...

What do you think? Fix https://github.com/python-poetry/poetry/blob/80b03ece039ec72e86d87295ea557a4282891bd9/src/poetry/puzzle/transaction.py#L45-L51 one way or the other (so it does not rely on the weird is_same_package_as() behavior) and than merge this one prior to the next pre-release? (No fix necessary, see next comment.)

[radoering]

I did some further investigations. It seems an installed package is never of type legacy. That's just not tracked. If you install a package with source_type=="legacy", the package in the installed repository has no source_type. Thus, cases in the table in my previous comment with installed_package of type legacy are only theoretical use cases and making is_same_package_as() commutative should not change the behavior of the install command (except for the desired fix when overwriting a direct origin dependency by a PyPI dependency). No need for a downstream fix.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
1.5% Duplication

[neersighted]

I did some further investigations. It seems an installed package is never of type legacy. That's just not tracked. If you install a package with source_type=="legacy", the package in the installed repository has no source_type. Thus, cases in the table in my previous comment with installed_package of type legacy are only theoretical use cases and making is_same_package_as() commutative should not change the behavior of the install command (except for the desired fix when overwriting a direct origin dependency by a PyPI dependency). No need for a downstream fix.

Thanks for looking into this! Given your investigation, I am pretty comfortable merging this, especially as we want to get a beta out. 🎉

[abn]

@radoering thanks again for digging into this. Just voicing that I am in agreement with @neersighted here. We can deal with any fall out in the lead upto poetry 1.2.0rc1. We definitely need to add a bit more test coverage I feel around this area. Additionally, we should either add parts of your digging into either a README.md in the puzzle module or witin the code itself.

[radoering]

Code documentation may be enough because the situation simplifies with the fix in this PR.

See python-poetry/poetry#5671