refactor(pg): store nil map as empty {} #6929

janisz · 2023-07-13T17:07:38Z

Description

Currently we store nil map as 'null' (string). In next version of pgx the semantic has changed and it will be stored as null.
This PR seal this behaviour and whenever we would like to store nil map it will be replaced with {} (empty but not nil map).

TestIndex/TestMapHighlights/key_does_not_exist shows the difference between having 'null' and null in JSON field and null always returns false so negative queries will not work correctly.

Checklist

Investigated and inspected CI test results
Unit test and regression tests added
Evaluated and added CHANGELOG entry if required
Determined and documented upgrade steps
Documented user facing changes (create PR based on openshift/openshift-docs and merge into rhacs-docs)

If any of these don't apply, please comment below.

Testing Performed

CI

janisz · 2023-07-13T17:07:51Z

Current dependencies on/for this PR:

master
- PR test(pg): add test for nil map serialisation #6937
  - PR test(pg): log queries in tests #6938
  - PR refactor(pg): store nil map as empty {} #6929 👈
    - PR refactor(pg): extract input values template #7108
      - PR refactor(pg): remove unused context arg #7051
    - PR WIP: chore(deps): upgrade pgx to v5 #6915
      - PR chore(deps): upgrade pgx to v5 #6916

This comment was auto-generated by Graphite.

roxbot · 2023-07-13T17:25:58Z

Images are ready for the commit at 7d1d2f6.

To use with deploy scripts, first export MAIN_IMAGE_TAG=4.1.x-525-g7d1d2f6084.

connorgorman · 2023-07-13T18:07:18Z

What about all the preexisting entries in the DB using the old format?

janisz · 2023-07-13T18:14:36Z

What about all the pre existing entries in the DB using the old format?

I think they are fine. Queries will be the same a before and will work, when upsert will happen we will replace them with {}. I thought about using v4 approach and store 'null' but this doesn't make much sense.

connorgorman · 2023-07-13T19:33:25Z

If we change all the values to {}, will pgx v4 still work the same way? We need to ensure any changes are completely backwards compatible

janisz · 2023-07-13T19:57:50Z

That's tricky. If we want be 100% backward compatible then we should store 'null'.

The only incompatibility I could think of is a query that checks exact value or if value is a string and not map. I think our code does not allow such queries but I'm not an expert in this area. @md2119 do we expose any other queries than tested for maps?

dashrews78 · 2023-07-13T20:25:37Z

That's tricky. If we want be 100% backward compatible then we should store 'null'.

The only incompatibility I could think of is a query that checks exact value or if value is a string and not map. I think our code does not allow such queries but I'm not an expert in this area. @md2119 do we expose any other queries than tested for maps?

We pretty much have to be backwards compatible in the event of a rollback from 4.2 to 4.1. Greatly increases the amount of testing required to verify that we can make this change.

janisz · 2023-07-14T11:35:38Z

@connorgorman @dashrews78 I added test for this behaviour and changed from {} to 'null'

test(pg): add test for nil map serialisation #6937

janisz · 2023-07-17T11:11:45Z

/retest

rhybrillou

please ensure value array construction consistency across insertInto and copyFrom

rhybrillou · 2023-07-25T08:21:31Z

tools/generate-helpers/pg-table-bindings/store.go.tpl

