Doesn't this means someone broke decodeUri for the provider? (My suspicion is #62446)

🪟 Windows Qt6 builds

Download Windows Qt6 builds of this PR for testing.
(Built from commit e72f011)

🍎 MacOS Qt6 builds

Download MacOS Qt6 builds of this PR for testing.
This app is not notarized, run sudo xattr -d com.apple.quarantine /Applications/QGIS*.app to avoid the warning
(Built from commit e72f011)

Doesn't this means someone broke decodeUri for the provider? (My suspicion is #62446)

Yes, I think so. After discussing with @benoitdm-oslandia , my understanding of the situation is the following.

We store url and format fully encoded in the layer source (returned by source() and publicSource() ). That allow us to deal for instance with url containing & character without mixing it with & separating the different part of the source uri.

BUT we don't fully decode params when calling decodeUri(), and we should fix it by decoding them (here for instance for WMS url ). That would mean modifying all providers...

@gacarrillor Your Pull request seems ok but IMHO it would be better to fix the root cause than dealing with the consequences because it could break plugins. That would be a more resilient way to fix, don't you think?

@benoitdm-oslandia Do you confirm ? Can you propose a fix ?

@troopa81, answers below:

We store url and format fully encoded in the layer source (returned by source() and publicSource() ). That allow us to deal for instance with url containing & character without mixing it with & separating the different part of the source uri.

We talked about it with @benoitdm-oslandia. This may be considered already as an API break, since 3rd party plugins could be relying on layer.source()/layer.publicSource() to get and handle URIs. I'm not saying this is a good practice for devs, but we should be aware that changing this will have an impact.

BUT we don't fully decode params when calling decodeUri(), and we should fix it by decoding them (here for instance for WMS url ). That would mean modifying all providers...

Correct. We talked with @benoitdm-oslandia that the QgsProviderMetadata::decodeUri() method (see docs) is currently only promising to split a URI into its parts, and does not talk about returning each part in an encoded/decoded form. This could be improved in the docs.

However, changing the current behavior of decodeUri(), regardless of the docs ambiguity, would also be an API break, which is the current situation in master.

@gacarrillor Your Pull request seems ok but IMHO it would be better to fix the root cause than dealing with the consequences because it could break plugins. That would be a more resilient way to fix, don't you think?

I see no issues with the proposed fix in this PR, given that this is for a specific functionality (enhancing 2 processing algs.) that I added for QGIS 4.0. I proposed this fix so that our client could have their functionality working for QGIS 4.0, regardless of the status of source()/publicSource()/decodeUri(), which was uncertain less than 2 days before the scheduled 4.0 release, by the time this PR was created.

Moreover, using QgsDataSourceUri() is another way of accessing connection parameters, suggested (informally) e.g., here, and on which we can fully rely because it will decode the parameters if we pass an encoded URI.

On the other hand, avoiding the API break would be the most important challenge, and out of scope for me at the moment.

@gacarrillor does #65114 fix the issue?

We talked about it with @benoitdm-oslandia. This may be considered already as an API break, since 3rd party plugins could be relying on layer.source()/layer.publicSource() to get and handle URIs.

That's the first question that we need to answer. Do we consider encoding each part in the URI an API break. Meaning that an URI was

before

crs=CRS:84&dpiMode=7&featureCount=10&format=image/png&layers=dachseiten&styles&tilePixelRatio=0&url=https://geodienste.hamburg.de/HH_WMS_Solaratlas?VERSION=1.3.0

after

crs=CRS:84&dpiMode=7&featureCount=10&format=image%2Fpng&layers=dachseiten&styles&tilePixelRatio=0&url=https%3A%2F%2Fgeodienste.hamburg.de%2FHH_WMS_Solaratlas%3FVERSION%3D1.3.0

If we consider this is an API break, I propose to revert the fix instead of dealing of the aftermath of the modification. I'd say no from my point of view because the URI is more valid than before, but I can understand one think otherwise. It could break plugins that read URI elements without decoding them. Any opinion @nyalldawson @benoitdm-oslandia ?

However, changing the current behavior of decodeUri(), regardless of the docs ambiguity, would also be an API break, which is the current situation in master.

If we fix it, like partially proposed in #65114, it wouldn't be an API break. It would be consistent with the method name that returned decoded elements.

I see no issues with the proposed fix in this PR, given that this is for a specific functionality (enhancing 2 processing algs.) that I added for QGIS 4.0. I proposed this fix so that our client could have their functionality working for QGIS 4.0, regardless of the status of source()/publicSource()/decodeUri(), which was uncertain less than 2 days before the scheduled 4.0 release, by the time this PR was created.

Agreed. The PR seems good to me. But If we revert the initial PR or fix the decodeUri method, this wouldn't be needed.

@gacarrillor does #65114 fix the issue?

Yes, but there may be other encoded parameter (url, format) to deal with in the other providers.

@gacarrillor does #65114 fix the issue?

Yes, it does, thanks.

Agreed. The PR seems good to me. But If we revert the initial PR or fix the decodeUri method, this wouldn't be needed.

Of course, this PR was only created in reaction to the API break.

My 2c:

That's the first question that we need to answer. Do we consider encoding each part in the URI an API break.

I don't think this is an API stability issue. It probably WILL affect plugins, but we never made any statement about those URIs being in a stable format (and we break other manual project deserialisation code on a near-daily basis 😆 )

I DO think that breaking decodeUri is a regression that needs fixing. That API was designed to be the canonical way of getting human readable/usable values out of a layer's URL.

If we consider this is an API break, I propose to revert the fix

I'm -1 to reverting the fix. After looking into this, I understand the importance of the fix and the severity of the underlying issue it was designed to address. I think 4.0 is the PERFECT time to test-run a fix like this... there's going to be so many other stability issues in 4.0.x that I think a bit of short-term pain around URI handling is acceptable at this stage in the 4.x lifecycle. And I don't think we'll get that chance again till 5.0!

So my question is -- if the original fix is OK and stays, then what's the best way to fix the decodeUri regression? If it's the approach from #65114, then I'll port that over to the other providers and soak in unit tests....

@nyalldawson

So my question is -- if the original fix is OK and stays, then what's the best way to fix the decodeUri regression? If it's the approach from #65114, then I'll port that over to the other providers and soak in unit tests....

Hi! IMPOV #65114 is is the only good approach even if it means to port theses changes to all other providers. Do you need some help?

@benoitdm-oslandia i've updated #65114 now so that the tests reflect the escaped values from the encodeUri call.

I had a look, and can't immediately see any other providers which would require similar updates. Most rely on QgsDataSourceUri directly and have already been handled in the original fix PR.

Superseded by #65114