Skip to content

Feat: add triggers endpoints #116

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

Merged
merged 5 commits into from
Jul 7, 2021
Merged

Feat: add triggers endpoints #116

merged 5 commits into from
Jul 7, 2021

Conversation

kiwicopple
Copy link
Member

What kind of change does this PR introduce?

Fix #114

@w3b6x9
Copy link
Member

w3b6x9 commented Jun 30, 2021

Adding the following list to keep track of development.

PR Status:

  • GET /triggers
  • GET /triggers/:id
  • POST /triggers
  • DELETE /triggers/:id
  • PATCH /triggers
    Trigger properties that can be changed
    • Name
    • Extension dependence
    • Enable/Disable

@w3b6x9

This comment has been minimized.

@w3b6x9
Copy link
Member

w3b6x9 commented Jul 2, 2021

@kiwicopple @soedirgo I have a bit more work to follow up 987f84f but wanted to push up my progress so everyone can play around with the endpoints and provide feedback.

@w3b6x9 w3b6x9 force-pushed the feat/add_triggers branch 2 times, most recently from 1f9fc50 to 263b1c6 Compare July 3, 2021 04:54
Comment on lines 269 to 222
private splitQualifiedName(schemas: string[], fullName: string): string[] {
const names = []
const sortedDescSchemas = schemas.sort((a, b) => b.length - a.length)

for (const schema of sortedDescSchemas) {
const startIdx = fullName.indexOf(schema)
const splitIdx = schema.length

if (startIdx === 0 && fullName[splitIdx] === '.') {
names.push(fullName.slice(0, splitIdx))
names.push(fullName.slice(splitIdx + 1))
break
}
}

return names[0] ? names : [fullName]
}
}
Copy link
Member

Choose a reason for hiding this comment

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

@soedirgo this is something we discussed a few days ago about escaping qualified table names. Even though this has to make an additional query for existing schemas, thought it covers the edge case when schema name itself has period(s). It doesn't cover every edge case of qualified table names but I think it's more than good enough for now. Let me know what you think!

describe('/triggers', () => {
const trigger = {
name: 'test_trigger',
table: 'public.users_audit',
Copy link
Member Author

Choose a reason for hiding this comment

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

I see you have a fully qualified function in the triggers logic, but I think on all the other routes we use table and schema. I think that's a bit cleaner

Copy link
Member

Choose a reason for hiding this comment

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

@kiwicopple sounds good, I'll go and make the changes.

@w3b6x9 w3b6x9 force-pushed the feat/add_triggers branch from 82120dc to d7339f2 Compare July 6, 2021 02:54
Copy link
Member

@soedirgo soedirgo left a comment

Choose a reason for hiding this comment

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

@w3b6x9 left some comments. Bit of a chore to slog through, sorry about that... Let me know if you need some help, esp. for consuming the system catalog, it can be kinda finnicky sometimes.

@w3b6x9 w3b6x9 requested a review from soedirgo July 7, 2021 03:16
@w3b6x9 w3b6x9 force-pushed the feat/add_triggers branch 2 times, most recently from f03f85b to 5492411 Compare July 7, 2021 03:36
Copy link
Member

@soedirgo soedirgo left a comment

Choose a reason for hiding this comment

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

Looks good, just a few more small changes and I think this is ready to be merged :)

@w3b6x9 w3b6x9 force-pushed the feat/add_triggers branch from 5492411 to 756d9d0 Compare July 7, 2021 05:15
@w3b6x9 w3b6x9 requested a review from soedirgo July 7, 2021 06:51
@w3b6x9 w3b6x9 changed the title [WIP] Adds triggers for testing Feat: add triggers endpoints Jul 7, 2021
@w3b6x9 w3b6x9 force-pushed the feat/add_triggers branch from a305696 to 815d919 Compare July 7, 2021 06:58
@w3b6x9
Copy link
Member

w3b6x9 commented Jul 7, 2021

@soedirgo thanks so much for the review!

@w3b6x9 w3b6x9 force-pushed the feat/add_triggers branch from 815d919 to d531870 Compare July 7, 2021 07:05
@w3b6x9 w3b6x9 force-pushed the feat/add_triggers branch from d531870 to 6594cae Compare July 7, 2021 07:06
@w3b6x9 w3b6x9 merged commit 060c13e into develop Jul 7, 2021
@w3b6x9 w3b6x9 deleted the feat/add_triggers branch July 7, 2021 07:08
@github-actions
Copy link

github-actions bot commented Jul 7, 2021

🎉 This PR is included in version 0.19.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

avallete pushed a commit that referenced this pull request May 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for triggers
3 participants