@@ -205,6 +205,8 @@ func {{ template "insertFunctionName" $schema }}({{ if eq (len $schema.Children)
        pgutils.NilOrTime({{$field.Getter "obj"}}),
        {{- else if eq $field.SQLType "uuid" }}
        pgutils.NilOrUUID({{$field.Getter "obj"}}),
+        {{- else if eq $field.DataType "map" }}
+        pgutils.EmptyOrMap({{$field.Getter "obj"}}),


I think the copyFrom template needs this change too (at least to be future-proof).
Maybe the {{- range $field := $schema.DBColumnFields -}} ... {{- end }} pattern could be extracted into another template.

oh, good point. I'll try to add a test for it too

refactor(pg): extract input values template #7108

janisz · 2023-07-25T13:33:46Z

/retest

janisz · 2023-07-26T05:21:00Z

/retest

janisz · 2023-07-26T09:15:18Z

@janisz started a stack merge that includes this pull request via Graphite.

janisz · 2023-07-26T09:15:36Z

@janisz merged this pull request with Graphite.

## Description Fixes: - #6929 (comment) ## Checklist - [ ] Investigated and inspected CI test results - [ ] Unit test and regression tests added - [ ] Evaluated and added CHANGELOG entry if required - [ ] Determined and documented upgrade steps - [ ] Documented user facing changes (create PR based on [openshift/openshift-docs](https://github.com/openshift/openshift-docs) and merge into [rhacs-docs](https://github.com/openshift/openshift-docs/tree/rhacs-docs)) If any of these don't apply, please comment below. ## Testing Performed CI

janisz requested a review from a team as a code owner July 13, 2023 17:07

github-actions bot added area/central area/postgres labels Jul 13, 2023

This was referenced Jul 13, 2023

WIP: chore(deps): upgrade pgx to v5 #6915

Closed

chore(deps): upgrade pgx to v5 #6916

Merged

janisz requested a review from a team July 13, 2023 17:13

janisz changed the title ~~refactor(pg): store empty map as {} not null~~ refactor(pg): store nil map as empty; not null Jul 13, 2023

janisz requested a review from connorgorman July 13, 2023 17:26

janisz force-pushed the master-janisz/07-13-refactor_pg_store_empty_map_as_not_null branch from 26a47a2 to bea289d Compare July 14, 2023 11:32

janisz changed the base branch from master to master-janisz/07-14-test_pg_add_test_for_nil_map_serialisation July 14, 2023 11:32

janisz mentioned this pull request Jul 14, 2023

test(pg): add test for nil map serialisation #6937

Merged

5 tasks

janisz changed the title ~~refactor(pg): store nil map as empty; not null~~ refactor(pg): store nil map as 'null' Jul 14, 2023

janisz force-pushed the master-janisz/07-14-test_pg_add_test_for_nil_map_serialisation branch from 2ad6341 to 8f85554 Compare July 14, 2023 12:18

janisz force-pushed the master-janisz/07-13-refactor_pg_store_empty_map_as_not_null branch from bea289d to da0fb0b Compare July 14, 2023 12:19

janisz mentioned this pull request Jul 14, 2023

test(pg): log queries in tests #6938

Closed

5 tasks

janisz requested a review from dashrews78 July 18, 2023 10:50

janisz force-pushed the master-janisz/07-14-test_pg_add_test_for_nil_map_serialisation branch from 8f85554 to e6bb503 Compare July 20, 2023 11:16

Base automatically changed from master-janisz/07-14-test_pg_add_test_for_nil_map_serialisation to master July 24, 2023 11:05

janisz changed the title ~~refactor(pg): store nil map as 'null'~~ refactor(pg): store nil map as empty {} Jul 24, 2023

janisz force-pushed the master-janisz/07-13-refactor_pg_store_empty_map_as_not_null branch from da0fb0b to baad7ef Compare July 24, 2023 16:20

connorgorman approved these changes Jul 24, 2023

View reviewed changes

rhybrillou requested changes Jul 25, 2023

View reviewed changes

janisz force-pushed the master-janisz/07-13-refactor_pg_store_empty_map_as_not_null branch from baad7ef to f688304 Compare July 25, 2023 10:45

janisz requested a review from rhybrillou July 25, 2023 10:45

janisz mentioned this pull request Jul 25, 2023

refactor(pg): extract input values template #7108

Merged

5 tasks

janisz force-pushed the master-janisz/07-13-refactor_pg_store_empty_map_as_not_null branch from f688304 to 7ccf79d Compare July 25, 2023 16:19

rhybrillou approved these changes Jul 25, 2023

View reviewed changes

janisz mentioned this pull request Jul 25, 2023

refactor(pg): remove unused context arg #7051

Merged

1 task

refactor(pg): store empty map as {} not null

7d1d2f6

janisz force-pushed the master-janisz/07-13-refactor_pg_store_empty_map_as_not_null branch from 7ccf79d to 7d1d2f6 Compare July 25, 2023 18:27

janisz merged commit 42f13d8 into master Jul 26, 2023
67 checks passed

janisz deleted the master-janisz/07-13-refactor_pg_store_empty_map_as_not_null branch July 26, 2023 09:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(pg): store nil map as empty {} #6929

refactor(pg): store nil map as empty {} #6929

janisz commented Jul 13, 2023 •

edited

janisz commented Jul 13, 2023 •

edited

roxbot commented Jul 13, 2023 •

edited

connorgorman commented Jul 13, 2023

janisz commented Jul 13, 2023

connorgorman commented Jul 13, 2023

janisz commented Jul 13, 2023

dashrews78 commented Jul 13, 2023

janisz commented Jul 14, 2023 •

edited

janisz commented Jul 17, 2023

rhybrillou left a comment

rhybrillou Jul 25, 2023

janisz Jul 25, 2023

janisz Jul 25, 2023

janisz commented Jul 25, 2023

janisz commented Jul 26, 2023

janisz commented Jul 26, 2023

janisz commented Jul 26, 2023

refactor(pg): store nil map as empty {} #6929

refactor(pg): store nil map as empty {} #6929

Conversation

janisz commented Jul 13, 2023 • edited

Description

Checklist

Testing Performed

janisz commented Jul 13, 2023 • edited

roxbot commented Jul 13, 2023 • edited

connorgorman commented Jul 13, 2023

janisz commented Jul 13, 2023

connorgorman commented Jul 13, 2023

janisz commented Jul 13, 2023

dashrews78 commented Jul 13, 2023

janisz commented Jul 14, 2023 • edited

janisz commented Jul 17, 2023

rhybrillou left a comment

Choose a reason for hiding this comment

rhybrillou Jul 25, 2023

Choose a reason for hiding this comment

janisz Jul 25, 2023

Choose a reason for hiding this comment

janisz Jul 25, 2023

Choose a reason for hiding this comment

janisz commented Jul 25, 2023

janisz commented Jul 26, 2023

janisz commented Jul 26, 2023

janisz commented Jul 26, 2023

janisz commented Jul 13, 2023 •

edited

janisz commented Jul 13, 2023 •

edited

roxbot commented Jul 13, 2023 •

edited

janisz commented Jul 14, 2023 •

edited