-
Notifications
You must be signed in to change notification settings - Fork 3
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
WIP: auto crud #51
base: master
Are you sure you want to change the base?
WIP: auto crud #51
Conversation
@kalinkrustev Can you please review the approach? The skeleton is ready. |
DECLARE @result ${binding.tt} | ||
BEGIN TRY | ||
INSERT INTO ${binding.name} (${columns}) | ||
OUTPUT INSERTED.* INTO @result |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is @result
really needed here? Is it needed because of history triggers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No praticular reason. Just thought that storing the data in a table type would be easier to return all the data as a named resultset at the end.
SELECT ${columns} | ||
FROM @data | ||
|
||
SELECT 'data' AS resultSetName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we discuss if 'data' is good as name? If we call multiple procedures as part of a bigger one, it is better each to return different name for the resultset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. Instead of 'data' it can be the table name. e.g. 'customer.customer', 'customer.organization', etc.
"uuid": "3.2.1", | ||
"xml2js": "0.4.19" | ||
}, | ||
"peerDependencies": { | ||
"tedious": "^2.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this already a peer dependency of mssql?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is a dependency of mssql. And I saw it is directly required here so I had to add it to the package.json. If we end up with 2 different versions of tedious that patch won't work.
SET NOCOUNT ON | ||
DECLARE @result ${binding.tt} | ||
BEGIN TRY | ||
INSERT INTO ${binding.name} (${columns}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also have a generic logic for checking for already existing records and raising standard errors?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, I just wrote some generic SQL code which I didn't mean to be final ... just to test the concept.
We can change the bodies and the names of the procedures however we like. My main idea was to align with you whether the changes in index.js are fine. Maybe @mlessevsg can help with the SQL.
About naming I am still not sure. If we leave the CRUD naming, then we could have also our current procedures and call the CRUD within them, as the manual ones will probably do some additional actions. |
Features:
Fixes:
Notes: