Fix NULL support in WFS server and client #8958
Conversation
| @@ -584,7 +584,7 @@ QVariant QgsSpatiaLiteFeatureIterator::getFeatureAttribute( sqlite3_stmt *stmt, | |||
| } | |||
|
|
|||
| // assuming NULL | |||
| return QVariant( type ); | |||
| return QVariant( ); | |||
There was a problem hiding this comment.
Is that required? Won't that create an invalid QVariant instead of a Null QVariant?
I know we often use them interchangeably, but I was always hoping that in the long run we'll get a clean coherent usage for them. That would open up some nice possibilities for "we know the attribute is NULL" vs. "the attribute was not requested / not yet initialized (still needs default values)" and similar.
There was a problem hiding this comment.
Not that it makes much sense (all passing here on 5.11.2):
QVERIFY( QVariant().isNull() );
QVERIFY( !QVariant().isValid() );
QCOMPARE(QVariant().toString(), QString( "" ) );
QVERIFY( QVariant(QVariant::LongLong).isNull() );
QVERIFY( QVariant(QVariant::LongLong).isValid() );
QCOMPARE(QVariant(QVariant::LongLong).toString(), QString( "0" ) );Btw, this was the main source of the bug because there are a lot of checks for the QVariant type == field.type() and a conversion to the field type passing through the string representation if the type does not match.
This one-liner change costed me an afternoon (mostly spent on writing tests).
There was a problem hiding this comment.
Btw, this was the main source of the bug because there are a lot of checks for the QVariant type == field.type() and a conversion to the field type passing through the string representation if the type does not match.
Isn't this the real problem? There should rather be an isNull() check before these. NULL QVariants are produced in many places throughout our codebase and should be accepted as such.
There was a problem hiding this comment.
I'll have a second look, is this the semantics?
- isNull() when the value has been set to NULL
- !isValid() when we the value has not been set
There was a problem hiding this comment.
@m-kuhn ok, I've found another solution that respects the null semantic, with all test cases in place it was much easier now!
There was a problem hiding this comment.
Good job @elpaso !
Thanks for the tests and the fix.
|
Nice one! |
This is part of a bigger fix to support NULL values in WFS client and server components.
- expose nillable in describefeaturetype - serve xsi:nil="true" in getfeature - check for NULL in transactions and report an error Fixes qgis#20961 - plus some other unreported
No functional changes intended
This was nasty and only reproduceable when features were cached (so, the second time you get them) and was due to a QVariant( type ) default constructor for numeric values being initialized to 0, yielding 0 instead of NULL. Reported in qgis#20961
elpaso
force-pushed
the
bugfix-20961-wfs-null-transactions
branch
from
d912ca3
to
33788c9
Compare
This is the right behavior, btw there are other issues in the server component that ignores the main bool setting WMSServiceCapabilities completely.
…ctions Fix NULL support in WFS server and client Cherry-picked from master 861a8b7
Fix NULL support in WFS server and client Cherry-picked from master 861a8b7
This PR contains a set of related fixes for NULL WFS support:
This was nasty and only reproduceable when features
were cached (the second time you fetch them) and
was due to a QVariant( type ) default constructor
for numeric values being initialized to 0, yielding
0 instead of NULL.
Reported in #20961
With tests for all fixes (and more)