Summary

Cache Key now takes Git's Raw File URL Mask into account

Review Request #7928 — Created Jan. 30, 2016 and submitted Feb. 22, 2016, 11:04 p.m.

Information

Owner

kschiu

Repository

Review Board

Branch

release-2.5.x

Bugs

4051

Depends On

Reviewers

Groups

reviewboard

People

Description

After initializing a Git repository and adding a bad Raw File URL Mask, the retrieved file is cached. After the Raw File URL Mask is changed to a valid one, the retrieved file is still from the cache because it does not take into consideration the new Raw File URL.

This was fixed by including the raw_file_url into the cache key for the functions: _make_file_exists_cache_key and _make_file_exists_cache_key. By including the raw_file_url into the cache key, the cache key will change whenever the raw file url is changed, such that it will not retrieve stale files from the cache. This change was included in reviewboard/scmtools/models.py.

Testing Done

Added unit tests to check that the cached copy of the file is grabbed when there are no changes to the raw_file_url and to check that a new file is retrieved when the raw_file_url changes.
Added unit test for get_file_exists() function.

Ran whole test suite- all tests pass.

Issues

Description	From	Last Updated
Single quotes for strings.	brennie	Jan. 30, 2016, 12:37 p.m.
The two strings should line up, e.g.: ... = ('http://github.com...' 'reviewboard/....')	brennie	Jan. 30, 2016, 12:37 p.m.
Single quotes for strings.	brennie	Jan. 30, 2016, 12:37 p.m.
Col: 38 E128 continuation line under-indented for visual indent	reviewbot	Jan. 30, 2016, 1:47 p.m.
Col: 38 E128 continuation line under-indented for visual indent	reviewbot	Jan. 30, 2016, 1:47 p.m.
Col: 45 E128 continuation line under-indented for visual indent	reviewbot	Jan. 30, 2016, 1:47 p.m.
Col: 45 E128 continuation line under-indented for visual indent	reviewbot	Jan. 30, 2016, 1:47 p.m.
Col: 80 E501 line too long (83 > 79 characters)	reviewbot	Jan. 30, 2016, 1:47 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Jan. 30, 2016, 1:47 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Jan. 30, 2016, 1:47 p.m.
Col: 80 E501 line too long (89 > 79 characters)	reviewbot	Jan. 30, 2016, 1:47 p.m.
Col: 80 E501 line too long (81 > 79 characters)	reviewbot	Jan. 30, 2016, 1:47 p.m.
Col: 63 W291 trailing whitespace	reviewbot	Jan. 30, 2016, 1:50 p.m.
Col: 28 W291 trailing whitespace	reviewbot	Jan. 30, 2016, 1:50 p.m.
Col: 28 W291 trailing whitespace	reviewbot	Jan. 30, 2016, 1:50 p.m.
Col: 28 W291 trailing whitespace	reviewbot	Jan. 30, 2016, 1:50 p.m.
This can be None, so we need to normalize like above. (Same with the code below.) We're getting to the …	chipx86	Jan. 31, 2016, 10:03 a.m.
How about: """Testing Repository.get_file re-fetches when raw file URL changes"""	chipx86	Jan. 31, 2016, 10:01 a.m.
Here's a way to enhance the test and make it more thorough, self-explanatory, and future-proof: Return a fake result in …	chipx86	Jan. 31, 2016, 10:03 a.m.
Col: 31 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 31, 2016, 10:09 a.m.
Col: 31 E251 unexpected spaces around keyword / parameter equals	reviewbot	Jan. 31, 2016, 10:09 a.m.
Col: 9 E265 block comment should start with '# '	reviewbot	Jan. 31, 2016, 10:09 a.m.
Col: 9 E265 block comment should start with '# '	reviewbot	Jan. 31, 2016, 10:09 a.m.
I'd like to see this have a few more comments, to explain exactly what the order of operations are. Comments …	david	Feb. 3, 2016, 2:33 p.m.

Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/models.py

Summary:

-	Added failing test that checks number of times a file is cached
+	Cache Key now takes Git's Raw File URL Mask into account

reviewboard/scmtools/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Single quotes for strings.
```

reviewboard/scmtools/tests.py (Diff revision 1)

The issue has been resolved. Show all issues

The two strings should line up, e.g.:

... = ('http://github.com...'
       'reviewboard/....')

reviewboard/scmtools/tests.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Single quotes for strings.
```

Bugs:

+	4051

Diff:

Revision 2 (+17 -5)

Show changes

	reviewboard/scmtools/models.py
	reviewboard/scmtools/tests.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/models.py

reviewboard/scmtools/models.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 38
 E128 continuation line under-indented for visual indent
```
reviewboard/scmtools/models.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 38
 E128 continuation line under-indented for visual indent
```
reviewboard/scmtools/models.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 45
 E128 continuation line under-indented for visual indent
```
reviewboard/scmtools/models.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 45
 E128 continuation line under-indented for visual indent
```
reviewboard/scmtools/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (83 > 79 characters)
```
reviewboard/scmtools/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/scmtools/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```
reviewboard/scmtools/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (89 > 79 characters)
```
reviewboard/scmtools/tests.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Col: 80
 E501 line too long (81 > 79 characters)
```

Diff:

Revision 3 (+23 -7)

Show changes

	reviewboard/scmtools/models.py
	reviewboard/scmtools/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/models.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/models.py

reviewboard/scmtools/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 63
 W291 trailing whitespace
```
reviewboard/scmtools/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 28
 W291 trailing whitespace
```
reviewboard/scmtools/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 28
 W291 trailing whitespace
```
reviewboard/scmtools/tests.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Col: 28
 W291 trailing whitespace
```

Diff:

Revision 4 (+23 -7)

Show changes

	reviewboard/scmtools/models.py
	reviewboard/scmtools/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/models.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/models.py

Make sure to follow this guide on writing good summary/description content: https://www.reviewboard.org/docs/codebase/dev/writing-good-descriptions/

I also want to see a unit test for get_file_exists().

kschiu Jan. 31, 2016, 10 a.m.

Should I test get_file_exists() to see how it responds to changes in the raw_file_url?

reviewboard/scmtools/models.py (Diff revision 4)

The issue has been resolved. Show all issues

This can be None, so we need to normalize like above. (Same with the code below.)
We're getting to the point now, though, where this is going to hit the max line length. So how about:

return 'file:...............' % (
    self.pk,
    urlquote(...),
    urlquote(...),
    ...
)


(One per line.)
That'll help make this more readable. (Also, if you switch to single line quotes for this string while you're making this change, it'll help get our codebase more consistent.)

reviewboard/scmtools/tests.py (Diff revision 4)

The issue has been resolved. Show all issues

How about:

"""Testing Repository.get_file re-fetches when raw file URL changes"""

reviewboard/scmtools/tests.py (Diff revision 4)

The issue has been resolved. Show all issues

Here's a way to enhance the test and make it more thorough, self-explanatory, and future-proof:

Return a fake result in your spy_on.
Call and check the output.
Change the fake result.
Call and check that the output remains what it was before. (This ensures that we are in fact grabbing the cached copy, protecting us from test infrastructure failure or other weirdness.)
Change the raw file URL.
Call and check that we get the value from step 3.

Testing Done:

	+	What was wrong
	+
		After initializing a Git repository and adding a bad Raw File URL Mask, the retrieved file is cached. After the Raw File URL Mask is changed to a valid one, the retrieved file is still from the cache because it does not take into consideration the new Raw File URL.

~		This was fixed by including the raw_file_url into the cache key for the functions: `_make_file_exists_cache_key` and `_make_file_exists_cache_key`. This change was included in `reviewboard/scmtools/models.py`. An automated test is included to check for two separate calls to the `_get_file_uncached` function, since the function should be invoked whenever the `raw_file_url` is changed.
	~	How it was fixed
	+
	+	This was fixed by including the raw_file_url into the cache key for the functions: `_make_file_exists_cache_key` and `_make_file_exists_cache_key`. By including the raw_file_url into the cache key, the cache key will change whenever the raw file url is changed, such that it will not retrieve stale files from the cache. This change was included in `reviewboard/scmtools/models.py`.
	+
	+	Testing
	+
	+	Added unit tests to check that the cached copy of the file is grabbed when there are no changes to the raw_file_url and to check that a new file is retrieved when the raw_file_url changes. All tests pass.

