Skip to content

Fix substring matching on cpan and mlflow type checks#232

Open
arpitjain099 wants to merge 2 commits into
package-url:mainfrom
arpitjain099:fix/substring-type-match
Open

Fix substring matching on cpan and mlflow type checks#232
arpitjain099 wants to merge 2 commits into
package-url:mainfrom
arpitjain099:fix/substring-type-match

Conversation

@arpitjain099

Copy link
Copy Markdown

Both the cpan and the mlflow type checks are missing a trailing comma:

if ptype and ptype in ("cpan"):
if ptype and ptype in ("mlflow"):

("cpan") is just the string "cpan", not a one-element tuple, so in runs a substring test instead of an equality test. Any type that happens to be a substring of the literal takes the branch.

On current main:

>>> PackageURL(type="pan", namespace="Some-Namespace", name="x").to_string()
'pkg:pan/SOME-NAMESPACE/x'

The namespace gets uppercased because "pan" in "cpan" is true. Same for "c", "cpa", "an" and so on. The mlflow one is the same shape, and it returns early, so a name normalized under type "flow" goes down the mlflow path and gets lowercased when a repository_url databricks qualifier is set.

No currently registered purl type is a substring of either literal, so this isn't biting a spec type today. It is reachable right now though, since arbitrary types are accepted, and it silently corrupts the namespace or name rather than erroring.

Switched both to ==, which is what the pypi, hackage and pub checks a few lines down already use. I also dropped the now-redundant ptype and guard since == "cpan" is already false for None and "".

I grepped the rest of src/ for the same in ("...") single-string shape and these two are the only instances.

Tests: added two cases to tests/test_packageurl.py covering both. Reverting the source change and keeping the tests fails them:

>           assert normalize_namespace("Some-Namespace", ptype) == "Some-Namespace"
E           AssertionError: assert 'SOME-NAMESPACE' == 'Some-Namespace'

>           assert normalize_name("MyModel", databricks, ptype) == "MyModel"
E           AssertionError: assert 'mymodel' == 'MyModel'

2 failed, 47 deselected

With the fix, pytest tests is 987 passed. black --line-length 100, isort --profile black and mypy are clean.

One note on overlap: #188 adds PURL_TYPES and touches this file, but it only adds a constant near the top and doesn't change these two lines, so it shouldn't conflict.

The cpan and mlflow branches used `ptype in ("cpan")` and
`ptype in ("mlflow")`. The missing trailing comma makes each a plain
string rather than a one-element tuple, so `in` is a substring test
instead of an equality test.

Any type that is a substring of the literal took the branch. For
example pkg:pan/Some-Namespace/x had its namespace uppercased to
SOME-NAMESPACE, and a name normalized under type "flow" took the
mlflow path and was lowercased when a databricks repository_url
qualifier was present.

Use == to match the neighbouring pypi, hackage and pub checks.

Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
Signed-off-by: Arpit Jain <arpitjain099@gmail.com>
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

Successfully merging this pull request may close these issues.

1 participant