Summary

Encode non-string query parameters as strings

Review Request #10685 — Created Sept. 6, 2019 and submitted Jan. 7, 2020, 12:46 a.m.

Information

Owner

brennie

Repository

RBTools

Branch

release-1.0.x

Bugs

Depends On

Blocks

10695

Reviewers

Groups

rbtools

People

Description

There are a few cases where we are passing non-string arguments to
methods that make API calls, which was fine in the past. However, we are
now assuming that all arguments are strings and erroring out if they are
not.

Instead, we now make a best-case attempt to serialize values to strings
and produces a better error message when this process fails.

Testing Done

Confirmed the following hold true on Python versions 2.7, 3.5, 3.6, and
3.7:

With this patch applied, rbt status prints the list of status, where
previously it would throw an exception due to attempting to stringify
non-string values.
With this entire patch stack (/r/10695, /r/10696, and /r/10697),
rbt post is able to create review requests with commit history.

Commits

Summary	ID	Author
Encode non-string query parameters as strings There are a few cases where we are passing non-string arguments to methods that make API calls, which was fine in the past. However, we are now assuming that all arguments are strings and erroring out if they are not. Instead, we now make a best-case attempt to serialize values to strings and produces a better error message when this process fails. Testing done: Confirmed the following hold on Python verisons 2.7, 3.5, 3.6, and 3.7: - With this patch applied, `rbt status` prints the list of status, where previously it would throw an exception due to attempting to stringify non-string values. - With this entire patch stack (/r/10695, /r/10696, and /r/10697), `rbt post` is able to create review requests with commit history.	6e6c34885ada13f65fed674d12833b0051c95e39	Barret Rennie

Issues

Description	From	Last Updated
I wonder if instead we should fix this during the request building, instead of here, so that we don't have …	chipx86	Sept. 6, 2019, 2:05 p.m.
Also, I want to see testing on all supported Python versions (2.7, 3.5-3.7).	chipx86	Sept. 6, 2019, 2:05 p.m.
Can you expand on this, to better define when the behavior changed?	chipx86	Sept. 6, 2019, 2:05 p.m.
Wrapping in Testing Done is wrong. "hold" is probably not the right word there.	chipx86	Sept. 7, 2019, 3:58 p.m.
This should be on release-1.0.x.	chipx86	Sept. 8, 2019, 9:41 a.m.
Can you add unit tests for all the encoding possibilities?	chipx86	Sept. 8, 2019, 10:27 a.m.
In "Testing Done", "Confirmed the following hold true" is still pretty weird phrasing. How about just "Confirmed the following on …	david	Sept. 27, 2019, 2:04 p.m.
How about also adding encode_url_key, which forces to Unicode and does the replace()? That'll simplify this and make it a …	chipx86	Sept. 8, 2019, 9:41 a.m.
Missing Returns and Raises.	chipx86	Sept. 8, 2019, 9:41 a.m.
Okay, so I ran into this with Python porting work before. long is also valid -- but only on Python …	chipx86	Sept. 8, 2019, 10:27 a.m.
E999 SyntaxError: invalid syntax	reviewbot	Sept. 27, 2019, 2:08 p.m.
E113 unexpected indentation	reviewbot	Sept. 27, 2019, 2:08 p.m.
F841 local variable 'request' is assigned to but never used	reviewbot	Sept. 27, 2019, 2:14 p.m.

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

I wonder if instead we should fix this during the request building, instead of here, so that we don't have to make a dozen similar changes (and so that customers using the API don't have to either).

The issue has been resolved. Show all issues

Can you expand on this, to better define when the behavior changed?

The issue has been resolved. Show all issues

Also, I want to see testing on all supported Python versions (2.7, 3.5-3.7).

Summary:

Pass query args as strings in rbt status

Encode non-string query parameters as strings

Description:

~		The status command was passing the repository ID as an integer to
~		`get_review_requests()`, which was fine in the past, but it requires all
~		argument values to be strings.
	~	There are a few cases where we are passing non-string arguments to
	~	methods that make API calls, which was fine in the past. However, we are
	~	now assuming that all arguments are strings and erroring out if they are
	+	not.
	+
	+	Instead, we now make a best-case attempt to serialize values to strings
	+	and produces a better error message when this process fails.

Testing Done:

~		With this patch applied, rbt status works.
	~	Confirmed the following hold on Python verisons 2.7, 3.5, 3.6, and 3.7:
	+	- With this patch applied, `rbt status` prints the list of status, where
	+	previously it would throw an exception due to attempting to stringify
	+	non-string values.
	+	- With this entire patch stack (/r/10695, /r/10696, and /r/10697), `rbt`
	+	post is able to create review requests with commit history.

Commits:

	Summary	ID	Author
	Pass query args as strings in rbt status The status command was passing the repository ID as an integer to `get_review_requests()`, which was fine in the past, but it requires all argument values to be strings. Testing done: With this patch applied, rbt status works.	744b98f0b219c78b4dbb6dd2c2ccf5db5b61613e	Barret Rennie
	Encode non-string query parameters as strings There are a few cases where we are passing non-string arguments to methods that make API calls, which was fine in the past. However, we are now assuming that all arguments are strings and erroring out if they are not. Instead, we now make a best-case attempt to serialize values to strings and produces a better error message when this process fails. Testing done: Confirmed the following hold on Python verisons 2.7, 3.5, 3.6, and 3.7: - With this patch applied, `rbt status` prints the list of status, where previously it would throw an exception due to attempting to stringify non-string values. - With this entire patch stack (/r/10695, /r/10696, and /r/10697), `rbt post` is able to create review requests with commit history.	58d0bb153548686a6c896d6b9c4a99219ce9eac7	Barret Rennie

Diff:

Revision 2 (+66 -2)

Show changes

rbtools/api/request.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

Wrapping in Testing Done is wrong.

"hold" is probably not the right word there.

The issue has been resolved. Show all issues
```
This should be on release-1.0.x.
```

The issue has been resolved. Show all issues

Can you add unit tests for all the encoding possibilities?

rbtools/api/request.py (Diff revision 2)

The issue has been resolved. Show all issues

How about also adding encode_url_key, which forces to Unicode and does the replace()? That'll simplify this and make it a little nicer to maintain.

rbtools/api/request.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing Returns and Raises.
```

rbtools/api/request.py (Diff revision 2)

The issue has been resolved. Show all issues

Okay, so I ran into this with Python porting work before. long is also valid -- but only on Python 2.x. Fortunately, six has our back. So we actually want:

`python if isinstance(value, six.integer_types + (float,)):

Testing Done:: ~
Confirmed the following hold on Python verisons 2.7, 3.5, 3.6, and 3.7:
~ - With this patch applied, rbt status prints the list of status, where
~ previously it would throw an exception due to attempting to stringify
~ non-string values.
~ - With this entire patch stack (/r/10695, /r/10696, and /r/10697), rbt
~
Confirmed the following hold true on Python versions 2.7, 3.5, 3.6, and
~ 3.7:
~
~
With this patch applied, rbt status prints the list of status, where
previously it would throw an exception due to attempting to stringify
non-string values.
~
With this entire patch stack (/r/10695, /r/10696, and /r/10697),
rbt post is able to create review requests with commit history.
- post is able to create review requests with commit history.

Looks like you marked Christian's issues as fixed but haven't posted a new revision.

The issue has been resolved. Show all issues

In "Testing Done", "Confirmed the following hold true" is still pretty weird phrasing. How about just "Confirmed the following on Python versions ..."?

Commits:

	Summary	ID	Author
	Encode non-string query parameters as strings There are a few cases where we are passing non-string arguments to methods that make API calls, which was fine in the past. However, we are now assuming that all arguments are strings and erroring out if they are not. Instead, we now make a best-case attempt to serialize values to strings and produces a better error message when this process fails. Testing done: Confirmed the following hold on Python verisons 2.7, 3.5, 3.6, and 3.7: - With this patch applied, `rbt status` prints the list of status, where previously it would throw an exception due to attempting to stringify non-string values. - With this entire patch stack (/r/10695, /r/10696, and /r/10697), `rbt post` is able to create review requests with commit history.	58d0bb153548686a6c896d6b9c4a99219ce9eac7	Barret Rennie
	Encode non-string query parameters as strings There are a few cases where we are passing non-string arguments to methods that make API calls, which was fine in the past. However, we are now assuming that all arguments are strings and erroring out if they are not. Instead, we now make a best-case attempt to serialize values to strings and produces a better error message when this process fails. Testing done: Confirmed the following hold on Python verisons 2.7, 3.5, 3.6, and 3.7: - With this patch applied, `rbt status` prints the list of status, where previously it would throw an exception due to attempting to stringify non-string values. - With this entire patch stack (/r/10695, /r/10696, and /r/10697), `rbt post` is able to create review requests with commit history.	f7a53e71fec932918554fda1e20da428b12e82b5	Barret Rennie

Diff:

Revision 3 (+196 -6)

Show changes

	rbtools/api/request.py
	rbtools/api/tests.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/api/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E999 SyntaxError: invalid syntax
```
rbtools/api/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E113 unexpected indentation
```

Commits:

	Summary	ID	Author
	Encode non-string query parameters as strings There are a few cases where we are passing non-string arguments to methods that make API calls, which was fine in the past. However, we are now assuming that all arguments are strings and erroring out if they are not. Instead, we now make a best-case attempt to serialize values to strings and produces a better error message when this process fails. Testing done: Confirmed the following hold on Python verisons 2.7, 3.5, 3.6, and 3.7: - With this patch applied, `rbt status` prints the list of status, where previously it would throw an exception due to attempting to stringify non-string values. - With this entire patch stack (/r/10695, /r/10696, and /r/10697), `rbt post` is able to create review requests with commit history.	f7a53e71fec932918554fda1e20da428b12e82b5	Barret Rennie
	Encode non-string query parameters as strings There are a few cases where we are passing non-string arguments to methods that make API calls, which was fine in the past. However, we are now assuming that all arguments are strings and erroring out if they are not. Instead, we now make a best-case attempt to serialize values to strings and produces a better error message when this process fails. Testing done: Confirmed the following hold on Python verisons 2.7, 3.5, 3.6, and 3.7: - With this patch applied, `rbt status` prints the list of status, where previously it would throw an exception due to attempting to stringify non-string values. - With this entire patch stack (/r/10695, /r/10696, and /r/10697), `rbt post` is able to create review requests with commit history.	295e320552c17e18388150b902fb9861a9e22f1d	Barret Rennie

Diff:

Revision 4 (+196 -6)

Show changes

	rbtools/api/request.py
	rbtools/api/tests.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/api/tests.py (Diff revision 4)
The issue has been resolved. Show all issues
```
F841 local variable 'request' is assigned to but never used
```

Commits:

	Summary	ID	Author
	Encode non-string query parameters as strings There are a few cases where we are passing non-string arguments to methods that make API calls, which was fine in the past. However, we are now assuming that all arguments are strings and erroring out if they are not. Instead, we now make a best-case attempt to serialize values to strings and produces a better error message when this process fails. Testing done: Confirmed the following hold on Python verisons 2.7, 3.5, 3.6, and 3.7: - With this patch applied, `rbt status` prints the list of status, where previously it would throw an exception due to attempting to stringify non-string values. - With this entire patch stack (/r/10695, /r/10696, and /r/10697), `rbt post` is able to create review requests with commit history.	295e320552c17e18388150b902fb9861a9e22f1d	Barret Rennie
	Encode non-string query parameters as strings There are a few cases where we are passing non-string arguments to methods that make API calls, which was fine in the past. However, we are now assuming that all arguments are strings and erroring out if they are not. Instead, we now make a best-case attempt to serialize values to strings and produces a better error message when this process fails. Testing done: Confirmed the following hold on Python verisons 2.7, 3.5, 3.6, and 3.7: - With this patch applied, `rbt status` prints the list of status, where previously it would throw an exception due to attempting to stringify non-string values. - With this entire patch stack (/r/10695, /r/10696, and /r/10697), `rbt post` is able to create review requests with commit history.	6e6c34885ada13f65fed674d12833b0051c95e39	Barret Rennie

Diff:

Revision 5 (+244 -6)

Show changes

	rbtools/api/request.py
	rbtools/api/tests.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:: This was posted while master was checked out because it was originally posted as a DVCS change and release-1.0.x cannot post to dvcs changes.
Branch:: master
release-1.0.x

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-2.0.x (7b7188e)