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

feat: load protos from JSON, grpc-fallback support #749

Merged
merged 4 commits into from
Sep 5, 2019

Conversation

yoshi-automation
Copy link
Contributor

This PR was generated using Autosynth. 🌈

Here's the log from Synthtool:

synthtool > Executing /tmpfs/src/git/autosynth/working_repo/synth.py.
synthtool > Ensuring dependencies.
synthtool > Pulling artman image.
latest: Pulling from googleapis/artman
Digest: sha256:0e6f3a668cd68afc768ecbe08817cf6e56a0e64fcbdb1c58c3b97492d12418a1
Status: Image is up to date for googleapis/artman:latest
synthtool > Cloning googleapis.
synthtool > Running generator for google/firestore/admin/artman_firestore_v1.yaml.
synthtool > Generated code into /home/kbuilder/.cache/synthtool/googleapis/artman-genfiles/js/firestore-admin-v1.
synthtool > Running generator for google/firestore/artman_firestore.yaml.
synthtool > Generated code into /home/kbuilder/.cache/synthtool/googleapis/artman-genfiles/js/firestore-v1beta1.
synthtool > Running generator for google/firestore/artman_firestore_v1.yaml.
synthtool > Generated code into /home/kbuilder/.cache/synthtool/googleapis/artman-genfiles/js/firestore-v1.
synthtool > Replaced '../../package.json' in dev/src/v1/firestore_admin_client.js.
synthtool > Replaced '../../package.json' in dev/src/v1beta1/firestore_client.js.
synthtool > Replaced '../../package.json' in dev/src/v1/firestore_client.js.
synthtool > Replaced 'return this\\._innerApiCalls\\.listen\\(options\\);' in dev/src/v1beta1/firestore_client.js.
synthtool > Replaced 'return this\\._innerApiCalls\\.listen\\(options\\);' in dev/src/v1/firestore_client.js.
.eslintignore
.eslintrc.yml
.github/ISSUE_TEMPLATE/bug_report.md
.github/ISSUE_TEMPLATE/feature_request.md
.github/ISSUE_TEMPLATE/support_request.md
.jsdoc.js
.kokoro/common.cfg
.kokoro/continuous/node10/common.cfg
.kokoro/continuous/node10/docs.cfg
.kokoro/continuous/node10/lint.cfg
.kokoro/continuous/node10/samples-test.cfg
.kokoro/continuous/node10/system-test.cfg
.kokoro/continuous/node10/test.cfg
.kokoro/continuous/node12/common.cfg
.kokoro/continuous/node12/test.cfg
.kokoro/continuous/node8/common.cfg
.kokoro/continuous/node8/test.cfg
.kokoro/docs.sh
.kokoro/lint.sh
.kokoro/presubmit/node10/common.cfg
.kokoro/presubmit/node10/docs.cfg
.kokoro/presubmit/node10/lint.cfg
.kokoro/presubmit/node10/samples-test.cfg
.kokoro/presubmit/node10/system-test.cfg
.kokoro/presubmit/node10/test.cfg
.kokoro/presubmit/node12/common.cfg
.kokoro/presubmit/node12/test.cfg
.kokoro/presubmit/node8/common.cfg
.kokoro/presubmit/node8/test.cfg
.kokoro/presubmit/windows/common.cfg
.kokoro/presubmit/windows/test.cfg
.kokoro/publish.sh
.kokoro/release/docs.cfg
.kokoro/release/docs.sh
.kokoro/release/publish.cfg
.kokoro/samples-test.sh
.kokoro/system-test.sh
.kokoro/test.bat
.kokoro/test.sh
.kokoro/trampoline.sh
.nycrc
.prettierignore
.prettierrc
CODE_OF_CONDUCT.md
CONTRIBUTING.md
LICENSE
README.md
codecov.yaml
renovate.json
samples/README.md
synthtool > Replaced 'https:\\/\\/cloud\\.google\\.com[\\s\\*]*http:\\/\\/(.*)[\\s\\*]*\\)' in dev/src/v1beta1/doc/google/protobuf/doc_timestamp.js.
synthtool > Replaced 'https:\\/\\/cloud\\.google\\.com[\\s\\*]*http:\\/\\/(.*)[\\s\\*]*\\)' in dev/src/v1/doc/google/protobuf/doc_timestamp.js.
synthtool > No replacements made in **/doc/google/protobuf/doc_timestamp.js for pattern toISOString\], maybe replacement is not longer needed?

