feat: Support for using multiple databases in datastore #1090
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
Adds databaseId to the request options, client options and code for carrying over the client options to the request options as well as a test to make sure it actually works.
product-auto-label
bot
added
size: s
The test should include additional checks to ensure get is running the way it is supposed to.
product-auto-label
bot
added
size: m
Since we are saving things to another datastore we need to delete the data from that datastore.
The user needs a way to get the database Id from the client so that they can keep track of their clients more easily.
Ensure that the database id gets passed all the way down to the generated layer and gets included in the request.
We should check for the existance of datastore.options so that the code doesn’t fail if they are not provided at all.
danieljbruce
changed the title
The test on kokoro is failing because in that environment a project id isn’t defined. The solution is to eliminate the get request so that a call never reaches the backend and initialize the gapic client so it can be mocked out and then catch any call that reaches the mock.
Add docs comments for newly added functions and add a test that ensures setDatabaseId works correctly.
Don’t use the same string multiple times. When the database name we are using for testing changes then we should only have to make that change in one place.
This was a typo. We want to test against the database we were using before
Add the getRequestWithDatabaseId function and change it so that it can easily be used in multiple places and the tests still pass.
The request options should contain databaseId so rename the function as appropriate.
This commit contains a comment in all the places we want to add the reqOpts change.
Add a warning because the way the assert checking was done before, the test suite was not receiving any indication of which test was failing which makes debugging really hard.
Add request ids back in all the places they are required
Add addDatabaseIdToRequest to the transaction test framework so that it can be used in the tests.
Mock out add database id for the tests in createReadStream that correspond to createReadStream.
Add the addDatabaseIdToRequest mock to the tests for every runQueryStream test so that the tests pass.
addDatabaseIdToRequest should be a default function in datastore object of request so that we don’t have to mock it out in many different places.
This reverts commit a3bdb92.
This reverts commit 7b5f902.
Imports that were used in old mocks no longer exist in the file so should be removed.
Parameterized testing can be added to the test/index file to test to see how the tests behave with databaseId set to the second database and without this setting too.
Parameterized testing should be used on transaction.ts. This commit adds the testing with async.
Tests need to run on the default database as well as the secondary database. This commit ensures that they do.
Parameterized testing for the system tests will ensure that tests run with the default database as well as with the secondary database.
The intent of the datastore variable in the new tests is to be the default database so that we are able to see if reads/writes affect the default or secondary database in these tests. They should not use the parameterized version of datastore.
product-auto-label
bot
added
size: l
danieljbruce
commented
Sep 20, 2023
test/transaction.ts
Outdated
| @@ -50,19 +52,24 @@ const fakePfy = Object.assign({}, pfy, { | |||
| }, | |||
| }); | |||
|
|
|||
| async.each( | |||
| [{}, {databaseId: SECOND_DATABASE_ID}], | |||
| (clientOptions: DatastoreOptions) => { | |||
| describe('Transaction', () => { | |||
There was a problem hiding this comment.
When we run the linter, this block will become indented and the PR will be hard to read.
There was a problem hiding this comment.
oh, there's a setting in the GitHub interface to do just that. Clicking the small cog icon when in the "Files Changed" tab will open the popup where it's possible to select & click on "Hide whitespace > Apply and reload" option. Using that option will give you just the same type of view you have right now after you run the linter.
It's also a git command line option: git diff -w
kolea2
suggested changes
Sep 20, 2023
Now that the tests don’t need to mock out adding database id function, we can inline the code that adds the database id to the request options.
product-auto-label
bot
added
size: m
kolea2
suggested changes
Sep 21, 2023
src/request.ts
Outdated
| @@ -1155,6 +1158,7 @@ export interface SharedQueryOptions { | |||
| }; | |||
| } | |||
| export interface RequestOptions extends SharedQueryOptions { | |||
| databaseId?: string; | |||
There was a problem hiding this comment.
discussed offline, will move databaseId to shared query options to match proto: example https://github.com/googleapis/googleapis/blob/master/google/datastore/v1/datastore.proto#L208-L234
Parameterized tests should have a different namespace for the second set of tests for sure so we add this prefix.
The databaseId should be moved to shared query options as discussed.
Linting was broken so that the PR could be read. Ran the linter again.
product-auto-label
bot
added
size: xl
kolea2
approved these changes
Sep 28, 2023
danieljbruce
added
the
owlbot:run
gcf-owl-bot
bot
removed
the
owlbot:run
This was referenced Sep 14, 2024
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.
This PR will add multi db support so that datastore users can interact with multiple databases.