Summary

Fix up some string types.

Review Request #12106 — Created March 7, 2022 and submitted March 14, 2022, 8:44 a.m.

Information

Owner

david

Repository

Review Board

Branch

release-5.0.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This change fixes up a handful of issues related to string types. Some

of these are more important than others.
For a couple places, we had code deep in the diff rendering which was

creating a SafeString. Because we have caching at many levels in the

diff viewer, these values would end up trying to go through pickle to

store the rendered data. When pickle encounters a value that it doesn't

understand, it tries to call str() on it. With the older Django

version, this would correctly convert, but with the modern Django,

calling str() on a SafeString would return the SafeString again.

This would just end up infinitely recursing inside of the pickle

machinery. There are two places where this affects. For the first, the

fix is to remove some old mark_safe calls. For the second (where we

were rendering a template), the solution is to explicitly convert to

str by using a slice trick.
Next, the HTTP digest authentication backend was calling MD5, but

passing in an str instead of bytes. We also had a handful of other

places where there was some confusion about bytes/str which was found

via static analysis.
Finally, a bunch of unit tests were passing in int types for the

revision when creating test filediffs. These do end up getting cast to

str, but since we say the type of the argument is str, it's more correct

to pass that in directly.

Testing Done


Created a review request with a diff and was able to successfully load

  the diff viewer page.
Ran unit tests.

Commits

Summary	ID
Fix up some string types. This change fixes up a handful of issues related to string types. Some of these are more important than others. For a couple places, we had code deep in the diff rendering which was creating a `SafeString`. Because we have caching at many levels in the diff viewer, these values would end up trying to go through pickle to store the rendered data. When pickle encounters a value that it doesn't understand, it tries to call `str()` on it. With the older Django version, this would correctly convert, but with the modern Django, calling `str()` on a `SafeString` would return the `SafeString` again. This would just end up infinitely recursing inside of the pickle machinery. There are two places where this affects. For the first, the fix is to remove some old `mark_safe` calls. For the second (where we were rendering a template), the solution is to explicitly convert to `str` by using a slice trick. Next, the HTTP digest authentication backend was calling MD5, but passing in an `str` instead of `bytes`. We also had a handful of other places where there was some confusion about bytes/str which was found via static analysis. Finally, a bunch of unit tests were passing in `int` types for the revision when creating test filediffs. These do end up getting cast to str, but since we say the type of the argument is str, it's more correct to pass that in directly. Testing Done: - Created a review request with a diff and was able to successfully load the diff viewer page. - Ran unit tests.	86d3e35c2eb070ba7f85322b7a2f68338915c9ff

Summary

Fix up some string types.

This change fixes up a handful of issues related to string types. Some of these are more important than others. For a couple places, we had code deep in the diff rendering which was creating a `SafeString`. Because we have caching at many levels in the diff viewer, these values would end up trying to go through pickle to store the rendered data. When pickle encounters a value that it doesn't understand, it tries to call `str()` on it. With the older Django version, this would correctly convert, but with the modern Django, calling `str()` on a `SafeString` would return the `SafeString` again. This would just end up infinitely recursing inside of the pickle machinery. There are two places where this affects. For the first, the fix is to remove some old `mark_safe` calls. For the second (where we were rendering a template), the solution is to explicitly convert to `str` by using a slice trick. Next, the HTTP digest authentication backend was calling MD5, but passing in an `str` instead of `bytes`. We also had a handful of other places where there was some confusion about bytes/str which was found via static analysis. Finally, a bunch of unit tests were passing in `int` types for the revision when creating test filediffs. These do end up getting cast to str, but since we say the type of the argument is str, it's more correct to pass that in directly. Testing Done: - Created a review request with a diff and was able to successfully load the diff viewer page. - Ran unit tests.

86d3e35c2eb070ba7f85322b7a2f68338915c9ff

Issues

Description	From	Last Updated
I saw the bit in the description about this, but I really don't like it. This is a valid part …	chipx86	March 7, 2022, 10:18 p.m.

flake8 passed.

JSHint passed.

reviewboard/diffviewer/tests/test_diffutils.py (Diff revision 1)

The issue has been resolved. Show all issues

I saw the bit in the description about this, but I really don't like it. This is a valid part of the contract. We should be allowed to do this, and the tool is just wrong. I don't want to cater to a misinformed tool. What's reporting the error?

david March 7, 2022, 9:52 p.m.

The thing which I've been playing with is pyright, but I figured given that create_filediff says source_revision should be str that this was a correct fix. I'm happy to undo the source_revision changes for Revision objects (though I think it's still correct for the changes that change int -> str).

chipx86 March 13, 2022, 11:25 p.m.
```
Yeah int -> str is absolutely correct.
```

Description:

		This change fixes up a handful of issues related to string types. Some
		of these are more important than others.

		For a couple places, we had code deep in the diff rendering which was
		creating a `SafeString`. Because we have caching at many levels in the
		diff viewer, these values would end up trying to go through pickle to
		store the rendered data. When pickle encounters a value that it doesn't
		understand, it tries to call `str()` on it. With the older Django
		version, this would correctly convert, but with the modern Django,
		calling `str()` on a `SafeString` would return the `SafeString` again.
		This would just end up infinitely recursing inside of the pickle
		machinery. There are two places where this affects. For the first, the
		fix is to remove some old `mark_safe` calls. For the second (where we
		were rendering a template), the solution is to explicitly convert to
		`str` by using a slice trick.

		Next, the HTTP digest authentication backend was calling MD5, but
		passing in an `str` instead of `bytes`. We also had a handful of other
		places where there was some confusion about bytes/str which was found
		via static analysis.

~		Finally, a bunch of unit tests were passing in various types of objects
~		(int, Revision) when creating test filediffs. This would end up getting
~		cast appropriately, but was tripping up some tooling. I've changed these
~		to explicitly pass in the strings.
	~	Finally, a bunch of unit tests were passing in `int` types for the
	~	revision when creating test filediffs. These do end up getting cast to
	~	str, but since we say the type of the argument is str, it's more correct
	~	to pass that in directly.

Commits:

	Summary	ID
	Fix up some string types. This change fixes up a handful of issues related to string types. Some of these are more important than others. For a couple places, we had code deep in the diff rendering which was creating a `SafeString`. Because we have caching at many levels in the diff viewer, these values would end up trying to go through pickle to store the rendered data. When pickle encounters a value that it doesn't understand, it tries to call `str()` on it. With the older Django version, this would correctly convert, but with the modern Django, calling `str()` on a `SafeString` would return the `SafeString` again. This would just end up infinitely recursing inside of the pickle machinery. There are two places where this affects. For the first, the fix is to remove some old `mark_safe` calls. For the second (where we were rendering a template), the solution is to explicitly convert to `str` by using a slice trick. Next, the HTTP digest authentication backend was calling MD5, but passing in an `str` instead of `bytes`. We also had a handful of other places where there was some confusion about bytes/str which was found via static analysis. Finally, a bunch of unit tests were passing in various types of objects (int, Revision) when creating test filediffs. This would end up getting cast appropriately, but was tripping up some tooling. I've changed these to explicitly pass in the strings. Testing Done: - Created a review request with a diff and was able to successfully load the diff viewer page. - Ran unit tests.	2078677af1950aea265e4fd93ae45a3d8471dfdf
	Fix up some string types. This change fixes up a handful of issues related to string types. Some of these are more important than others. For a couple places, we had code deep in the diff rendering which was creating a `SafeString`. Because we have caching at many levels in the diff viewer, these values would end up trying to go through pickle to store the rendered data. When pickle encounters a value that it doesn't understand, it tries to call `str()` on it. With the older Django version, this would correctly convert, but with the modern Django, calling `str()` on a `SafeString` would return the `SafeString` again. This would just end up infinitely recursing inside of the pickle machinery. There are two places where this affects. For the first, the fix is to remove some old `mark_safe` calls. For the second (where we were rendering a template), the solution is to explicitly convert to `str` by using a slice trick. Next, the HTTP digest authentication backend was calling MD5, but passing in an `str` instead of `bytes`. We also had a handful of other places where there was some confusion about bytes/str which was found via static analysis. Finally, a bunch of unit tests were passing in `int` types for the revision when creating test filediffs. These do end up getting cast to str, but since we say the type of the argument is str, it's more correct to pass that in directly. Testing Done: - Created a review request with a diff and was able to successfully load the diff viewer page. - Ran unit tests.	86d3e35c2eb070ba7f85322b7a2f68338915c9ff

Diff:

Revision 2 (+208 -234)

Show changes

	reviewboard/accounts/backends/http_digest.py
	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/renderers.py
	reviewboard/diffviewer/tests/test_diffutils.py
	reviewboard/diffviewer/tests/test_forms.py
	reviewboard/hostingsvcs/gitlab.py
	reviewboard/notifications/tests/test_webhooks.py
	reviewboard/reviews/fields.py
	5 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to django-3.2 (dcd61ac)