-
Notifications
You must be signed in to change notification settings - Fork 23
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
refactor: Rework internals #54
Merged
Merged
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
Changing to "postgresql" as the official Postgres type as the parser library currently requires that format and there are no other dependencies on the alternative.
Update how values are passed into `executeTransaction` so LiteREST calls succeed
Added CORS check back in and updated JWKS logic to support auth requests with JWT
Fix up export and import functionality for internal sources
Needs additional testing before official support
Brayden
approved these changes
Dec 21, 2024
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.
I've added a few follow-up commits after testing all of the features. Overall this PR is a quality of life improvements introducing these high-level items, but many more (see original description) included as well.
- Type safety
- Ability to toggle on/off features
- Shared RPC reference for reduced costs
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.
Purpose
As discussed on Discord, this is a pretty large change to allow full configurability of Starbase and overhauls some of the internals.
Key changes:
DataSource.rpc
as a single RPC Session, this means for the duration of a single request, we're only charged for a single DO request, whereas currently it's multiple.StarbaseDB
.DataSource
instance, with:rpc
property. This is the result of calling the DOinit()
method to return an RPC session.source
, eitherexternal
orinternal
. For worker deployed users, this is still set via theX-Starbase-Source
header.cache
boolean property - works is set totrue
and source isexternal
. For worker deployed users, this is still set via theX-Starbase-Cache
header istrue
.context
property, which is what the RLS rules use. For worker deployed users, decodes the JWT and puts in the payload data. But for custom implementation, they can get the context from wherever they want.external
property, which accepts aExternalDatabaseSource
type, which has all of our providers nicely typed.expireCache
logic has been moved into the internal handler.role
, of eitheradmin
orclient
. Admin role does not apply allow list or RLS rules. For the deployed worker, this is set by observing the env api keys which are set.Some additional notes / details:
new_sqlite_classes = ["StarbaseDBDurableObject"]
? Do we have to tag the migration somehow?source
on the URL when connecting via websocket... Shouldn't this be per query on the message level?I've roughly tested things work.. but with no testing I'm conscious this is a big PR with lots of room to go wrong 👀