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.

Sign up for GitHub

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

Jump to bottom

perf: use prettyPrint=false by default #28

Merged
merged 2 commits into from Sep 21, 2020
Merged

perf: use prettyPrint=false by default #28

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1672355
perf: use prettyPrint=false by default
tswast Sep 16, 2020
5e98030
Merge branch 'master' into b168522192-prettyPrint=false
busunkim96 Sep 16, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
8 changes: 6 additions & 2 deletions google/cloud/_http.py
View file
Open in desktop
Expand Up @@ -211,8 +211,12 @@ def build_api_url(
)

query_params = query_params or {}
if query_params:
url += "?" + urlencode(query_params, doseq=True)

if "prettyPrint" not in query_params:
Copy link

Choose a reason for hiding this comment

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

Trying to think of cases where this could alter requests we're not anticipating and may have issue with the prettyprint param.

  • will this interfere with media uploads? Mainly thinking about the resumable upload scenario.
  • will this interfere with oauth negotiation?

Copy link
Contributor

Choose a reason for hiding this comment

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

We could make a pre-release and verify that the tests for other libs that depend on google-cloud-core still pass. (The pre-release can also be given to the customer to pin explicitly).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will this interfere with media uploads? Mainly thinking about the resumable upload scenario.

Since google-resumable-media uses "requests" directly, this shouldn't affect it.

I've tested these changes locally with the BigQuery system & sample tests, which all pass and do several load jobs.

I'm trying to test locally with the Storage system tests, but seem to be missing some setup steps around their encryption-related tests.

will this interfere with oauth negotiation?

No, since google-auth uses "requests" directly. It doesn't go through this library. Even if it did, I believe they've actually moved to a One Platform API for fetching tokens now. (Guessing based on the new-ish oauth2.googleapis.com endpoint)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created a pre-release at #31

query_params = query_params.copy()
query_params["prettyPrint"] = "false"

url += "?" + urlencode(query_params, doseq=True)

return url

Expand Down
13 changes: 10 additions & 3 deletions tests/unit/test__http.py
View file
Open in desktop
Expand Up @@ -184,9 +184,16 @@ def test_build_api_url_no_extra_query_params(self):
client = object()
conn = self._make_mock_one(client)
# Intended to emulate self.mock_template
URI = "/".join([conn.API_BASE_URL, "mock", conn.API_VERSION, "foo"])
URI = "/".join([conn.API_BASE_URL, "mock", conn.API_VERSION, "foo?prettyPrint=false"])
self.assertEqual(conn.build_api_url("/foo"), URI)

def test_build_api_url_w_pretty_print_query_params(self):
client = object()
conn = self._make_mock_one(client)
uri = conn.build_api_url("/foo", {"prettyPrint": "true"})
URI = "/".join([conn.API_BASE_URL, "mock", conn.API_VERSION, "foo?prettyPrint=true"])
self.assertEqual(uri, URI)

def test_build_api_url_w_extra_query_params(self):
from six.moves.urllib.parse import parse_qs
from six.moves.urllib.parse import urlsplit
Expand Down Expand Up @@ -319,7 +326,7 @@ def test_api_request_defaults(self):
"User-Agent": conn.user_agent,
CLIENT_INFO_HEADER: conn.user_agent,
}
expected_url = "{base}/mock/{version}{path}".format(
expected_url = "{base}/mock/{version}{path}?prettyPrint=false".format(
base=conn.API_BASE_URL, version=conn.API_VERSION, path=path
)
http.request.assert_called_once_with(
Expand Down Expand Up @@ -481,7 +488,7 @@ def test_api_request_w_timeout(self):
"User-Agent": conn.user_agent,
CLIENT_INFO_HEADER: conn.user_agent,
}
expected_url = "{base}/mock/{version}{path}".format(
expected_url = "{base}/mock/{version}{path}?prettyPrint=false".format(
base=conn.API_BASE_URL, version=conn.API_VERSION, path=path
)
http.request.assert_called_once_with(
Expand Down