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.43k stars 2.98k forks source link

range widget breaks serial/nextval column #36396

Open gioman opened 4 years ago

gioman commented 4 years ago

QGIS 3.10/12/master

take a postgresql table with a column defined as "serial", if the "range" widget is applied to this column then it stops to be automatically filled with the autoincrementing value.

Worked with no issues in 2.18.

lucamanga commented 4 years ago

As you can see, in QGIS 2.18 the ogc_fid field (serial type) is correctly set as "Text Edit", as in this image:

immagine

gioman commented 4 years ago

As you can see, in QGIS 2.18 the ogc_fid field (serial type) is correctly set as "Text Edit", as in this image:

@lucamanga I'm confused, are you saying that QGIS 3 applies automatically the range widget to such columns (when a layer is loaded in a project)?

lucamanga commented 4 years ago

the problem is that, after importing the layer from postgres (with the style saved in the DB):

With range, QGIS 3 cannot set the value as nextval(...)

gioman commented 4 years ago

the problem is that, after importing the layer from postgres (with the style saved in the DB):

* QGIS 2.18 threats the ogc_fid field as Text Edit, **correctly**

* QGIS 3 threats the ogc_fid field as Range, **incorrectly**

if your style is set to give the ogc_fid column a range widget then is normal that you get that... are you sure that your saved style is not set to give that column that widget?

lucamanga commented 4 years ago

OK. Removing the old style and re-saving the updated style with Text Edit, this fixes the bug. You can close it. Thank you.

gioman commented 4 years ago

Possibly it could make sense that giving (by mistake, I can't see any other reason) the range widget to a serial column should not break the autoincrement... anyway seems a very low priority issue.

m-kuhn commented 4 years ago

Alternatively, enabling "evaluate default values server side" should also help.

lucamanga commented 4 years ago

Alternatively, enabling "evaluate default values server side" should also help.

where is this setting?

m-kuhn commented 4 years ago

project properties -> data source

elpaso commented 4 years ago

I'm wondering if is there anything we should do here: maybe a smarter logic to select the text widget instead of the range one in case we have a default value + unique + not null constraints on a numeric field?

m-kuhn commented 4 years ago

I'd see the "fix" in the Range widget itself - and add the default value as special value, like NULL.

For a "deeper" fix, it would be good to send a special value (instead of a default value string) to the dataprovider. For example an invalid QVariant (not a null QVariant) but @nyalldawson will disagree with me on this implementation detail.

lucamanga commented 4 years ago

Note that QGIS 2.18 just works, without doing anything special. It is interpreting as Range value (correctly)

nyalldawson commented 4 years ago

Yeah - I'm still of the opinion that trying to keep a distinction between null and invalid variants is a headache we don't need (especially on the python side)...

If we wanted to have a "use default value" flag for attributes, we could consider making a little class "QgsDefaultProviderValue", register it as a meta type, and then use that whenever we want the default value used... (Instead of the string representation of the clause)..... Maybe?....

elpaso commented 4 years ago

@m-kuhn I think we should think it carefully: from a UX point of view the autoincrement value should probably be completely hidden and automatically handled from QGIS (with a little help from the back-end).

But I can hear our power users screaming "Oooohhh I really want to be able to override the default autoincrement value!".

If we want to show the field I'd probably prefer to see the explicit string representation of the autoincrement "nextval, whatever the provider uses" than a magic/special value (NULL, DEFAULT, or whatever we choose).

m-kuhn commented 4 years ago

My comment was completely on an API level. I don't mind still showing it to the user, it could be on a tooltip or other gui elements also.

Currently, to use a default value we will send the provider a string ('nextval(...)') and the provider will use this magic value and treat it as expression or do some other tricks. It works good enough, but a cleaner API would be to send a dedicated QVariant( DefaultValue ) instead of a magic value to the provider.

This does not prevent us from showing the string itself to the user and allowing him to replace it.

If we wanted to have a "use default value" flag for attributes, we could consider making a little class "QgsDefaultProviderValue", register it as a meta type, and then use that whenever we want the default value used... (Instead of the string representation of the clause)..... Maybe?....

An idea worth exploring.

elpaso commented 4 years ago

My comment was completely on an API level. I don't mind still showing it to the user, it could be on a tooltip or other gui elements also.

Currently, to use a default value we will send the provider a string ('nextval(...)') and the provider will use this magic value and treat it as expression or do some other tricks. It works good enough, but a cleaner API would be to send a dedicated QVariant( DefaultValue ) instead of a magic value to the provider.

I like that!

Pedro-Murteira commented 2 years ago

This issue is still valid on QGIS 3.22.3. And as mentioned in the previous comments on this issue, Project Properties > Data Sources > "Evaluate default values on provider side" seems to fix the autoincrement issue.