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

V2 Stored Procedure Support #697

Merged
merged 53 commits into from
Jan 22, 2025
Merged

V2 Stored Procedure Support #697

merged 53 commits into from
Jan 22, 2025

Conversation

devhawk
Copy link
Collaborator

@devhawk devhawk commented Jan 8, 2025

Adds @DBOS.storedProcedure (aka v2 stored procedures)

Note, plv8 does not support node's AsyncLocalStorage API. Since all DBOS functions are static methods in order to support decorators, the stored proc runtime uses Function.apply to provide a custom this object that the stored proc runtime version of DBOS context methods accesses. This does change the expected behavior of this object in static methods - normally, this would point to the class object instance. However, access to the class object instance via this isn't very useful to stored procedures anyway. If we feel it's important to maintain access to the class object instance, we can add a property to the custom this object that references the class object.

@chuck-dbos chuck-dbos self-requested a review January 13, 2025 15:25
@devhawk devhawk marked this pull request as ready for review January 13, 2025 17:06
@chuck-dbos
Copy link
Collaborator

"normally, this would point to the class object instance". DBOS is already not doing that, usually inside a DBOS static function 'this' is 'undefined'.

src/dbos-executor.ts Outdated Show resolved Hide resolved
src/dbos.ts Outdated Show resolved Hide resolved
@devhawk
Copy link
Collaborator Author

devhawk commented Jan 16, 2025

"normally, this would point to the class object instance". DBOS is already not doing that, usually inside a DBOS static function 'this' is 'undefined'.

I'm not sure what this is in reference to

@chuck-dbos
Copy link
Collaborator

"normally, this would point to the class object instance". DBOS is already not doing that, usually inside a DBOS static function 'this' is 'undefined'.

I'm not sure what this is in reference to

Try reading it as normally, 'this' would point to the class object instance. DBOS is already giving 'undefined' as 'this' in its static functions.

@devhawk
Copy link
Collaborator Author

devhawk commented Jan 16, 2025

I should have updated the initial PR comment. The Function.apply approach didn't work - the 'this' object gets reset when you call into a context method like 'DBOS.workflowID'. Global state is the only option for storing context state so the 'DBOS' statics can retrieve them.

I confirmed that the global 'plv8' object is shared between connections. So we hang the DBOS context info off that object where the DBOS context methods can get it. That context info is set and deleted in the generated proc to avoid information leakage

Copy link
Member

@kraftp kraftp left a comment

Choose a reason for hiding this comment

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

Let's hold off on merging this until after release

@devhawk
Copy link
Collaborator Author

devhawk commented Jan 16, 2025

Merge when you feel comfortable, I'm OOF until after release anyway

@devhawk devhawk merged commit e76dc26 into main Jan 22, 2025
5 checks passed
@devhawk devhawk deleted the devhawk/storedProc-v2 branch January 22, 2025 21:21
kraftp added a commit that referenced this pull request Jan 28, 2025
This was removed in
#697. Re-enabling it
and adding tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants