Fix: regression in dependent layer reload for WFS/OAPIF providers#64842
Fix: regression in dependent layer reload for WFS/OAPIF providers#64842ludotosk wants to merge 7 commits intoqgis:masterfrom
Conversation
src/core/vector/qgsvectorlayer.cpp
Outdated
There was a problem hiding this comment.
WFS and OAPIF are not the only providers to implement reloadProviderData().
I'm wondering if emitDataChanged() shouldn't call reload() itself instead of connecting reload to afterCommitChanges, since not emitting dataChanged() is probably undesirable.
That said, I'm not a specialist of that area of the code. Perhaps @Djedouas @troopa81 who have been involved in #58528 can have a second look
There was a problem hiding this comment.
I forgot to mention in the issue description that I use for work only the WFS, so I managed to have a working version for the WFS but I wasn't able to test the solution with other providers.
|
I tried to recollect my memories about all this, sorry if I failed 😄 IIRC, "dependencies" were set to call dataChanged() signal so we could detect when it's needed to rebuild index cache for dependent layer B when layer A was modified in QGIS (because a database trigger was set to update B when A was modified for instance). But we assume that, since A was modified in QGIS, so a repaint was triggered, and so, data were read again from all layers provider, so no need to explicity call to reloadData()
Could you describe your use case so I better understand why you need to explicitly call reloadData() ? Is it because there is some cache involved in the WFS provider and you need to invalid it ? |
Just a note about that: WFS provider does actually have a local disk cache of data stored in a sqlite file. |
I have a layer that represents a pipeline and another one that graphically shows the pipeline’s footprint. When I update or insert a pipeline, there is a trigger in Postgres that creates the footprint feature. Up to QGIS 3.40, which used reload, the footprint appeared automatically; starting from 3.44 it no longer appears unless I press F5.
Yes, without deleting the entire cache the dependency does not work. Unless I do F5 forcing a reload, but F5 would work even without the dependency making the dependency feature useless. |
|
@ludotosk OK thank you for taking the time to explain. I'm wondering if we should not addi a VectorProviderCapability |
I don't think that it is neither necessary nor advisable: the provider itself should (privately) take care of its own internal caching logic. IMHO there is no need to bubble up this logic to the client code. |
In that use case, provider has no way to know the cache need to be invalidate. It's because user has set the layer as a depency from another layer that cache need to be invalidate. So we need to force a reloadData, either like @ludotosk has proposed by having a special if condition for WFS/OAPIFS (and any other potential provider using cache), or either using a new capabiity flag. I don't see any other third better way. |
Well, in that case a capability flag is probably preferable over an hardcoded conditional. |
|
@nyalldawson I was checking my PRs and I notice that this PR was labled as possible AI. I'm sorry if my bad english, or anything else lead you to think that I'm a bot. But I'm just trying to figure out how to migrate a workflow about waste water management that we previously had on ESRI to QGIS using WFS as a provider. While doing this I encountered some bugs and I open this PR and also #64633 and #64617. They are all realated about WFS issues that I'm having, and they all have in common the fact that are real bugs not some weird hypotetical issue found on the source code. I'm sorry that you mantainers have to deal with a lot of AI slop and have to make the QEP 408 to deal with this issue. But this is a real bug that can be triggered by anyone in from the gui which a bot can't even use. So I want kindly ask if is it possible to label it as bug and not ai bot. Kind regards, Ludovico. |
|
@ludotosk no offence intended there -- that's why the label is "possibly", not a definite statement 😆 Unfortunately the reality of open source development in 2026 means that we'll be forced to take a skeptical stance toward any new contributor (especially in the wake of https://theshamblog.com/an-ai-agent-published-a-hit-piece-on-me/ |
|
Hi, following @troopa81 I have made a commit that add CacheData as flag in VectorProviderCapability and added the new flag in getCapabilities in the wfs and oapif provider. Let me know if this changes make sense, or if you were thinking about something different. I did the bare minimum to have working prototype to test. |
🍎 MacOS Qt6 buildsDownload MacOS Qt6 builds of this PR for testing. 🪟 Windows Qt6 buildsDownload Windows Qt6 builds of this PR for testing. |
|
The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check
|
|
You need to rebase and resolve conflicts with master |
46fba9d to
6d90418
Compare
getcabalities on wfs and oapif provider to add cachedata flag
6d90418 to
b530d74
Compare
|
I rebased the branch and resolved the conflict, which only involved two autogenerated files. The failing tests do not seem to be related to the rebase; in fact, I can see that master was already failing those tests. |
It's not related, let's restart the CI see if it's ok now |
|
There is actually a tests failing because of this modification which require to be fixed in test_provider_wfs.py
|
|
Fixed! I'm sorry if I missed these tests before but I didn't saw them between the unrelated tests, anyway even though the new capability is marked as new from QGIS 4.2 this bug fix can be backported to the current LTR? |
Description
Since QGIS 3.44,
QgsVectorLayer::setDependenciesno longer works correctly with the WFS provider. This regression was introduced by PR #58528, which changed the dependent layer behavior when the source layer emitsafterCommitChanges.Previously (up to 3.40), dependent layers performed a
reload()on this signal. PR #58528 changed this toemitDataChanged()to avoid side effects discussed in that PR. However, WFS requiresreload()to actually refresh data.This fix adds a conditional: WFS and OAPIF providers restore the
reload()behavior, while other providers keep usingemitDataChanged().