Database triggers #4860
Open
Database triggers #4860
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…triggers page and backend function
…op for database_trigger
|
All contributors have signed the CLA ✍️ ✅ |
Deploying windmill with
|
| Latest commit: |
beed5fd
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6710650f.windmill.pages.dev |
| Branch Preview URL: | https://database-triggers.windmill.pages.dev |
rubenfiszel
force-pushed
the
main
branch
5 times, most recently
from
e36281c to
f85cf46
Compare
![ellipsis-dev[bot]](https://avatars.githubusercontent.com/in/64358?s=60&v=4){kind=small}
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 7011cda in 56 seconds
More details
- Looked at
283lines of code in13files - Skipped
2files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. package.json:1
- Draft comment:
Thepackage.jsonfile is empty. Ensure it contains the necessary metadata and dependencies for the project. - Reason this comment was not posted:
Comment did not seem useful.
Workflow ID: wflow_cZwmVncO4kWVN47y
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Generated with ❤️ by ellipsis.dev |
|
Generated with ❤️ by ellipsis.dev |
|
Generated with ❤️ by ellipsis.dev |
…end error with missing feature on back
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on eaf47f5 in 4 minutes and 7 seconds
More details
- Looked at
9806lines of code in299files - Skipped
0files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. backend/.sqlx/query-0ef37117c369f03236e18f9dbb1f3d52776c8cb73f2507199c6ca16d4d2405ba.json:4
- Draft comment:
Using a leading wildcard '%' in a LIKE clause can cause performance issues if the column is not indexed properly. Consider reviewing the indexing strategy for the 'email' column. - Reason this comment was not posted:
Confidence changes required:50%
The query in the JSON file uses a wildcard '%' in a LIKE clause, which can lead to performance issues if the column is not indexed properly. This is a common issue in SQL queries that use LIKE with leading wildcards.
2. backend/.sqlx/query-12a0fd7d8d99fb73b01bc24774fe9a8da57b5204bb6b1207aed47143c17a20bc.json:15
- Draft comment:
Thenullablefield for theCOUNTresult should befalseinstead ofnullasCOUNTalways returns a non-null value. - Reason this comment was not posted:
Comment was on unchanged code.
Workflow ID: wflow_XuhP44EXSa8rdmDf
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Generated with ❤️ by ellipsis.dev |
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 66e305d in 1 minute and 59 seconds
More details
- Looked at
12lines of code in1files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. backend/windmill-api/src/database_triggers/trigger.rs:42
- Draft comment:
The#[allow(unused)]attribute is unnecessary here as theRowExisttrait is implemented and used. Consider removing it. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
While the trait is implemented, I don't see where this implementation is actually used in the code. The trait might be used in other files, but I can't see those. Without seeing the full usage context, I can't be certain that the #[allow(unused)] is unnecessary. The author may have added it for a reason.
I could be wrong about the usage - there might be implicit uses of this trait that I'm not seeing, or it could be used through type inference. The implementation exists, which suggests it's used somewhere.
Even with those possibilities, I don't have strong evidence that the attribute is unnecessary. The presence of an implementation doesn't guarantee the trait is actually used.
Without being able to see the full context of how this trait is used across the codebase, I cannot confidently say that the #[allow(unused)] attribute is unnecessary.
Workflow ID: wflow_jEWh2OKighlJcOrf
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
|
Generated with ❤️ by ellipsis.dev |
HugoCasa
requested changes
Jan 9, 2025
| @@ -258,6 +261,7 @@ wasm-bindgen-test = "0.3.42" | |||
| convert_case = "0.6.0" | |||
| getrandom = "0.2" | |||
| tokio-postgres = {version = "^0.7", features = ["array-impls", "with-serde_json-1", "with-chrono-0_4", "with-uuid-1", "with-bit-vec-0_6"]} | |||
| rust-postgres = { package = "tokio-postgres", git = "https://github.com/imor/rust-postgres", rev = "20265ef38e32a06f76b6f9b678e2077fc2211f6b"} | |||
There was a problem hiding this comment.
do you know why they use this specific version?
There was a problem hiding this comment.
because it implement postgres copy both query which is needed to make the logical replication protocol works
frontend/src/lib/components/graph/renderers/triggers/TriggersBadge.svelte
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
better if put in another PR
There was a problem hiding this comment.
you're missing auth for a bunch of these APIs
also you should use authed to get the resource with get_resource_value_interpolated_internal
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on c26c0f8 in 2 minutes and 19 seconds
More details
- Looked at
1270lines of code in29files - Skipped
1files when reviewing. - Skipped posting
8drafted comments based on config settings.
1. frontend/src/lib/components/GfmMarkdown.svelte:4
- Draft comment:
Consider adding a comment explaining thatrehypeRawandrehypeGithubAlertsare used as plugins forsvelte-exmarkdown. - Reason this comment was not posted:
Confidence changes required:20%
The code infrontend/src/lib/components/GfmMarkdown.svelteis importingrehypeRawandrehypeGithubAlertsbut does not use them directly. Instead, they are passed as plugins tosvelte-exmarkdown. This is correct usage, but it might not be immediately clear to someone reading the code. A comment explaining this could be helpful.
2. frontend/src/lib/components/settings/ChangeWorkspaceColor.svelte:15
- Draft comment:
Consider adding a comment explaining the purpose of the reactive statement that callsgenerateRandomColor. - Reason this comment was not posted:
Confidence changes required:20%
Infrontend/src/lib/components/settings/ChangeWorkspaceColor.svelte, the functiongenerateRandomColoris called in a reactive statement. This is correct, but it might not be immediately clear why this is done. A comment explaining the purpose of this reactive statement could be helpful.
3. frontend/src/lib/components/sidebar/MenuButton.svelte:14
- Draft comment:
Consider adding a comment explaining the purpose of thecolorprop and how it affects the button's styling. - Reason this comment was not posted:
Confidence changes required:20%
Infrontend/src/lib/components/sidebar/MenuButton.svelte, thecolorprop is used to conditionally apply styles. This is correct, but it might not be immediately clear why this is done. A comment explaining the purpose of thecolorprop could be helpful.
4. frontend/src/lib/components/sidebar/WorkspaceMenu.svelte:64
- Draft comment:
Consider adding a comment explaining the purpose of thecolorproperty and how it affects the workspace button's styling. - Reason this comment was not posted:
Confidence changes required:20%
Infrontend/src/lib/components/sidebar/WorkspaceMenu.svelte, thecolorproperty is used to style the workspace button. This is correct, but it might not be immediately clear why this is done. A comment explaining the purpose of thecolorproperty could be helpful.
5. frontend/src/lib/stores.ts:66
- Draft comment:
Consider adding a comment explaining the purpose of thecolorproperty in theuserWorkspacesstore. - Reason this comment was not posted:
Confidence changes required:20%
Infrontend/src/lib/stores.ts, theuserWorkspacesstore includes acolorproperty. This is correct, but it might not be immediately clear why this property is included. A comment explaining the purpose of thecolorproperty could be helpful.
6. frontend/src/routes/(root)/(logged)/user/(user)/create_workspace/+page.svelte:43
- Draft comment:
Consider adding a comment explaining the purpose of thegenerateRandomColorfunction and when it is used. - Reason this comment was not posted:
Confidence changes required:20%
Infrontend/src/routes/(root)/(logged)/user/(user)/create_workspace/+page.svelte, thegenerateRandomColorfunction is used to set a random color for the workspace. This is correct, but it might not be immediately clear why this is done. A comment explaining the purpose of this function could be helpful.
7. frontend/src/routes/(root)/(logged)/user/(user)/workspaces/+page.svelte:31
- Draft comment:
Consider adding a comment explaining the purpose of thecolorproperty and how it affects the workspace button's styling. - Reason this comment was not posted:
Confidence changes required:20%
Infrontend/src/routes/(root)/(logged)/user/(user)/workspaces/+page.svelte, thecolorproperty is used to style the workspace button. This is correct, but it might not be immediately clear why this is done. A comment explaining the purpose of thecolorproperty could be helpful.
8. frontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte:59
- Draft comment:
Consider adding a comment explaining the purpose of theChangeWorkspaceColorcomponent and how it is used. - Reason this comment was not posted:
Confidence changes required:20%
Infrontend/src/routes/(root)/(logged)/workspace_settings/+page.svelte, theChangeWorkspaceColorcomponent is used to change the color of a workspace. This is correct, but it might not be immediately clear why this component is included. A comment explaining the purpose of this component could be helpful.
Workflow ID: wflow_eFQq8cU1t8Ad8GXG
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on 81d3ec8 in 3 minutes and 52 seconds
More details
- Looked at
490lines of code in11files - Skipped
1files when reviewing. - Skipped posting
2drafted comments based on config settings.
1. backend/windmill-api/src/database_triggers/trigger.rs:223
- Draft comment:
serde_json::from_valueis used incorrectly here.resourceis aString, not aserde_json::Value. Useserde_json::from_strinstead. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment assumes resource is a String, but looking at the code flow: 1) It comes from a SQL query 2) It's wrapped in Option and flattened 3) The value column in databases often stores JSON data 4) The code is trying to deserialize it into a Database struct. This suggests resource is actually a serde_json::Value, not a String.
I could be wrong about the database schema - maybe the value column actually stores strings. I'm making assumptions about the schema.
Even with that uncertainty, the code pattern of using from_value after a SQL query strongly suggests this is correct usage for deserializing from a JSON database column. The comment appears to be making incorrect assumptions.
The comment should be deleted. The code is likely correct in using from_value to deserialize JSON data from a database column.
2. backend/windmill-api/src/database_triggers/handler.rs:246
- Draft comment:
The error handling here is too generic. Instead of converting all errors toError::NotFound, consider propagating the original error to provide more context. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
The comment has a point - the code is losing error context by mapping all errors to NotFound. However, looking at the code more carefully, this seems intentional as the error message specifically states "Database resource do not exist". The function's purpose appears to be to verify resource existence, so mapping to NotFound is reasonable. The error handling matches the function's intent.
I could be wrong about the function's intent - maybe there are important error cases being hidden. The error message grammar is also incorrect ("do not exist" vs "does not exist").
While the grammar could be improved, the error handling itself appears intentional and appropriate for a resource lookup function. The comment's suggestion would make the error handling more complex without clear benefit.
The comment should be deleted as the current error handling appears to be an intentional design choice appropriate for this resource lookup function.
Workflow ID: wflow_SSYeRwdbDhS1VowD
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
HugoCasa
reviewed
Jan 10, 2025
| @@ -210,13 +209,32 @@ async fn listen_to_transactions( | |||
| db: DB, | |||
| mut killpill_rx: tokio::sync::broadcast::Receiver<()>, | |||
| ) -> Result<(), Error> { | |||
| let resource = get_database_resource( | |||
| &db, | |||
| let resource = sqlx::query_scalar!( | |||
There was a problem hiding this comment.
you should also use get_resource_value_interpolated here. You can use fetch_api_authed with the trigger editor/email fields
There was a problem hiding this comment.
👍 Looks good to me! Incremental review on beed5fd in 1 minute and 40 seconds
More details
- Looked at
1127lines of code in25files - Skipped
0files when reviewing. - Skipped posting
1drafted comments based on config settings.
1. backend/.sqlx/query-ef6795d93423f98eea82eb18e6332580dc7f7a9e5a67026f8c0b3077f371fc62.json:9
- Draft comment:
Thetype_infoforhashisInt8, which is unusual for a hash value. Consider using a more appropriate data type likeTextorBytea. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable:
Since this is an auto-generated file reflecting the actual database schema, suggesting a type change here won't be useful - the change would need to be made in the actual database schema, not this file. Additionally, while Int8 might be unusual for a hash, we don't have enough context about why this design choice was made. The hash could be a numeric hash or could have specific performance requirements.
The comment raises a valid point about typical hash storage patterns. Maybe there's a real performance or data integrity issue here.
Even if the concern is valid, this auto-generated file is not the right place to raise it. Schema design discussions should happen when the database schema is being modified.
Delete the comment because it's targeting an auto-generated file that reflects the actual database schema, making it not actionable in this context.
Workflow ID: wflow_lXIU1UZF4lqkd2un
You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PostgreSQL database trigger implementation that captures transaction events (INSERT, UPDATE, DELETE). Currently, the implementation identifies the type of transaction.
Currently working on decoding the trigger message to extract detailed information about the underlying data that caused the trigger to fire.
Important
Add support for managing database triggers in the frontend and backend, including new UI components and service methods.
DatabaseTriggersPanelto display database triggers inflowsandscriptspages.DatabaseTriggerEditorfor creating and editing database triggers.+layout.svelteto load database trigger usage.database_triggersroute with+page.svelteand+page.jsfor managing database triggers.DatabaseTriggerService.getTriggersCountOfFlowandgetTriggersCountOfScript.SidebarContent.svelteto include database triggers in the menu.TriggersBadge.svelte.This description was created by
for beed5fd. It will automatically update as commits are pushed.