Diff:

Revision 5 (+30 -7)

Show changes

	reviewboard/scmtools/models.py
	reviewboard/scmtools/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/models.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/models.py

reviewboard/scmtools/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 31
 E251 unexpected spaces around keyword / parameter equals
```
reviewboard/scmtools/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 31
 E251 unexpected spaces around keyword / parameter equals
```
reviewboard/scmtools/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 9
 E265 block comment should start with '# '
```
reviewboard/scmtools/tests.py (Diff revision 5)
The issue has been resolved. Show all issues
```
Col: 9
 E265 block comment should start with '# '
```

Diff:

Revision 6 (+30 -7)

Show changes

	reviewboard/scmtools/models.py
	reviewboard/scmtools/tests.py

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/models.py

reviewboard/scmtools/tests.py (Diff revision 6)

The issue has been resolved. Show all issues

I'd like to see this have a few more comments, to explain exactly what the order of operations are. Comments should also start with a capital letter and end in a period.

Hi Kevin,

The info on what was wrong and how it was fixed should go in the description of the change. It shouldn't have its own headers, but just be a complete story. Take a look at the examples in that guide and you'll see what we expect. You can also dig into other commits by David and myself to see some other examples.

Description:

~		Incorporated raw_file_url into cache key in Repository class
	~	After initializing a Git repository and adding a bad Raw File URL Mask, the retrieved file is cached. After the Raw File URL Mask is changed to a valid one, the retrieved file is still from the cache because it does not take into consideration the new Raw File URL.
	+
	+	This was fixed by including the raw_file_url into the cache key for the functions: `_make_file_exists_cache_key` and `_make_file_exists_cache_key`. By including the raw_file_url into the cache key, the cache key will change whenever the raw file url is changed, such that it will not retrieve stale files from the cache. This change was included in `reviewboard/scmtools/models.py`.

Testing Done:

~		What was wrong
	~	Added unit tests to check that the cached copy of the file is grabbed when there are no changes to the raw_file_url and to check that a new file is retrieved when the raw_file_url changes.
	+	Added unit test for get_file_exists() function.

~		After initializing a Git repository and adding a bad Raw File URL Mask, the retrieved file is cached. After the Raw File URL Mask is changed to a valid one, the retrieved file is still from the cache because it does not take into consideration the new Raw File URL.
	~	Ran whole test suite- all tests pass.
-
-		How it was fixed
-
-		This was fixed by including the raw_file_url into the cache key for the functions: `_make_file_exists_cache_key` and `_make_file_exists_cache_key`. By including the raw_file_url into the cache key, the cache key will change whenever the raw file url is changed, such that it will not retrieve stale files from the cache. This change was included in `reviewboard/scmtools/models.py`.
-
-		Testing
-
-		Added unit tests to check that the cached copy of the file is grabbed when there are no changes to the raw_file_url and to check that a new file is retrieved when the raw_file_url changes. All tests pass.

Diff:

Revision 7 (+51 -7)

Show changes

	reviewboard/scmtools/models.py
	reviewboard/scmtools/tests.py

Tool: Pyflakes
Processed Files:
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/models.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/scmtools/tests.py
    reviewboard/scmtools/models.py

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (7db43b4)

Cache Key now takes Git's Raw File URL Mask into account

Information

Reviewers

Summary:

Bugs:

Diff:

Diff:

Diff:

Testing Done:

What was wrong

How it was fixed

Testing

Diff:

Diff:

Description:

Testing Done:

What was wrong

How it was fixed

Testing

Diff:

Status: Closed (submitted)

Change Summary: