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.

Sign up for GitHub

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

Jump to bottom

[OTE-308] Update upsert querybuilder and compliance table upsert function #1477

Merged
merged 2 commits into from
May 8, 2024
Merged

[OTE-308] Update upsert querybuilder and compliance table upsert function #1477

merged 2 commits into from
May 8, 2024

Conversation

affanv14
Copy link
Contributor

@affanv14 affanv14 commented May 8, 2024

Changelist

  • updates compliance table to use upsert query builder
  • upsert query builder was updated to account for multiple ids

Test Plan

  • existing tests pass(upserting a new val and existing val)

Author/Reviewer Checklist

  • If this PR has changes that result in a different app state given the same prior state and transaction list, manually add the state-breaking label.
  • If the PR has breaking postgres changes to the indexer add the indexer-postgres-breaking label.
  • If this PR isn't state-breaking but has changes that modify behavior in PrepareProposal or ProcessProposal, manually add the label proposal-breaking.
  • If this PR is one of many that implement a specific feature, manually label them all feature:[feature-name].
  • If you wish to for mergify-bot to automatically create a PR to backport your change to a release branch, manually add the label backport/[branch-name].
  • Manually add any of the following labels: refactor, chore, bug.

@affanv14 affanv14 changed the title Update upsert querybuilder and compliance table upsert function [OTE-308] Update upsert querybuilder and compliance table upsert function May 8, 2024
@linear Linear
Copy link

linear bot commented May 8, 2024

OTE-308 Add Upsert logic to Compliance Data Table to prevent race conditions

Christopher-Li
Christopher-Li reviewed May 8, 2024
View reviewed changes
indexer/packages/postgres/src/query-builders/upsert.ts
@@ -26,11 +28,16 @@ export default class UpsertQueryBuilder<M extends Model, R = M[]> extends QueryB
const colBindings = cols.map(() => '??').join(', ');
const valBindings = cols.map(() => '?').join(', ');
const setBindings = cols.map(() => '?? = ?').join(', ');
const idConditionBindings = idColumns.map(() => '?? = ?').join(' AND ');
Copy link
Contributor

Choose a reason for hiding this comment

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

what does ?? = ? do in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

Positional bindings, see here

Christopher-Li
Christopher-Li reviewed May 8, 2024
View reviewed changes
indexer/packages/postgres/src/query-builders/upsert.ts
@@ -40,15 +47,14 @@ export default class UpsertQueryBuilder<M extends Model, R = M[]> extends QueryB
`(${colBindings}) VALUES (${valBindings})`,
'ON CONFLICT (??) DO',
`UPDATE SET ${setBindings}`,
'WHERE ?? = ?',
`WHERE ${idConditionBindings}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

iiuc if there are two idColumns like in the case of compliance data, this would be WHERE ?? = ? AND ?? = ? right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes.

@affanv14
Copy link
Contributor Author

affanv14 commented May 8, 2024

https://github.com/Mergifyio backport release/indexer/v5.x

@mergify Mergify
Copy link
Contributor

mergify bot commented May 8, 2024 •
edited
Loading

backport release/indexer/v5.x

✅ Backports have been created

@affanv14 affanv14 merged commit a699c24 into main May 8, 2024
11 checks passed
@affanv14 affanv14 deleted the affan/fix-upsert branch May 8, 2024 17:05
mergify bot pushed a commit that referenced this pull request May 8, 2024
@affanv14 @mergify
[OTE-308] Update upsert querybuilder and compliance table upsert func…
d12a8d8 
…tion (#1477)

* Update upsert querybuilder and compliance data upsert function

* lint

(cherry picked from commit a699c24)
@mergify mergify bot mentioned this pull request May 8, 2024
Merged
6 tasks
affanv14 added a commit that referenced this pull request May 8, 2024
@mergify @affanv14
[OTE-308] Update upsert querybuilder and compliance table upsert func…
f664999 
…tion (#1477) (#1478)

* Update upsert querybuilder and compliance data upsert function

* lint

(cherry picked from commit a699c24)

Co-authored-by: Mohammed Affan <affanmd@nyu.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Christopher-Li Christopher-Li Christopher-Li left review comments

@vincentwschau vincentwschau vincentwschau approved these changes

Assignees
No one assigned
Labels
indexer
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@affanv14 @Christopher-Li @vincentwschau