> core-js@2.6.9 postinstall /tmpfs/src/git/autosynth/working_repo/node_modules/core-js
> node scripts/postinstall || echo "ignore"

Thank you for using core-js ( https://github.com/zloirock/core-js ) for polyfilling JavaScript standard library!

The project needs your help! Please consider supporting of core-js on Open Collective or Patreon: 
> https://opencollective.com/core-js 
> https://www.patreon.com/zloirock 

Also, the author of core-js ( https://github.com/zloirock ) is looking for a good job -)


> protobufjs@6.8.8 postinstall /tmpfs/src/git/autosynth/working_repo/node_modules/protobufjs
> node scripts/postinstall


> @google-cloud/firestore@2.2.9 prepare /tmpfs/src/git/autosynth/working_repo
> npm run compile


> @google-cloud/firestore@2.2.9 compile /tmpfs/src/git/autosynth/working_repo
> tsc -p . && cp -r dev/protos build && cp -r dev/test/fake-certificate.json build/test/fake-certificate.json && cp dev/src/v1beta1/*.json build/src/v1beta1/ && cp dev/src/v1/*.json build/src/v1/ && cp dev/conformance/test-definition.proto build/conformance && cp dev/conformance/test-suite.binproto build/conformance

/tmpfs/src/git/autosynth/working_repo/node_modules/typescript/lib/tsc.js:75634
                throw e;
                ^

TypeError: Cannot read property 'parseDiagnostics' of undefined
    at hasParseDiagnostics (/tmpfs/src/git/autosynth/working_repo/node_modules/typescript/lib/tsc.js:52531:30)
    at grammarErrorOnNode (/tmpfs/src/git/autosynth/working_repo/node_modules/typescript/lib/tsc.js:52552:18)
    at checkAssignmentDeclaration (/tmpfs/src/git/autosynth/working_repo/node_modules/typescript/lib/tsc.js:45692:40)
    at checkBinaryLikeExpression (/tmpfs/src/git/autosynth/working_repo/node_modules/typescript/lib/tsc.js:45658:21)
    at checkBinaryExpression (/tmpfs/src/git/autosynth/working_repo/node_modules/typescript/lib/tsc.js:45514:20)
    at checkExpressionWorker (/tmpfs/src/git/autosynth/working_repo/node_modules/typescript/lib/tsc.js:46238:28)
    at checkExpression (/tmpfs/src/git/autosynth/working_repo/node_modules/typescript/lib/tsc.js:46125:38)
    at checkExpressionStatement (/tmpfs/src/git/autosynth/working_repo/node_modules/typescript/lib/tsc.js:48139:13)
    at checkSourceElementWorker (/tmpfs/src/git/autosynth/working_repo/node_modules/typescript/lib/tsc.js:50067:28)
    at checkSourceElement (/tmpfs/src/git/autosynth/working_repo/node_modules/typescript/lib/tsc.js:49956:17)
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! @google-cloud/firestore@2.2.9 compile: `tsc -p . && cp -r dev/protos build && cp -r dev/test/fake-certificate.json build/test/fake-certificate.json && cp dev/src/v1beta1/*.json build/src/v1beta1/ && cp dev/src/v1/*.json build/src/v1/ && cp dev/conformance/test-definition.proto build/conformance && cp dev/conformance/test-suite.binproto build/conformance`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the @google-cloud/firestore@2.2.9 compile script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/kbuilder/.npm/_logs/2019-09-04T11_15_18_768Z-debug.log
npm ERR! code ELIFECYCLE
npm ERR! errno 1
npm ERR! @google-cloud/firestore@2.2.9 prepare: `npm run compile`
npm ERR! Exit status 1
npm ERR! 
npm ERR! Failed at the @google-cloud/firestore@2.2.9 prepare script.
npm ERR! This is probably not a problem with npm. There is likely additional logging output above.

npm ERR! A complete log of this run can be found in:
npm ERR!     /home/kbuilder/.npm/_logs/2019-09-04T11_15_18_821Z-debug.log

> @google-cloud/firestore@2.2.9 fix /tmpfs/src/git/autosynth/working_repo
> gts fix

synthtool > Cleaned up 2 temporary directories.
synthtool > Wrote metadata to synth.metadata.

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 4, 2019
@alexander-fenster alexander-fenster changed the title [CHANGE ME] Re-generated to pick up changes in the API or client library generator. feat: load protos from JSON, grpc-fallback support Sep 4, 2019
@alexander-fenster alexander-fenster added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 4, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 4, 2019
@alexander-fenster alexander-fenster added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 5, 2019
@kokoro-team kokoro-team removed kokoro:force-run Add this label to force Kokoro to re-run the tests. labels Sep 5, 2019
@alexander-fenster
Copy link
Contributor

Something is wrong here, I need to take a look.

@alexander-fenster alexander-fenster self-assigned this Sep 5, 2019
@codecov
Copy link

codecov bot commented Sep 5, 2019

Codecov Report

Merging #749 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #749   +/-   ##
=======================================
  Coverage   96.46%   96.46%           
=======================================
  Files          20       20           
  Lines        2236     2236           
  Branches      467      467           
=======================================
  Hits         2157     2157           
  Misses         24       24           
  Partials       55       55

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 20a1ab6...181cd1d. Read the comment docs.

@alexander-fenster
Copy link
Contributor

@schmidt-sebastian Just FYI - this PR brings in the new auto-generated code that:

  • loads protos from the pre-compiled JSON file, no more parsing proto files in runtime;
  • adds logic for gRPC-fallback (which may or may not be relevant for Firestore). The idea is that when fallback: true and an instance of OAuth2Client is passed to a client constructor, it will use protobuf-over-HTTP/1 instead of gRPC, as described here.

Since tests pass, I'm pretty confident everything is fine, but would like to have your ✅ here anyway.

Thank you!

# Node.js specific cleanup
subprocess.run(["npm", "install"])
subprocess.run(["npm", "run", "fix"])
os.chdir("dev")
subprocess.run(["npx", "compileProtos", "src"])
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also run https://github.com/googleapis/nodejs-firestore/blob/master/dev/protos/update.sh here? That generates our d.ts typings

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes I guess so (it's unrelated to this particular PR but it's a good place to call any custom scripts after generation).

@schmidt-sebastian
Copy link
Contributor

This LGTM. Just curious if the fallback would also work for streams?

@alexander-fenster
Copy link
Contributor

No, unfortunately it won't work for streaming methods. All other types of calls (unary, LRO, paginatied calls) are supported.

@alexander-fenster alexander-fenster merged commit 6cb9d68 into master Sep 5, 2019
@manwithsteelnerves
Copy link

manwithsteelnerves commented Apr 8, 2021

@alexander-fenster I see some problem when timestamp is processed. While REST call (through fallback as rest) is returning time values as string, protobuf parsers expect it as ITimestamp as per protos.json.

Do you see a way to solve this?
The problem is obvious but I'm not finding a way to convert the date string to ITimestamp instance to by pass the error.

.google.firestore.v1.Document.createTime: object expected

I tried changing the firestore_v1_proto_api.js but its not updating as expected. For sure something i'm missing. It will be of great help if you can help on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants