Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

fix(Postgres Node): Account for JSON expressions #12012

Merged
merged 8 commits into from
Dec 20, 2024

Conversation

dana-gill
Copy link
Contributor

@dana-gill dana-gill commented Dec 2, 2024

Summary

Fixes the bug where query params which include multiple JSON key/vals fails in Postgres Node

Workflows for testing:

{
  "nodes": [
    {
      "parameters": {
        "operation": "executeQuery",
        "query": "SELECT insert_ex(\n    $1::jsonb\n);",
        "options": {
          "queryReplacement": "={{ JSON.stringify(\n\n{id: \"848da11d-e72e-44c5-xxxx-c6fb9f17d366\",id2: \"848da11d-e72e-44c5-yyyy-c6fb9f17d366\"}\n\n) }}"
        }
      },
      "id": "4c1d023e-ab9c-467f-a43f-a830b214eefa",
      "name": "This is the problem1",
      "type": "n8n-nodes-base.postgres",
      "typeVersion": 2.5,
      "position": [
        1560,
        300
      ],
      "credentials": {
        "postgres": {
          "id": "UeO14x83grJWzgYI",
          "name": "Postgres account 2"
        }
      }
    },
    {
      "parameters": {
        "operation": "executeQuery",
        "query": "SELECT insert_ex(\n        $1::jsonb\n\n);",
        "options": {
          "queryReplacement": "={{ JSON.stringify(\n\n{id: $$66$$}\n\n) }}"
        }
      },
      "id": "110341e5-7b89-4a49-a4bd-fe860a6fa7fd",
      "name": "This works1",
      "type": "n8n-nodes-base.postgres",
      "typeVersion": 2.5,
      "position": [
        1560,
        120
      ],
      "credentials": {
        "postgres": {
          "id": "UeO14x83grJWzgYI",
          "name": "Postgres account 2"
        }
      }
    }
  ],
  "connections": {},
  "pinData": {}
}

Related Linear tickets, Github issues, and Community forum posts

https://linear.app/n8n/issue/NODE-1753/postgres-node-inserting-json-with-multiple-keyvalue-via-query
https://community.n8n.io/t/issue-with-inserting-json-with-multiple-key-value-pairs-in-postgresql-jsonb/53939/

Review / Merge checklist

  • PR title and summary are descriptive. (conventions)
  • Docs updated or follow-up ticket created.
  • Tests included.
  • PR Labeled with release/backport (if the PR is an urgent fix that needs to be backported)

@dana-gill dana-gill changed the title Node 1753 inserting json multiple key val fails fix(Postgres Node): Account for JSON expressions Dec 2, 2024
@n8n-assistant n8n-assistant bot added n8n team Authored by the n8n team node/improvement New feature or request labels Dec 2, 2024
Copy link

codecov bot commented Dec 2, 2024

Codecov Report

Attention: Patch coverage is 85.00000% with 3 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
...gres/v2/actions/database/executeQuery.operation.ts 80.00% 1 Missing and 1 partial ⚠️
...ages/nodes-base/nodes/Postgres/v2/helpers/utils.ts 90.00% 1 Missing ⚠️

📢 Thoughts on this report? Let us know!

@dana-gill dana-gill force-pushed the node-1753-inserting-json-multiple-key-val-fails branch from 9614fd6 to fa94cee Compare December 3, 2024 09:53
@dana-gill dana-gill marked this pull request as ready for review December 3, 2024 11:35
@dana-gill dana-gill force-pushed the node-1753-inserting-json-multiple-key-val-fails branch 4 times, most recently from 27f09c0 to 166eaa8 Compare December 18, 2024 10:49
@dana-gill
Copy link
Contributor Author

fwiw, i'm fairly certain the codecov is complaining because I converted the loop into a map and therefore touched lines which are yet to be tested. however, code coverage is increased by 0.21% 😄

@dana-gill dana-gill force-pushed the node-1753-inserting-json-multiple-key-val-fails branch 2 times, most recently from 561f690 to cc210b1 Compare December 20, 2024 10:58
@dana-gill dana-gill force-pushed the node-1753-inserting-json-multiple-key-val-fails branch from cc210b1 to c6702ec Compare December 20, 2024 11:12
ShireenMissi
ShireenMissi previously approved these changes Dec 20, 2024
Copy link
Contributor

@ShireenMissi ShireenMissi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

Copy link

cypress bot commented Dec 20, 2024

n8n    Run #8434

Run Properties:  status check failed Failed #8434  •  git commit c6702ececb: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 dana-gill 🗃️ e2e/*
Project n8n
Branch Review node-1753-inserting-json-multiple-key-val-fails
Run status status check failed Failed #8434
Run duration 04m 22s
Commit git commit c6702ececb: 🌳 🖥️ browsers:node18.12.0-chrome107 🤖 dana-gill 🗃️ e2e/*
Committer Dana Lee
View all properties for this run ↗︎

Test results
Tests that failed  Failures 1
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 387
View all changes introduced in this branch ↗︎

Tests for review

Failed  14-mapping.cy.ts • 1 failed test

View Output Video

Test Artifacts
Data mapping > maps expressions from json view Test Replay Screenshots Video
Failed  24-ndv-paired-item.cy.ts • 0 failed tests

View Output

Test Artifacts
Failed  27-two-factor-authentication.cy.ts • 0 failed tests

View Output

Test Artifacts
Failed  17-workflow-tags.cy.ts • 0 failed tests

View Output

Test Artifacts
Failed  28-debug.cy.ts • 0 failed tests

View Output

Test Artifacts

The first 5 failed specs are shown, see all 35 specs in Cypress Cloud.

@dana-gill dana-gill force-pushed the node-1753-inserting-json-multiple-key-val-fails branch from c6702ec to f204733 Compare December 20, 2024 13:26
@dana-gill dana-gill force-pushed the node-1753-inserting-json-multiple-key-val-fails branch from f204733 to 0f9922a Compare December 20, 2024 13:43
Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

2 similar comments
Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

Copy link
Contributor

⚠️ Some Cypress E2E specs are failing, please fix them before merging

Copy link
Contributor

✅ All Cypress E2E specs passed

@dana-gill dana-gill merged commit 06b86af into master Dec 20, 2024
37 checks passed
@dana-gill dana-gill deleted the node-1753-inserting-json-multiple-key-val-fails branch December 20, 2024 17:35
riascho pushed a commit that referenced this pull request Dec 23, 2024
Co-authored-by: Shireen Missi <94372015+ShireenMissi@users.noreply.github.com>
@github-actions github-actions bot mentioned this pull request Jan 8, 2025
@janober
Copy link
Member

janober commented Jan 9, 2025

Got released with n8n@1.74.1

1 similar comment
@janober
Copy link
Member

janober commented Jan 9, 2025

Got released with n8n@1.74.1

@janober
Copy link
Member

janober commented Jan 9, 2025

Got released with n8n@1.74.0

1 similar comment
@janober
Copy link
Member

janober commented Jan 9, 2025

Got released with n8n@1.74.0

riascho pushed a commit that referenced this pull request Jan 14, 2025
Co-authored-by: Shireen Missi <94372015+ShireenMissi@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
n8n team Authored by the n8n team node/improvement New feature or request Released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants