refactor(pg): store nil map as empty {} by janisz · Pull Request #6929 · stackrox/stackrox · GitHub

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

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.

Images are ready for the commit at 7d1d2f6.

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

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

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.

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

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?

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.

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

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

/retest

please ensure value array construction consistency across insertInto and copyFrom

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

/retest

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

@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 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

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

                       {{- else if eq $field.SQLType "uuid" }}
                       pgutils.NilOrUUID({{$field.Getter "obj"}}),
+                      {{- else if eq $field.DataType "map" }}
+                      pgutils.EmptyOrMap({{$field.Getter "obj"}}),

refactor(pg): store nil map as empty {} by janisz · Pull Request #6929 · stackrox/stackrox · GitHub

Uh oh!

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

refactor(pg): store nil map as empty {}#6929janisz merged 1 commit intomasterstackrox/stackrox:masterfrom master-janisz/07-13-refactor_pg_store_empty_map_as_not_nullstackrox/stackrox:master-janisz/07-13-refactor_pg_store_empty_map_as_not_nullCopy head branch name to clipboard

Conversation

janisz commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Testing Performed

Uh oh!

janisz commented Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jul 13, 2023 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

connorgorman commented Jul 13, 2023

Uh oh!

janisz commented Jul 13, 2023

Uh oh!

connorgorman commented Jul 13, 2023

Uh oh!

janisz commented Jul 13, 2023

Uh oh!

dashrews78 commented Jul 13, 2023

Uh oh!

janisz commented Jul 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

janisz commented Jul 17, 2023

Uh oh!

rhybrillou left a comment

Choose a reason for hiding this comment

Uh oh!

rhybrillou Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

janisz Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

janisz Jul 25, 2023

Choose a reason for hiding this comment

Uh oh!

janisz commented Jul 25, 2023

Uh oh!

janisz commented Jul 26, 2023

Uh oh!

janisz commented Jul 26, 2023

Uh oh!

janisz commented Jul 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

refactor(pg): store nil map as empty {}#6929
janisz merged 1 commit into
masterfrom
master-janisz/07-13-refactor_pg_store_empty_map_as_not_null

janisz commented Jul 13, 2023 •
edited
Loading

janisz commented Jul 13, 2023 •
edited
Loading

ghost commented Jul 13, 2023 •
edited by ghost
Loading

janisz commented Jul 14, 2023 •
edited
Loading