Skip to content
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

It is possible to store text into a numeric field, while it shouldn't be, using the Date/Time edit widget #36715

Closed
agiudiceandrea opened this issue May 25, 2020 · 5 comments · Fixed by #37039
Assignees
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter!

Comments

@agiudiceandrea
Copy link
Contributor

agiudiceandrea commented May 25, 2020

Describe the bug
It seems it is possible to store text into a numeric field, at least for memory layers, while I think it shouldn't be.

How to Reproduce

  1. create a New scratch layer (whatever geometry type), open the attribute table and add a numeric field (e.g. type: Whole number (integer), length: 10)
  2. close the attribute table, open the layer properties Attributes Form and change the field edit widget to Date/Time
  3. add a feature
  4. open the attribute table and click into the field to edit it, click the little down arrow to open the QCalendar, select a date and press enter

then

  1. stop editing and save the change: as you can see, a date e.g. 2020-05-25 is stored into the field

  2. open the field calculator and enter the filed name as an expression: as you can see, the output preview shows '2020-05-25' (its enclosed in single quotes, so it's a text string)

  3. in the python console:

l = iface.activeLayer()
field = l.fields()[0]
field.typeName()

outputs 'integer'

f = l.getFeature(1)
f.attribute(0)

outputs '2020-05-25'

type(f.attribute(0))

outputs <class 'str'>

QGIS and OS versions

Tested with QGIS

  • 3.10.1 (standalone installer) on Windows 7
  • 3.10.6 (standalone installer) on Windows 10
  • 3.12.3 (standalone installer) on Windows 10
  • 3.13.0-master (c2715d7) on Windows 10
@elpaso
Copy link
Contributor

elpaso commented May 28, 2020

I guess this is a particular behavior of the memory provider because it stores everything in memory as QVariant that can contain almost anything. I doubt it will happen with other providers, even if some of them are very tolerant about types.

What do you think should be the expected behavior in case conversion to the field type cannot be successfully performed for the memory provider?

@agiudiceandrea
Copy link
Contributor Author

agiudiceandrea commented May 28, 2020

I think it should not allow to store text strings in a numeric field (like it isn't allowed to store a value > MAX_INT32 into an INT32 field).

It seems this could lead to loss of data:
in the above example, if you "make permanent" (or save) the memory layer (with a numeric field which contains a text string) either to shapefile or geopackage, you will silently lose the data stored in that field.

I've found that it is also possible to store text strings in a numeric field of a memory layer using the "Value Map" widget.

@elpaso elpaso self-assigned this Jun 5, 2020
@elpaso
Copy link
Contributor

elpaso commented Jun 8, 2020

@agiudiceandrea I've started working on this issue. The big problem is that every provider does a different thing regarding wrong attribute type, when you try to store a string into a numeric field:

  • OGR (GPKG), no errors and no warning, the string is converted to a 0 !!
  • Postgis, error when committing to the back end

I think that Postgis is doing right here, just silently fail and convert a string to 0 does not seem the right thing to do.

@elpaso
Copy link
Contributor

elpaso commented Jun 8, 2020

Looks like patching the data provider to refuse commits on incompatible attribute types is not enough: the vector file writer just ignores any error.

@agiudiceandrea
Copy link
Contributor Author

Thank you for looking at this!

I think maybe it could be less problematic just modifying the widgets so that they aren't available for certain types of fields or that they behave accordingly to the type of field.

For example, the Date/Time widget could store a string and a date/time respectively into a string and a date/time field, like now, and julian day or millisecond in numeric field and should be unavailable for e.g. a boolean field.
The "Value Map" widget should follow the rules of the "Text Edit" widget, preventing to enter a non numeric string into the value column if the underlying field is numeric.

elpaso added a commit to elpaso/QGIS that referenced this issue Jun 17, 2020
Fixes qgis#36715

Adds a method to check for QVariant conversions, also
check for integral type narrowing so that for example
floating point 123.45 does not get down casted to integer
without raising an error.
nyalldawson pushed a commit that referenced this issue Jun 19, 2020
Fixes #36715

Adds a method to check for QVariant conversions, also
check for integral type narrowing so that for example
floating point 123.45 does not get down casted to integer
without raising an error.
espinafre pushed a commit to espinafre/QGIS that referenced this issue Jun 19, 2020
Fixes qgis#36715

Adds a method to check for QVariant conversions, also
check for integral type narrowing so that for example
floating point 123.45 does not get down casted to integer
without raising an error.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Either a bug report, or a bug fix. Let's hope for the latter!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants