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

[Snyk] Upgrade pg-promise from 10.9.2 to 10.10.1 #7286

Merged
merged 4 commits into from
Apr 29, 2021

Conversation

snyk-bot
Copy link
Contributor

Snyk has created this PR to upgrade pg-promise from 10.9.2 to 10.9.3.

ℹ️ Keep your dependencies up-to-date. This makes it easier to fix existing vulnerabilities and to more quickly identify and fix newly disclosed vulnerabilities when they affect your project.


  • The recommended version is 1 version ahead of your current version.
  • The recommended version was released 21 days ago, on 2021-02-28.
Release notes
Package name: pg-promise from pg-promise GitHub release notes
Commit messages
Package name: pg-promise

Compare


Note: You are seeing this because you or someone else with access to this repository has authorized Snyk to open upgrade PRs.

For more information:

🧐 View latest project report

🛠 Adjust upgrade PR settings

🔕 Ignore this dependency or unsubscribe from future upgrade PRs

@ghost
Copy link

ghost commented Mar 21, 2021

Danger run resulted in 1 fail and 1 markdown; to find out more, see the checks page.

Generated by 🚫 dangerJS

@codecov
Copy link

codecov bot commented Mar 21, 2021

Codecov Report

Merging #7286 (2a6b92d) into master (36de1db) will decrease coverage by 0.01%.
The diff coverage is 96.00%.

❗ Current head 2a6b92d differs from pull request most recent head 47ebd6b. Consider uploading reports for the commit 47ebd6b to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7286      +/-   ##
==========================================
- Coverage   93.92%   93.91%   -0.02%     
==========================================
  Files         179      181       +2     
  Lines       13153    13194      +41     
==========================================
+ Hits        12354    12391      +37     
- Misses        799      803       +4     
Impacted Files Coverage Δ
src/Auth.js 100.00% <ø> (ø)
src/Controllers/AdaptableController.js 95.65% <ø> (-0.35%) ⬇️
src/Options/Definitions.js 100.00% <ø> (ø)
src/Options/index.js 100.00% <ø> (ø)
src/ParseServerRESTController.js 98.50% <ø> (+1.49%) ⬆️
src/GraphQL/loaders/usersMutations.js 91.20% <80.00%> (-2.22%) ⬇️
...dapters/Storage/Postgres/PostgresStorageAdapter.js 95.36% <100.00%> (-0.16%) ⬇️
src/Controllers/UserController.js 97.67% <100.00%> (+0.03%) ⬆️
src/Deprecator/Deprecations.js 100.00% <100.00%> (ø)
src/Deprecator/Deprecator.js 100.00% <100.00%> (ø)
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 36de1db...47ebd6b. Read the comment docs.

@dplewis
Copy link
Member

dplewis commented Apr 8, 2021

@vitaly-t Do you have any suggestions before I dig in on the failing test?

@vitaly-t
Copy link
Contributor

vitaly-t commented Apr 9, 2021

@dplewis There was a change that looks related, but I didn't expect it to break anything. Other than that, not sure what's going on. If you find the exact test that's now failing, I will look into it.

@dplewis
Copy link
Member

dplewis commented Apr 9, 2021

Looks like there is only one failing test

Screen Shot 2021-04-08 at 11 01 00 PM

Screen Shot 2021-04-08 at 11 00 04 PM

@vitaly-t
Copy link
Contributor

vitaly-t commented Apr 9, 2021

@dplewis Unfortunately, this doesn't tell me enough. You see, you do not really use parse-server for anything myself, and I do not have its tests set up either, to start any kind of debugging. Do you know how to reproduce the issue by using pg-promise, i.e. an isolated test?

If you can be give me an isolated (without parse-server code) test that just uses pg-promise, then I can look into it.

Again, this is likely somehow related to #774.

@dplewis
Copy link
Member

dplewis commented Apr 9, 2021

I’ll give it a go. You wrote most of the transaction code here so I at least have something to work on. Do you have documentation on savepoints?

@vitaly-t
Copy link
Contributor

vitaly-t commented Apr 9, 2021

You wrote most of the transaction code here

I didn't, I only refactored them.

Do you have documentation on savepoints?

Issue #744 has most of it. Plus the official docs.

@cbaker6
Copy link
Contributor

cbaker6 commented Apr 25, 2021

Note that there are two test that are failing with the update. One of them just looks like it passes:

it('schemaUpgrade, upgrade the database schema when schema changes', async done => {
await adapter.deleteAllClasses();
const config = Config.get('test');
config.schemaCache.clear();
await adapter.performInitialization({ VolatileClassesSchemas: [] });
const client = adapter._client;
const className = '_PushStatus';
const schema = {
fields: {
pushTime: { type: 'String' },
source: { type: 'String' },
query: { type: 'String' },
},
};
adapter
.createTable(className, schema)
.then(() => getColumns(client, className))
.then(columns => {
expect(columns).toContain('pushTime');
expect(columns).toContain('source');
expect(columns).toContain('query');
expect(columns).not.toContain('expiration_interval');
schema.fields.expiration_interval = { type: 'Number' };
return adapter.schemaUpgrade(className, schema);
})
.then(() => getColumns(client, className))
.then(async columns => {
expect(columns).toContain('pushTime');
expect(columns).toContain('source');
expect(columns).toContain('query');
expect(columns).toContain('expiration_interval');
await reconfigureServer();
done();
})
.catch(error => done.fail(error));
});

with error:

BatchError {
    stat: { total: 18, succeeded: 1, failed: 17, duration: 143 }
    errors: [
        1: error: savepoint "sp_1_2" does not exist
            at Parser.parseErrorMessage (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/parser.js:278:15)
            at Parser.handlePacket (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/parser.js:126:29)
            at Parser.parse (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/parser.js:39:38)
            at Socket.<anonymous> (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/index.js:10:42)
            at Socket.emit (node:events:376:20)
            at addChunk (node:internal/streams/readable:304:12)
            at readableAddChunk (node:internal/streams/readable:279:9)
            at Socket.Readable.push (node:internal/streams/readable:218:10)
            at TCP.onStreamRead (node:internal/stream_base_commons:192:23) {
          length: 93,
          severity: 'ERROR',
          code: '3B001',
          detail: undefined,
          hint: undefined,
          position: undefined,
          internalPosition: undefined,
          internalQuery: undefined,
          where: undefined,
          schema: undefined,
          table: undefined,
          column: undefined,
          dataType: undefined,
          constraint: undefined,
          file: 'xact.c',
          line: '4167',
          routine: 'ReleaseSavepoint'
        }
        10: error: current transaction is aborted, commands ignored until end of transaction block
            at Parser.parseErrorMessage (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/parser.js:278:15)
            at Parser.handlePacket (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/parser.js:126:29)
            at Parser.parse (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/parser.js:39:38)
            at Socket.<anonymous> (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/index.js:10:42)
            at Socket.emit (node:events:376:20)
            at addChunk (node:internal/streams/readable:304:12)
            at readableAddChunk (node:internal/streams/readable:279:9)
            at Socket.Readable.push (node:internal/streams/readable:218:10)
            at TCP.onStreamRead (node:internal/stream_base_commons:192:23) {
          length: 144,
          severity: 'ERROR',
          code: '25P02',
          detail: undefined,
          hint: undefined,
          position: undefined,
          internalPosition: undefined,
          internalQuery: undefined,
          where: undefined,
          schema: undefined,
          table: undefined,
          column: undefined,
          dataType: undefined,
          constraint: undefined,
          file: 'postgres.c',
          line: '1101',
          routine: 'exec_simple_query'
        }
        11: error: current transaction is aborted, commands ignored until end of transaction block
            at Parser.parseErrorMessage (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/parser.js:278:15)
            at Parser.handlePacket (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/parser.js:126:29)
            at Parser.parse (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/parser.js:39:38)
            at Socket.<anonymous> (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/index.js:10:42)
            at Socket.emit (node:events:376:20)
            at addChunk (node:internal/streams/readable:304:12)
            at readableAddChunk (node:internal/streams/readable:279:9)
            at Socket.Readable.push (node:internal/streams/readable:218:10)
            at TCP.onStreamRead (node:internal/stream_base_commons:192:23) {
          length: 144,
          severity: 'ERROR',
          code: '25P02',
          detail: undefined,
          hint: undefined,
          position: undefined,
          internalPosition: undefined,
          internalQuery: undefined,
          where: undefined,
          schema: undefined,
          table: undefined,
          column: undefined,
          dataType: undefined,
          constraint: undefined,
          file: 'postgres.c',
          line: '1101',
          routine: 'exec_simple_query'
        }
        12: error: current transaction is aborted, commands ignored until end of transaction block
            at Parser.parseErrorMessage (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/parser.js:278:15)
            at Parser.handlePacket (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/parser.js:126:29)
            at Parser.parse (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/parser.js:39:38)
            at Socket.<anonymous> (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/index.js:10:42)
            at Socket.emit (node:events:376:20)
            at addChunk (node:internal/streams/readable:304:12)
            at readableAddChunk (node:internal/streams/readable:279:9)
            at Socket.Readable.push (node:internal/streams/readable:218:10)
            at TCP.onStreamRead (node:internal/stream_base_commons:192:23) {
          length: 144,
          severity: 'ERROR',
          code: '25P02',
          detail: undefined,
          hint: undefined,
          position: undefined,
          internalPosition: undefined,
          internalQuery: undefined,
          where: undefined,
          schema: undefined,
          table: undefined,
          column: undefined,
          dataType: undefined,
          constraint: undefined,
          file: 'postgres.c',
          line: '1101',
          routine: 'exec_simple_query'
        }
        13: error: current transaction is aborted, commands ignored until end of transaction block
            at Parser.parseErrorMessage (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/parser.js:278:15)
            at Parser.handlePacket (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/parser.js:126:29)
            at Parser.parse (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/parser.js:39:38)
            at Socket.<anonymous> (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/index.js:10:42)
            at Socket.emit (node:events:376:20)
            at addChunk (node:internal/streams/readable:304:12)
            at readableAddChunk (node:internal/streams/readable:279:9)
            at Socket.Readable.push (node:internal/streams/readable:218:10)
            at TCP.onStreamRead (node:internal/stream_base_commons:192:23) {
          length: 144,
          severity: 'ERROR',
          code: '25P02',
          detail: undefined,
          hint: undefined,
          position: undefined,
          internalPosition: undefined,
          internalQuery: undefined,
          where: undefined,
          schema: undefined,
          table: undefined,
          column: undefined,
          dataType: undefined,
          constraint: undefined,
          file: 'postgres.c',
          line: '1101',
          routine: 'exec_simple_query'
        }
    ]
}

and

it('Create a table without columns and upgrade with columns', done => {
const client = adapter._client;
const className = 'EmptyTable';
dropTable(client, className)
.then(() => adapter.createTable(className, {}))
.then(() => getColumns(client, className))
.then(columns => {
expect(columns.length).toBe(0);
const newSchema = {
fields: {
columnA: { type: 'String' },
columnB: { type: 'String' },
},
};
return adapter.schemaUpgrade(className, newSchema);
})
.then(() => getColumns(client, className))
.then(columns => {
expect(columns.length).toEqual(2);
expect(columns).toContain('columnA');
expect(columns).toContain('columnB');
done();
})
.catch(done);
});

with error:

1) PostgresStorageAdapter Create a table without columns and upgrade with columns
  Message:
    Failed: savepoint "sp_1_2" does not exist
  Stack:
    error properties: BatchError {
        stat: { total: 2, succeeded: 1, failed: 1, duration: 21 }
        errors: [
            1: error: savepoint "sp_1_2" does not exist
                at Parser.parseErrorMessage (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/parser.js:278:15)
                at Parser.handlePacket (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/parser.js:126:29)
                at Parser.parse (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/parser.js:39:38)
                at Socket.<anonymous> (/Users/coreybaker/Documents/github/parse-server/node_modules/pg-protocol/dist/index.js:10:42)
                at Socket.emit (node:events:376:20)
                at addChunk (node:internal/streams/readable:304:12)
                at readableAddChunk (node:internal/streams/readable:279:9)
                at Socket.Readable.push (node:internal/streams/readable:218:10)
                at TCP.onStreamRead (node:internal/stream_base_commons:192:23) ... ...
        at <Jasmine>
        at check (/Users/coreybaker/Documents/github/parse-server/node_modules/spex/lib/ext/batch.js:135:32)
        at step (/Users/coreybaker/Documents/github/parse-server/node_modules/spex/lib/ext/batch.js:109:17)
        at /Users/coreybaker/Documents/github/parse-server/node_modules/spex/lib/ext/batch.js:83:17
        at /Users/coreybaker/Documents/github/parse-server/node_modules/spex/lib/utils/index.js:62:25
        at processTicksAndRejections (node:internal/process/task_queues:93:5)

@vitaly-t
Copy link
Contributor

vitaly-t commented Apr 25, 2021

Is that an issue with pg-promise or with the parse-server? There was a change in pg-promise that made function of savepoints more strict. So it is possible that parse-server tests have been doing something wrong that wasn't caught before.

I asked before to create an isolated test for pg-promise to show the issue. I for one never used parser-server for anything, and running its tests cannot be conclusive anyway. Tests within pg-promise itself however are comprehensive, to my knowledge. If something is missing, please show it.

@cbaker6
Copy link
Contributor

cbaker6 commented Apr 25, 2021

Is that an issue with pg-promise or with the parse-server? There was a change in pg-promise that made function of savepoints more strict. So it is possible that parse-server tests have been doing something wrong that wasn't caught before.

I'm not sure if it's pg-promise or the parse-server. There were also some changes to PostgresStorageAdapter.js and PostgresStorageAdapter.spec.js that could have introduced this or it could have always been doing something wrong like you mentioned.

@vitaly-t
Copy link
Contributor

@dplewis Any luck reproducing the issue via pg-promise directly? Without it we cannot make progress here. I suspect there is an issue with the test that fails, which only came up after savepoints in pg-promise became more strict.

@dplewis
Copy link
Member

dplewis commented Apr 26, 2021

@vitaly-t I haven't had time to look into yet. I'll try to get to it this week.

@dplewis
Copy link
Member

dplewis commented Apr 29, 2021

@vitaly-t I realized while working on #7283 that in Parse Server we are mixing the use of transactions and tasks. In the example below instead of tx(schema-upgrade) should be task(schema-upgrade) or at least the transaction connection should be reused instead of creating new ones that would be blocking each other (this could be the reason why our transaction tests are flaky and timeout).

09:55:46 tx(schema-upgrade)/start
09:55:46 tx(schema-upgrade): begin
09:55:46 tx(schema-upgrade): SELECT column_name FROM information_schema.columns WHERE table_name = 'EmptyTable'
debug: PG: addFieldIfNotExists
09:55:46 tx(add-field-if-not-exists)/start
09:55:46 tx(add-field-if-not-exists): savepoint sp_1_1
debug: PG: addFieldIfNotExists
09:55:46 tx(add-field-if-not-exists)/start
09:55:46 tx(add-field-if-not-exists): savepoint sp_1_2
09:55:46 tx(add-field-if-not-exists): ALTER TABLE "EmptyTable" ADD COLUMN IF NOT EXISTS "columnA" text
09:55:46 tx(add-field-if-not-exists): ALTER TABLE "EmptyTable" ADD COLUMN IF NOT EXISTS "columnB" text
09:55:46 tx(add-field-if-not-exists): SELECT "schema" FROM "_SCHEMA" WHERE "className" = 'EmptyTable' and ("schema"::json->'fields'->'columnA') is not null
09:55:46 tx(add-field-if-not-exists): SELECT "schema" FROM "_SCHEMA" WHERE "className" = 'EmptyTable' and ("schema"::json->'fields'->'columnB') is not null
09:55:46 tx(add-field-if-not-exists): UPDATE "_SCHEMA" SET "schema"=jsonb_set("schema", '{fields,columnA}', '{"type":"String"}')  WHERE "className"='EmptyTable'
09:55:46 tx(add-field-if-not-exists): UPDATE "_SCHEMA" SET "schema"=jsonb_set("schema", '{fields,columnB}', '{"type":"String"}')  WHERE "className"='EmptyTable'
09:55:46 tx(add-field-if-not-exists): release savepoint sp_1_1
09:55:46 tx(add-field-if-not-exists): release savepoint sp_1_2
09:55:46 tx(add-field-if-not-exists)/end; duration: .007, success: true
09:55:46 error: savepoint "sp_1_2" does not exist
         tx(add-field-if-not-exists): release savepoint sp_1_2
09:55:46 tx(add-field-if-not-exists)/end; duration: .009, success: false
09:55:46 tx(schema-upgrade): rollback
09:55:46 tx(schema-upgrade)/end; duration: .017, success: false

Screen Shot 2021-04-29 at 10 03 08 AM

@vitaly-t
Copy link
Contributor

I can see the issue there, savepoint sp_1_2 doesn't exist at that point, because a higher-level savepoint has been released, which in turn killed this one. I will try to reproduce the issue locally.

@dplewis dplewis changed the title [Snyk] Upgrade pg-promise from 10.9.2 to 10.9.3 [Snyk] Upgrade pg-promise from 10.9.2 to 10.10.1 Apr 29, 2021
@dplewis dplewis requested a review from davimacedo April 29, 2021 16:21
@dplewis
Copy link
Member

dplewis commented Apr 29, 2021

@cbaker6 How does this look?

@dplewis dplewis merged commit 755c494 into master Apr 29, 2021
@dplewis dplewis deleted the snyk-upgrade-0e4700db55bfbb050c1d0dd536146bb0 branch April 29, 2021 17:10
@vitaly-t
Copy link
Contributor

@dplewis I still want to get to the bottom of it. I'm not convinced it was an issue here either. Will need to run some tests on my end.

@dplewis
Copy link
Member

dplewis commented Apr 29, 2021

@vitaly-t I want to as well. Let me know if there is anything I can do to help.

@dplewis
Copy link
Member

dplewis commented Apr 29, 2021

@vitaly-t I ran the following and got the error. vitaly-t/pg-promise@master...dplewis:savepoint-test

db.tx(t => {
    const a = t.tx(t1 => { });
    const b = t.tx(t1 => { }); 
    const c = t.tx(t1 => { });

    return t.batch([a, b, c]);
});

@dplewis
Copy link
Member

dplewis commented Apr 30, 2021

@vitaly-t I opened a PR vitaly-t/pg-promise#784 Lets continue the conversation there

@vitaly-t
Copy link
Contributor

vitaly-t commented May 2, 2021

@dplewis @cbaker6

Thank you guys for your assistance in this matter. I have done my analysis of this issue, and posted details within this PR.

If anymore changes are needed, they would be within parse-server, to make sure that no code ever tries to execute a sub-transaction asynchronously. All sub-transactions must be synchronous, as explained in my notes there.

i.e. the only correct way to write such code:

await db.tx(async t => {
    const a = await t.tx(t1 => { });
    const b = await t.tx(t1 => { }); 
    const c = await t.tx(t1 => { });

    return {a, b, c}; // or return [a, b, c], if needed
});

The above approach makes sure the savepoints of the same parent transaction do not end up clashing because of asynchronous execution.

As per my notes, recent changes in pg-promise only made it more fool-proof. I suspect there is some more refactoring due in parse-server because of that. It's a tricky subject, and requires some good negative testing, which I believe is still missing here.

i.e. we did not create problems recently, we have revealed there have been problems all along, and we just did not know about them.

@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0-beta.1

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Nov 1, 2021
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
state:released Released as stable version state:released-beta Released as beta version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants