qgis / QGIS

QGIS is a free, open source, cross platform (lin/win/mac) geographical information system (GIS)
https://qgis.org
GNU General Public License v2.0
10.49k stars 2.99k forks source link

Oracle provider: crash on on request by fid #33430

Open tomass opened 4 years ago

tomass commented 4 years ago

WMS GetFeatureInfo request fails (QGIS Server crashes on Q_ASSERTION) when:

How to Reproduce

WMS GetFeatureInfo Request sample: SERVICE=WMS&VERSION=1.1.0&REQUEST=GetFeatureInfo&FORMAT=image%2Fpng&TRANSPARENT=true&QUERY_LAYERS=l_kzs&LAYERS=l_kzs&TILED=true&FILTER=l_kzs%3A%20 "METAI" %3D 2017 &INFO_FORMAT=application%2Fjson&X=494&Y=326&WIDTH=512&HEIGHT=512&SRS=EPSG%3A3346&STYLES=&BBOX=351450.2369578528%2C6209605.628514059%2C351721.1708330539%2C6209876.56238926 *here _lkzs is a vector layer from Oracle database.

Such request executed either via browser, either directly by launching _qgismapserv.fcgi ends in crash:

ASSERT: "pkVals.size() == primaryKeyAttrs.size()" in file /home/craftbeer/svn/QGIS/src/providers/oracle/qgsoracleprovider.cpp, line 426

QGIS and OS versions

QGIS 3.10 on Linux (Compiled from source)

Additional context

Error occurs in src/providers/oracle/qgsoracleprovider.cpp function QgsOracleUtils::whereClause

case PktFidMap:
{
  QVariant pkValsVariant = sharedData->lookupKey( featureId );
  if ( !pkValsVariant.isNull() )
  {
    QList<QVariant> pkVals = pkValsVariant.toList();

    if ( primaryKeyType == PktFidMap )
    {
      Q_ASSERT( pkVals.size() == primaryKeyAttrs.size() );

Here featureId has id of the feature requested passed correctly (f.e. 12345678). But through all processing via sharedData->lookupKey and toList() no elements end up in a list pkVals. That is pkVals.size() is 0.

And therefore Q_ASSERT fails as primarkyKeyAttrs.size() is 1 (vector layer source has one primary key column).

If right before Q_ASSERT I add such a workaround code:

if (pkVals.size() == 0) {
  pkVals.append(featureId);
}

then GetFeatureInfo is successful - returns correct json response.

gioman commented 4 years ago

If right before Q_ASSERT I add such a workaround code:

can you propose a pull request with a patch?

tomass commented 4 years ago

can you propose a pull request with a patch?

My code is just a workaround. It does not solve the root problem: featureId is not propagated to pkVals via the code there is now. And it will not help if more than one column is used as a primary key.

Correct fix would be to correct sharedData->lookupKey usage. But I do not know what is the function of sharedData->lookupKey (why is it there?). If somebody could help me with that, I could try creating a PR.

elpaso commented 4 years ago

@tomass I'm not very familiar with OCI provider, but I think that lookupKey takes a QGIS feature ID (fid) as an argument and return the value(s) of the keys, the provider keeps a mapping between QGIS feature IDs and the backend key values.

This does the opposite, taking key value(s) and returning the QGIS feature ID:

QgsFeatureId lookupFid( const QVariantList &v ); // lookup existing mapping or add a new one
jef-n commented 4 years ago

@elpaso correct. the mapping is not persistent. so the provider assigns running features id to new combinations of key values in the order it sees them and that map only lasts for the lifetime of the provider - ie. using the feature id from one layer on another will not work, neither will using feature ids across sessions. And there is no relation between the feature id and the actual key values - so the feature ids should never be put into the database. Question is where that empty variant list in the map came from.

elpaso commented 4 years ago

@jef-n a bit off-topic, but we really should find a way to use persistent fid/pk mappings in all providers or the server's transactional part will be totally unreliable.

jef-n commented 4 years ago

@jef-n a bit off-topic, but we really should find a way to use persistent fid/pk mappings in all providers or the server's transactional part will be totally unreliable.

I think that wfs provider already has something in that direction.

Pedro-Murteira commented 2 years ago

@tomass Hello, is this still true on more recent releases?

tomass commented 2 years ago

Sorry, I do not have access to newer version of QGIS Server connected to Oracle and have no possibility to get one, so I cannot check.

github-actions[bot] commented 2 years ago

The QGIS project highly values your report and would love to see it addressed. However, this issue has been left in feedback mode for the last 14 days and is being automatically marked as "stale". If you would like to continue with this issue, please provide any missing information or answer any open questions. If you could resolve the issue yourself meanwhile, please leave a note for future readers with the same problem and close the issue. In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this issue. If there is no further activity on this issue, it will be closed in a week.