feat: Allow the user to set fallback to rest #1203

danieljbruce · 2023-11-17T22:14:44Z

Change:

Users need to be able to use the rest fallback property. This change allows client library calls to be rest calls instead of grpc calls. They can make this happen by instantiating the client library like so:

new Datastore({namespace, fallback: 'rest'})

This provides a workaround for the connection issue.

pmwisdom · 2023-11-29T21:21:00Z

@danieljbruce Hey there. Is there a reason this was closed?

Currently having to hack around this in my code to support HTTP with the datastore class which isn't ideal.

conventional-commit-lint-gcf · 2023-12-01T18:13:43Z

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

Need to mock out Gapic and compare constructors

…odejs-datastore into use-http-fallback

The harness mocks out the constructor and calls reach the constructor as well as the get function. So now we can see what values get passed into the constructor and get function.

Add a working test that makes sure the rest parameter gets passed down to the gapic layer.

Proper describe blocks make it easier for the code to be arranged so that with and without rest tests can be incorporated into the code.

Remove the datastore mock. It is not necessary.

Make a programmatic link to the data type so that the intent is clearer.

Parameterized tests will allow us to delete half the tests. A new type is created for the test parameters.

Explain the purpose of the class. This will make it useful for testing.

This type is only used once so inline it.

Changes to system tests are not necessary. Remove them.

assert resolves to falsy in tests. See if this solves the issue.

Add console logs and assert checks back in.

This should allow the unit tests to pass in the continuous integration environment.

The title of the describe block should match the file. Remove console logs too.

cindy-peng · 2023-12-05T22:02:13Z

test/gapic-mocks/client-initialization-testing.ts

