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

refactor: Upgrade pg-promise from 11.3.0 to 11.5.0 #8586

Merged
merged 6 commits into from
May 29, 2023

Conversation

cbaker6
Copy link
Contributor

@cbaker6 cbaker6 commented May 28, 2023

Pull Request

Issue

The dependencies pg-promise is not up-to-date and the node-postgres driver pg-promise depends on is out-of-date. For some reason, these are not being updated by the dependency bot or the snyk bot

Closes: #8587

Approach

Update the dependencies using npm i -E pg-promise@11.5.0.

Tasks

  • Ensure tests pass

@parse-github-assistant
Copy link

parse-github-assistant bot commented May 28, 2023

Thanks for opening this pull request!

@codecov
Copy link

codecov bot commented May 28, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (505dd6b) 94.33% compared to head (c465b27) 94.33%.

❗ Current head c465b27 differs from pull request most recent head 1829bd5. Consider uploading reports for the commit 1829bd5 to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##            alpha    #8586   +/-   ##
=======================================
  Coverage   94.33%   94.33%           
=======================================
  Files         183      183           
  Lines       14576    14576           
=======================================
  Hits        13750    13750           
  Misses        826      826           

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@cbaker6
Copy link
Contributor Author

cbaker6 commented May 28, 2023

#8511 seems to have consistently failed a test in the suite which can be seen as a failure in multiple postgres versions original PR failure and alpha branch failure with:

1) InstallationsRouter query installations with count = 1
  - Expected 0 to equal 2.

I've checked the test and it seems like the original way returned an accurate value.

Should that PR be reverted until it's fixed as it never passed CI in the first place? cc: @mtrezza @dblythy

@cbaker6
Copy link
Contributor Author

cbaker6 commented May 28, 2023

I'm not sure why there's a definitions check failure as I didn't touch any definitions. I ran the line it suggests, but it generates no changes in my local repo to commit.

@mtrezza
Copy link
Member

mtrezza commented May 28, 2023

Definitions is flaky, see #8579.

#8511 should fix #8502, which seems to be an issue others can replicate. If the fix introduces new issues we can revert it. #8588

@cbaker6
Copy link
Contributor Author

cbaker6 commented May 28, 2023

#8511 should fix #8502, which seems to be an issue others can replicate. If the fix introduces new issues we can revert it.

I don't see the issue the others have reported (screenshot below, Parse Dashboard v5.1.0, Postgres 15-3.3). The count query used in the Dashboard is intended to be an estimate due to Postgres counting being expensive/slow (the Postgres wiki suggests something similar to the original parse-server implementation for count). It looks like the devs reporting the issue all have small amounts of rows in their tables which can easily be estimated to zero by Postgres. In addition, if a fix is suggested, I believe it should follow the process of any other PR, it should:

  1. Introduce a test to prove it works (fix: Inaccurate table total row count for PostgreSQL #8511 didn't do this)
  2. Not break other tests (fix: Inaccurate table total row count for PostgreSQL #8511 broke a test). If the PR creator believes the test is incorrect, they should justify why it's incorrect and provide a fix for the test. From what I can see, the original test is correct and matches the response value for the equivalent mongo test which is right above it in the same file.
image

I believe the PR should be reverted and the creator of the PR should continue to work on it. Even if a change is made, the adjustments in PR 8511 are still estimates, meaning they won't be exact either. The broken test is below for reference:

it_only_db('postgres')('query installations with count = 1', async () => {
const config = Config.get('test');
const androidDeviceRequest = {
installationId: '12345678-abcd-abcd-abcd-123456789abc',
deviceType: 'android',
};
const iosDeviceRequest = {
installationId: '12345678-abcd-abcd-abcd-123456789abd',
deviceType: 'ios',
};
const request = {
config: config,
auth: auth.master(config),
body: {},
query: {
count: 1,
},
info: {},
};
const router = new InstallationsRouter();
await rest.create(config, auth.nobody(config), '_Installation', androidDeviceRequest);
await rest.create(config, auth.nobody(config), '_Installation', iosDeviceRequest);
let res = await router.handleFind(request);
let response = res.response;
expect(response.results.length).toEqual(2);
expect(response.count).toEqual(0); // estimate count is zero
const pgAdapter = config.database.adapter;
await pgAdapter.updateEstimatedCount('_Installation');
res = await router.handleFind(request);
response = res.response;
expect(response.results.length).toEqual(2);
expect(response.count).toEqual(2);

@cbaker6 cbaker6 changed the title refactor: Upgrade pg-promise to 11.5.0 refactor: Upgrade pg-promise from 11.3.0 to 11.5.0 May 28, 2023
@mtrezza mtrezza merged commit fc3b752 into parse-community:alpha May 29, 2023
@cbaker6 cbaker6 deleted the updatePG branch May 29, 2023 16:43
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.1.0-alpha.17

@parseplatformorg parseplatformorg added the state:released-alpha Released as alpha version label Jun 7, 2023
@parseplatformorg
Copy link
Contributor

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

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Jun 10, 2023
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.3.0-alpha.1

@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 6.3.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Sep 16, 2023
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-alpha Released as alpha version state:released-beta Released as beta version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pg-promise is out-of-date
3 participants