feat: Determine URL protocol based on hostname by vchudnov-g · Pull Request #900 · googleapis/gapic-generator-python

vchudnov-g · 2021-05-20T01:37:55Z

This will make connections to localhost use HTTP. This is insecure but useful for testing. In a future change, we will allow setting transport options at client instantiation time.

codecov · 2021-05-20T01:38:57Z

Codecov Report

Merging #900 (f6e383f) into master (7c185e8) will not change coverage.
The diff coverage is 100.00%.

❗ Current head f6e383f differs from pull request most recent head 0141d94. Consider uploading reports for the commit 0141d94 to get more accurate results

@@            Coverage Diff            @@
##            master      #900   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           26        27    +1     
  Lines         1608      1697   +89     
  Branches       328       347   +19     
=========================================
+ Hits          1608      1697   +89

Impacted Files	Coverage Δ
gapic/samplegen_utils/types.py	`100.00% <ø> (ø)`
gapic/samplegen_utils/utils.py	`100.00% <ø> (ø)`
gapic/utils/case.py	`100.00% <ø> (ø)`
gapic/utils/reserved_names.py	`100.00% <ø> (ø)`
gapic/generator/generator.py	`100.00% <100.00%> (ø)`
gapic/samplegen/samplegen.py	`100.00% <100.00%> (ø)`
gapic/schema/api.py	`100.00% <100.00%> (ø)`
gapic/schema/metadata.py	`100.00% <100.00%> (ø)`
gapic/schema/wrappers.py	`100.00% <100.00%> (ø)`
gapic/utils/__init__.py	`100.00% <100.00%> (ø)`
... and 3 more

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 f14ceba...0141d94. Read the comment docs.

This will make connections to `localhost` use HTTP. This is insecure but useful for testing. In a future change, we will allow setting transport options at client instantiation time.

busunkim96

Could you add a unit test that exercises protocol_for?

busunkim96 · 2021-05-20T14:53:17Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

+         Tracking issue:
+         https://github.com/googleapis/gapic-generator-python/issues/901
+#}
+def protocol_for(host: str) -> str:


(Since It looks like this function shouldn't be called by end users)

Suggested change

def protocol_for(host: str) -> str:

def _protocol_for(host: str) -> str:

busunkim96 · 2021-05-20T15:00:29Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

+    LOCALHOST='localhost'
+    LOCALHOST_LEN=len(LOCALHOST)
+
+    if host.startswith(LOCALHOST) and (len(host)==LOCALHOST_LEN or host[LOCALHOST_LEN]==":"):
+        return 'http'


I have a slight preference for regex match here instead (especially since the docstring already seems to describe this in terms of regex :)) https://docs.python.org/3/library/re.html

Done. It's more compact. The performance impact shouldn't be too bad, especially as I memoize the regex compilation.

- mark it private with leading underscore - use regex for host matching

vchudnov-g

Made the code changes. Still need to implement the unit test. (Need to figure out how to run the tests locally first!)

vchudnov-g · 2021-05-20T18:49:42Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

+         Tracking issue:
+         https://github.com/googleapis/gapic-generator-python/issues/901
+#}
+def protocol_for(host: str) -> str:


vchudnov-g · 2021-05-20T19:11:00Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

+    LOCALHOST='localhost'
+    LOCALHOST_LEN=len(LOCALHOST)
+
+    if host.startswith(LOCALHOST) and (len(host)==LOCALHOST_LEN or host[LOCALHOST_LEN]==":"):
+        return 'http'


Done. It's more compact. The performance impact shouldn't be too bad, especially as I memoize the regex compilation.

vam-google · 2021-05-26T06:26:00Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

+    """Determine the URL protocol to use for connecting to the given host.
+
+    This function returns 'http' for hosts that match
+    `^localhost(:.*)?$` and `https` for all others. This is intended


Do we have anything like this in the other languages? Overall this looks suspicious I would prefer to not have something like this. Determining protocol from host name does not seem reliable and also we are modifying production code for testing purposes. Also what if the host is specified as an IP (land specifically 127.0.0.1)?

Yes. This was discussed in an internal doc; I'll ping you the link. We're specifically looking for localhost because yes, this is a temporary special case to allow testing (see #901 for the long-term solution)

vam-google · 2021-05-26T06:28:36Z

gapic/templates/%namespace/%name_%version/%sub/services/%service/transports/rest.py.j2

+        str: The protocol (`http` or `https`) to use for connecting to `host`.
+    """
+    if not hasattr(_protocol_for, 'regex'):
+        _protocol_for.regex = re.compile("^localhost(:.*)?$")


Will this regex be executed for every call made? If yes, it will have some performance implications (not big probably, but here we sacrifice production code to support rare testing case, which is suboptimal).

The compilation of the regex will only happen once because I have it memoized via this attribute.

The matching will happen for every call for now, though we could refactor things so the protocol is only computed once per client and stored in an attribute. However, given that the performance hit is small, I'm inclined to think it's preferable to keep things simple.

parthea · 2022-04-09T23:36:36Z

I'm going to close this PR due to it being stale but please feel free to re-open and update it.

vchudnov-g requested review from busunkim96, miraleung and vam-google May 20, 2021 01:37

vchudnov-g requested a review from a team as a code owner May 20, 2021 01:37

google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 20, 2021

vchudnov-g requested a review from software-dov May 20, 2021 01:38

vchudnov-g mentioned this pull request May 20, 2021

Allow setting REST transport client connection options. #901

Closed

vchudnov-g force-pushed the vchudnov-host-protocol branch from 5987173 to 11308f8 Compare May 20, 2021 01:42

feat: Determine URL protocol based on hostname

7dc5ce3

This will make connections to `localhost` use HTTP. This is insecure but useful for testing. In a future change, we will allow setting transport options at client instantiation time.

vchudnov-g force-pushed the vchudnov-host-protocol branch from 11308f8 to 7dc5ce3 Compare May 20, 2021 01:42

vchudnov-g changed the title ~~feat: Determine URL protocol based on hostname.~~ feat: Determine URL protocol based on hostname May 20, 2021

busunkim96 reviewed May 20, 2021

View reviewed changes

chore: Implement code-review changes to protocol_for_host()

f6e383f

- mark it private with leading underscore - use regex for host matching

vchudnov-g commented May 21, 2021

View reviewed changes

vam-google reviewed May 26, 2021

View reviewed changes

software-dov approved these changes Jun 1, 2021

View reviewed changes

miraleung approved these changes Jun 2, 2021

View reviewed changes

Merge branch 'master' into vchudnov-host-protocol

02e5fd3

parthea assigned vchudnov-g Jul 26, 2021

Merge branch 'master' into vchudnov-host-protocol

0141d94

parthea closed this Apr 9, 2022

	def protocol_for(host: str) -> str:
	def _protocol_for(host: str) -> str:

Conversation

vchudnov-g commented May 20, 2021

Uh oh!

codecov bot commented May 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

busunkim96 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vchudnov-g left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parthea commented Apr 9, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented May 20, 2021 •

edited

Loading