+ *
+ */
+class FakeDatastoreClient extends DatastoreClient {
+  restParameter: string | undefined;


Hi Dan, just wondering is this "restParameter" expecting a boolean type from Fallback type too? https://github.com/googleapis/nodejs-datastore/pull/1203/files#diff-a2a171449d862fe29692ce031981047d7ab755ae7f84c707aef80701b3ea0c80R100

I changed the type to Fallback here so that the types line up better.

cindy-peng · 2023-12-05T22:15:40Z

src/index.ts

@@ -1912,6 +1913,7 @@ export interface DatastoreOptions extends GoogleAuthOptions {
  apiEndpoint?: string;
  sslCreds?: ChannelCredentials;
  databaseId?: string;
+  fallback?: Fallback;


Maybe this is already commented somewhere else. Do you think we can provide an example how to initiate client library with fallback options?

I added a simple system test for this.

This parameter more specifically describes the change.

This system test should show how the rest parameter is being used.

This allows the test to pass with a simple assertion check.

cindy-peng · 2023-12-06T17:30:05Z

test/gapic-mocks/client-initialization-testing.ts

+     */
+    function compareRequest(
+      request: DatastoreRequest,
+      expectedFallback: string | undefined,


Maybe here change the type to Fallback to be consistent too?

Fallback type is more specific . Use it instead.

…odejs-datastore into use-http-fallback

looker-colmeia · 2023-12-19T21:29:31Z

hey @danieljbruce thanks for this feature, I would like to ask you a couple of things:

you have any estimate of how much percent of query performance in datastore we will loose by enabling fallback to rest?
this feature will use 100% of requests to datastore using rest or it's really some kind of fallback in case of error?

Allow the user to set fallback to rest

bf9c81f

This provides a workaround for the connection issue.

product-auto-label bot added size: xs Pull request size is extra small. api: datastore Issues related to the googleapis/nodejs-datastore API. labels Nov 17, 2023

danieljbruce closed this Nov 20, 2023

danieljbruce reopened this Dec 1, 2023

danieljbruce changed the title ~~Allow the user to set fallback to rest~~ feat: Allow the user to set fallback to rest Dec 1, 2023

danieljbruce marked this pull request as ready for review December 1, 2023 18:14

danieljbruce requested review from a team as code owners December 1, 2023 18:14

Merge branch 'main' into use-http-fallback

778f20e

danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 1, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 1, 2023

danieljbruce added 7 commits December 1, 2023 16:10

Putting together a test that mocks out gapic

b6ce5d9

Need to mock out Gapic and compare constructors

Merge branch 'use-http-fallback' of https://github.com/danieljbruce/n…

99ef146

…odejs-datastore into use-http-fallback

This harness works properly

4b8ab09

The harness mocks out the constructor and calls reach the constructor as well as the get function. So now we can see what values get passed into the constructor and get function.

Add client testing module

60cbaed

Add a working test that makes sure the rest parameter gets passed down to the gapic layer.

Regroup the code so it is better arranged

d066264

Proper describe blocks make it easier for the code to be arranged so that with and without rest tests can be incorporated into the code.

Remove the datastore mock

fa5d14b

Remove the datastore mock. It is not necessary.

Add comments for test helper functions. Add tests for the non rest case.

591ce30

product-auto-label bot added size: m Pull request size is medium. and removed size: xs Pull request size is extra small. labels Dec 4, 2023

danieljbruce added 7 commits December 4, 2023 16:07

Clean up source code change

fa56366

Make a programmatic link to the data type so that the intent is clearer.

Add parameterized test

850781c

Parameterized tests will allow us to delete half the tests. A new type is created for the test parameters.

Document the class for mocking datastore client

09ee75d

Explain the purpose of the class. This will make it useful for testing.

Remove only

115b40a

Remove fallback params type

7f4a02a

This type is only used once so inline it.

Remove changes to system tests

92030db

Changes to system tests are not necessary. Remove them.

Add header

a42e8c6

danieljbruce added 8 commits December 4, 2023 17:27

Check for client existence

e4bf66c

assert resolves to falsy in tests. See if this solves the issue.

Change it back

6faca17

Add console logs and assert checks back in.

Add more debugger logs

54a0889

Add project load logs

f2ad045

console error

b532c8d

Add comment for mocking out project id

2e5700a

This should allow the unit tests to pass in the continuous integration environment.

linting fix

31151be

Correct title of describe block

b3eda27

The title of the describe block should match the file. Remove console logs too.

danieljbruce requested review from kolea2 and cindy-peng December 5, 2023 16:28

cindy-peng reviewed Dec 5, 2023

View reviewed changes

danieljbruce and others added 4 commits December 6, 2023 09:51

Change rest parameter in test to fallback

3f97583

This parameter more specifically describes the change.

Create a simple system test for using rest

09233b5

This system test should show how the rest parameter is being used.

Should check to see if entity is undefined

f009f7b

This allows the test to pass with a simple assertion check.

Merge branch 'main' into use-http-fallback

c9a7153

cindy-peng reviewed Dec 6, 2023

View reviewed changes

cindy-peng approved these changes Dec 6, 2023

View reviewed changes

danieljbruce added 2 commits December 6, 2023 13:00

Specify fallback type instead of string | undefin

b5723a0

Fallback type is more specific . Use it instead.

Merge branch 'use-http-fallback' of https://github.com/danieljbruce/n…

afc8326

…odejs-datastore into use-http-fallback

kolea2 approved these changes Dec 7, 2023

View reviewed changes

danieljbruce added the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 7, 2023

gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Dec 7, 2023

murgatroid99 approved these changes Dec 11, 2023

View reviewed changes

danieljbruce merged commit 8a1fa54 into googleapis:main Dec 11, 2023
15 checks passed

release-please bot mentioned this pull request Dec 11, 2023

chore(main): release 8.3.0 #1184

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow the user to set fallback to rest #1203

feat: Allow the user to set fallback to rest #1203

danieljbruce commented Nov 17, 2023 •

edited

Loading

pmwisdom commented Nov 29, 2023

conventional-commit-lint-gcf bot commented Dec 1, 2023

cindy-peng Dec 5, 2023

danieljbruce Dec 6, 2023

cindy-peng Dec 5, 2023

danieljbruce Dec 6, 2023

cindy-peng Dec 6, 2023

danieljbruce Dec 6, 2023

looker-colmeia commented Dec 19, 2023

feat: Allow the user to set fallback to rest #1203

feat: Allow the user to set fallback to rest #1203

Conversation

danieljbruce commented Nov 17, 2023 • edited Loading

pmwisdom commented Nov 29, 2023

conventional-commit-lint-gcf bot commented Dec 1, 2023

cindy-peng Dec 5, 2023

Choose a reason for hiding this comment

danieljbruce Dec 6, 2023

Choose a reason for hiding this comment

cindy-peng Dec 5, 2023

Choose a reason for hiding this comment

danieljbruce Dec 6, 2023

Choose a reason for hiding this comment

cindy-peng Dec 6, 2023

Choose a reason for hiding this comment

danieljbruce Dec 6, 2023

Choose a reason for hiding this comment

looker-colmeia commented Dec 19, 2023

danieljbruce commented Nov 17, 2023 •

edited

Loading