Allows editing of postgres JSON fields from Text Edit Widget

[stev-0]

Fixes #29361

Description

This allows JSON to be entered in text format as well as with the Key/Value widget. I would value a review on the changes to qgspostgresconn.cpp as that was the only way I could prevent a JSON object becoming a JSON array. Also whether we should be checking for a generic QVariantMap as currently or narrow it down to JSON. Perhaps this might cause problems with some other providers?

Checklist

• Commit messages are descriptive and explain the rationale for changes
• Commits which fix bugs include fixes #11111 in the commit message next to the description
• Commits which add new features are tagged with [FEATURE] in the commit message
• Commits which change the UI or existing user workflows are tagged with [needs-docs] in the commit message and contain sufficient information in the commit message to be documented
• I have read the QGIS Coding Standards and this PR complies with them
• This PR passes all existing unit tests (test results will be reported by travis-ci after opening this PR)
• New unit tests have been added for core changes
• I have run the scripts/prepare-commit.sh script before each commit

[stev-0]

Should pass now. Never quite sure if I should squash commits or not..

[stev-0]

I think that this might also fix #30558, I got similar errors before (again) doing something similar to here. @signedav, maybe you could have a look?

[stale]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[stev-0]

Would be great to see this merged as it fixes minimum 2 bugs and possibly more. Anything else I can do to get it merged?

[signedav]

I cannot decide about the text edit widget implementation at the moment, but the fix avoiding QGIS from crash on storing to Postgres (= instead of {}) are quite important.

If there is no agreement to merge it, it would be great, when you could separate this fix from the implementation in the text widget.

[stale]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[stev-0]

Hi @signedav what aspects do you think changing / review? The concept as a whole, or a specific aspect of the implementation?

[nextstopsun]

Would very much like to see it land in 3.10 :)

[signedav]

Sorry for not replying earlier @stev-0

I like the approach. The implementation in the text edit widget looks IMO good. What I am uncertain is, if there is any danger by converting the data in case of type is QVariant::Map. As far as I know it's only used for the JSON content used for Keyvalue-, List- and ValueRelation widget. So it should be fine to convert it to JSON and back. But I would like to have other opinions here...

Btw. it would be nice to have tests for the widget itself. To read and write the text from and to a JSON field. And maybe you can test it with Geopackage JSON fields as well (not only Postgres) to be sure.

The part with the JSON value assignement is partly merged already with this: #31264
Except here https://github.com/qgis/QGIS/blob/master/src/providers/postgres/qgspostgresconn.cpp#L1113 - on purpose @m-kuhn ?

[signedav]

Hi Steve
Thanks a lot for your effort. I'm looking forward to merge this nice feature soon. But I found some issues.

First, on creating a new feature with geometry the warning message appears:

It's because it receives somewhere an empty string as value, what is not valid JSON. I didn't dig deeper there. Let me know if you need help.

Then there is something else:

• enter the widget with existing text in it
• select all and delete all (with the keyboard DEL)
• the widget is now empty, I cannot enter anything anymore
  Can you reproduce that?

And then I found a minor thing in the warning message.

[signedav]

I don't know if we have the behavior we want at the moment.
Since we do accept empty strings we don't get the warning message on the initialization.
But in every other case an empty string is accepted as valid JSON. And do we want that?
Shouldn't this NULL and "" be accepted but `` not?

If we don't accept the empty strings, we have to avoid the message on creating a feature with geometry otherwise...

[signedav]

I checked it again. It's like this:

• Entering empty string -> stored in PG as "" and displayed again like an empty string.
• Entering "" -> stored in PG as "" and displayed again like an empty string.
• Clearing field with <x Button displaying NULL (or representor) -> stored in PG as NULL and displayed again as NULL (or representer)

Fair enough for Postgres (haven't checked Geopackage).

But now the confusing part:

• Entering Hello -> not accepted - warning appears.
• Entereing "Hello" -> stored in PG as "Hello" and displayed again as Hello and warning appears that it's not accepted (still it's written in PG).
• Opening form where the field contains "Hello" it displays Hello and on edit or just leaving form the warning appears.

I suggest to display always the quotation marks on single strings. And on empty string we can decide if we want:

• not accept them
• quickly replace them (stored and displayed) as ""

While I think the second one would be much user friendlier.

[signedav]

And when I add a feature on the canvas with geometry, enter a invalid JSON and store it, it's not stored and there is no warning message appearing. That's not nice.

Sorry for coming back again and again, but it would be great to have a nice and solid feature here 🙂

[stale]

The QGIS project highly values your contribution and would love to see this work merged! Unfortunately this PR has not had any activity in the last 14 days and is being automatically marked as "stale". If you think this pull request should be merged, please check

• that all unit tests are passing

• that all comments by reviewers have been addressed

• that there is enough information for reviewers, in particular

  • link to any issues which this pull request fixes

  • add a description of workflows which this pull request fixes

  • add screenshots if applicable

• that you have written unit tests where possible
  In case you should have any uncertainty, please leave a comment and we will be happy to help you proceed with this pull request.
  If there is no further activity on this pull request, it will be closed in a week.

[nextstopsun]

Not stale. Fingers crossed for this to land in 3.12

[stev-0]

I think I have incorporated the main points from @signedav, which were definitely valid and well worth raising. The behaviour now is that primitive / raw JSON strings from the database are quoted in the widget, and only quoted strings are accepted backbas input. Blank strings "" are converted to blank JSON strings """". The behaviour of qgsattributedialog has also been fixed.

I think there are few other things that could be addressed in this, PR, but happy for it to go as it is, following review. In order of importance (I think):

• It would be good to change the default widget for JSON to Text, as this covers all possible values, not just maps / lists. I can't find out how to do this without changing the default type for JSON, which has much wider effects
• This doesn't work for Geopackage JSON subtype. I think this is due to different handling of Postgres and Geopackage, but the JSON functionality is at the form level. I have not seen many of these files in the wild, so I assume Postgres support is more important for the moment, but this should be looked into in the future
• I think longer term, a Scintilla widget would be the best input type for JSON, as it would allow immediate visibility of errors, but I consider this a longer term aim.

[stev-0]

@signedav this now passes and is good for a review now, which I hope covers everything!

[signedav]

Thanks @stev-0 I will review it in around week. I'm currently in vacation without good connection...

[signedav]

I checked the code and it looks good. Made some tests as well.
It would be really nice to have this for GPKG JSON as well in future. But since it's there just the status quo I can approve this PR.

Thanks @stev-0 for this nice work.

[stev-0]

Great, thanks @signedav

[nextstopsun]

Will it make it to 3.12?

[m-kuhn]

Hmm... Looks more like that has been forgotten. Or nobody assumed responsibility.

Thanks @stev-0 for the work and @signedav for the review.