Skip to content
This repository was archived by the owner on Feb 13, 2026. It is now read-only.

perf: use prettyPrint=false by default#28

Merged
tswast 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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 6 additions & 2 deletions google/cloud/_http.py
View file
Open in desktop
Original file line numberDiff line numberDiff line change
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:

@shollymanshollymanSep 16, 2020

Copy link
Copy Markdown

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?

@busunkim96busunkim96Sep 16, 2020

Copy link
Copy Markdown
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).

@tswasttswastSep 17, 2020

Copy link
Copy Markdown
ContributorAuthor

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)

@tswasttswastSep 21, 2020

Copy link
Copy Markdown
ContributorAuthor

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
Original file line numberDiff line numberDiff line change
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 DownExpand 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 DownExpand 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