-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Dataprovider ogr spatialite respect provider defaults #34012
Dataprovider ogr spatialite respect provider defaults #34012
Conversation
Fixes qgis#33383 (for OGR, spatialite commit follows)
Fixes qgis#33383 Homogenization is not complete but at least there are test cases for the future.
88f3b9b
to
f8e255e
Compare
@@ -1380,7 +1385,7 @@ QVariant QgsOgrProvider::defaultValue( int fieldId ) const | |||
|
|||
QString defaultVal = mDefaultValues.value( fieldId, QString() ); | |||
if ( defaultVal.isEmpty() ) | |||
return QVariant(); | |||
return defaultVal; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this change is correct -- there's lots of existing code which checks if the returned value isValid(), and now that will return true instead of false (yet the default value doesn't exist)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And wouldn't the check for mDefaultValues.hasKey
be correct here?
Not that an empty string as default value is common, but it's at least in SQL terms it's valid.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was an attempt to homogenize the behavior, I will revert it and give up with the attempt.
But if you have a look to the tests, you'll see that the actual behavior of the two methods it is totally different between providers (I tested PG ogr and spatialite, but I expect that the others will not behave better).
What they return depends on a permutation of:
- the fact that it is a literal default or not
- the fact that it is a particular function/magic-value or not (CURRENT_TIMESTAMP for instance)
- which provider (and maybe which driver for OGR)
- if EvaluateDefaultValues is honored and its value
The least we can do is to document it in the API and possibly try to fix it for QGIS 4.
... also fixes "Autogenerate" for PKs Fixes qgis#34085
Fixes #33383
There is also an initial attempt to homogenize defaultValue and defaultValueClause between providers.
See also: https://lists.osgeo.org/pipermail/qgis-developer/2020-January/060007.html