Property values are not URL quoted for matrix parameters

[briantist]

This only affects matrix parameters as far as I can tell; we should not quote parameters that are set after the fact because those go through a POST.

Because property values aren't encoded/quoted, it can cause undesirable characters to make it into properties, or cause the properties to be truncated. For example if your value contains a question mark ?, it seems to get stripped somewhere; this character plus any characters after it do not end up in the property value.

If your value contains % followed by any hex value, the resulting character with that code will be inserted into the property value instead of the literal, for example if your value contains %0 you'll end up with a null character in the value.

Example repros (can be done with the stock Artifactory OSS container):

from artifactory import ArtifactoryPath

with open('/tmp/delme', 'w'):
    pass

a = Artifactorypath('http://localhost:8082/artifactory/example-repo-local', auth=('admin', 'password))

props = {
    'test1': 'a?b',
    'test2': 'a%3Fb',
    'test3': 'a%0b',
}

a.deploy_file('/tmp/delme', parameters=props)

---

I believe this should be fixable with urllib3.quote being applied to each version.

If this is an acceptable fix, I might be able to put up a PR for it.

If the position of the library is that callers are responsible for encoding their property values, then we must also make that explicit (because if callers quote their values now, and later the library adds quoting, it will double encode and ruin the values).

For the same reason, if we do opt to do quoting in the library, we should call it out as a breaking change.

[allburov]

If this is an acceptable fix, I might be able to put up a PR for it.

It'd be great!

it will double encode and ruin the values

I'm not sure how to detect this state... If we quote properties - it may broke the current usage. Is there a way to detect that properties have been quoted already?

What do you think about adding a parameter quote_parameters with the value False by default - and show all examples with quote_parameters=True in the documentation and later we can make it True by default in few releases?

[briantist]

@allburov

I'm not sure how to detect this state... If we quote properties - it may broke the current usage. Is there a way to detect that properties have been quoted already?

While we could guess, as humans, it ends up being impossible to determine this without knowing intent, because we don't know if the caller intended to literally store an encoded value like a%2Fb or if they intended to pre-encode it, unless....

What do you think about adding a parameter quote_parameters with the value False by default

I was also going to suggest that approach! It's something I often use in other projects to make breaking changes over time:

1. Put new behavior behind a parameter
2. Parameter defaults to current behavior
3. Default value changes to the desired behavior in the next major version (or the one after that)

Sometimes supplement 2) with a default value of None which will:

• End up defaulting to the current behavior
• But show a warning indicating that the default will change to <value> in version X.0.0
• In version X.0.0

This ensures people who don't chose a value will be notified, and removes the warning for anyone who makes an explicit choice (in either direction).

---

I took a look at the release cycle and it seems like there's releases basically only when necessary (not a defined/periodic release), so I don't think we'd have to worry too much about skipping versions for the breaking change.

In that case I will start a PR for the above and put out a warning that the default will change in v0.10.0, with the idea that this PR would trigger the release of v0.9.0. Does that sound ok?

[allburov]

It sounds amazing! :rocket: