Summary

Get rid of duplicate entries in the root resource's URI templates.

Review Request #12681 — Created Oct. 14, 2022 and submitted Oct. 31, 2022, 2:39 p.m.

Information

Owner

maubin

Repository

Review Board

Branch

release-5.0.x

Bugs

Depends On

~~12682~~

Reviewers

Groups

reviewboard

People

Description

In Review Board 5.0, we introduced new top level reviews and comment resources
for querying across all review requests. These resources shared the same name
as other reviews and comment resources. This exposed a problem where resources
that have the same name will have their URI templates in the Root resource's
URI templates list overwritten by one another. Depending on the order that the
resources are added to the URI templates list, this can break the API by
replacing the URI template for a resource with another URI template that may
not behave in the same way or support the same methods as the previous one.

This is a problem that requires more thought and a rethink of how we handle
our resources in general, but for now this change applies a solution that
ensures we keep as much compatibility with our previous API (Python 2.7
behavior) as possible.

Instead of using the name attribute we use the uri_template_name and
uri_template_name_plural for the names of resources in the URI templates
list. This defaults to the name, but can be set to something else to avoid
duplicates in the templates list, or to None to have the resource omitted
from the templates list. Resources that share the same name had their
uri_template_name's changed to match how our previous API behaved and
to give access to the resources that were being overwritten.

Testing Done


Manually compared the URI templates list to our URI templates list from

  Python 2.7, made sure that the duplicate resources were renamed in a way that

  does not break compatibility with the Python 2.7 list.
Ran all unit tests.

Commits

Summary	ID
Get rid of duplicate entries in the root resource's URI templates list.	0f1b52bf53a0412d78cec927586f867b4b504b97

Issues

Description	From	Last Updated
What do you think about having a unit test that compares the entire uri_templates dictionary in a resulting payload to …	chipx86	Oct. 19, 2022, 8:59 p.m.
I made a comment on the Djblets side about possibly reusing name_plural. If that's worth adding there, then some of …	chipx86	Oct. 19, 2022, 9:05 p.m.
No indentation here within "Returns".	chipx86	Oct. 19, 2022, 8:59 p.m.
Should I put this in a settings.py setting? That way people could customize it if they wanted to. Also what …	maubin	Oct. 25, 2022, 11:41 p.m.
line too long (126 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:20 p.m.
line too long (143 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (106 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (101 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (121 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (85 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (101 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (110 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (123 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (112 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (103 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (100 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (144 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (142 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (115 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (120 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (103 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (117 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (107 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (100 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (121 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (155 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (88 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (118 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (95 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (113 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (82 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (124 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (91 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
line too long (102 > 79 characters) Column: 80 Error code: E501	reviewbot	Oct. 19, 2022, 9:21 p.m.
Let's call this expected_uri_templates. The above comment makes sense for right now, but that's more a technicality, as going forward …	chipx86	Oct. 27, 2022, 11:42 p.m.

flake8 passed.

JSHint passed.

The issue has been resolved. Show all issues

What do you think about having a unit test that compares the entire uri_templates dictionary in a resulting payload to a known result? We'd then notice any differences immediately (even if not a conflict), and have a good record of what we should expect in that result.

maubin Oct. 19, 2022, 10:09 a.m.

I like that idea. I'll compare it to the Python 2.7 list. Also when you say the Python 2.7 version of the API, this refers to Review Board 3.0 right?

chipx86 Oct. 19, 2022, 4:21 p.m.
```
Yep! 3.0, same as RBCommons's API.
```

The issue has been resolved. Show all issues

I made a comment on the Djblets side about possibly reusing name_plural. If that's worth adding there, then some of these get to go away.

reviewboard/webapi/resources/root.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No indentation here within "Returns".
```

Change Summary:


Got rid of some specifically set uri_template_name_plurals after refactoring the way the property works.
Added a unit test for comparing URI templates to our Python 2.7 version of them.

Commits:

	Summary	ID
	Get rid of duplicate entries in the root resource's URI templates list.	f75cc8490d711ebbf6d4b8c4b19e1733faa244fc
	Get rid of duplicate entries in the root resource's URI templates list.	643bdfe377c2d24cba9f3ff140c38145a89a9f81

Diff:

Revision 2 (+342 -36)

Show changes

	reviewboard/webapi/resources/diffcommit.py
	reviewboard/webapi/resources/file_attachment_comment.py
	reviewboard/webapi/resources/filediff_comment.py
	reviewboard/webapi/resources/repository.py
	reviewboard/webapi/resources/repository_info.py
	reviewboard/webapi/resources/review_diff_comment.py
	reviewboard/webapi/resources/review_reply_file_attachment_comment.py
	reviewboard/webapi/resources/review_reply_general_comment.py
	13 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

```
Warning: Showing 30 of 57 failures.
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (126 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (143 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (106 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (101 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (121 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (85 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (101 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (110 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (123 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (112 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (103 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (100 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (144 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (142 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (115 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (120 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (103 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (117 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (107 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (100 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (121 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (155 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (88 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (118 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (95 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (113 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (82 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (124 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (91 > 79 characters)

Column: 80
Error code: E501
```
reviewboard/webapi/tests/test_root.py (Diff revision 2)
The issue has been dropped. Show all issues
```
line too long (102 > 79 characters)

Column: 80
Error code: E501
```

reviewboard/webapi/tests/test_root.py (Diff revision 2)

The issue has been resolved. Show all issues

Should I put this in a settings.py setting? That way people could customize it if they wanted to. Also what should I do about the line length of the urls being > 79.

chipx86 Oct. 25, 2022, 4:02 p.m.

Discussed in the meeting today, but just for the record, since this will only run internally and won't take into account things like extensions, it'll be fine to keep these lines within the test itself.

For the line length, we can just wrap the string.

I did notice some of the strings are using double quotes, though. Those should be single quotes instead.

Change Summary:

Uses single quotes, wrapped strings and sorted the hardcoded URI templates for testing.

Commits:

	Summary	ID
	Get rid of duplicate entries in the root resource's URI templates list.	643bdfe377c2d24cba9f3ff140c38145a89a9f81
	Get rid of duplicate entries in the root resource's URI templates list.	03c3b292d952d1a7658a713d5b4d9537fa57c05f

Diff:

Revision 3 (+552 -36)

Show changes

	reviewboard/webapi/resources/diffcommit.py
	reviewboard/webapi/resources/file_attachment_comment.py
	reviewboard/webapi/resources/filediff_comment.py
	reviewboard/webapi/resources/repository.py
	reviewboard/webapi/resources/repository_info.py
	reviewboard/webapi/resources/review_diff_comment.py
	reviewboard/webapi/resources/review_reply_file_attachment_comment.py
	reviewboard/webapi/resources/review_reply_general_comment.py
	13 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/webapi/tests/test_root.py (Diff revision 3)

The issue has been resolved. Show all issues

Let's call this expected_uri_templates.

The above comment makes sense for right now, but that's more a technicality, as going forward this list is going to be updated.

What we're really wanting to check is whether the full list of URI templates in the API match exactly what we expect. Less so than a subset from 3.0, because we want to ensure there's no chance of this happening going forward either (even if Djblets is itself doing its own internal checking).

Any time we add a new resource, we'll want/need to change this as well.

Given that, I think the below dictionary building can also be simplified to just do a standard assertEqual() between the resulting URI templates and this dictionary.

Change Summary:

Updated the expected_uri_templates dict and renamed some resources so that everything matches up with our 3.0, 4.0 and 5.0 APIs

Commits:

	Summary	ID
	Get rid of duplicate entries in the root resource's URI templates list.	03c3b292d952d1a7658a713d5b4d9537fa57c05f
	Get rid of duplicate entries in the root resource's URI templates list.	0f1b52bf53a0412d78cec927586f867b4b504b97

Diff:

Revision 4 (+644 -36)

Show changes

	reviewboard/webapi/resources/diffcommit.py
	reviewboard/webapi/resources/file_attachment_comment.py
	reviewboard/webapi/resources/filediff_comment.py
	reviewboard/webapi/resources/repository.py
	reviewboard/webapi/resources/repository_info.py
	reviewboard/webapi/resources/review_diff_comment.py
	reviewboard/webapi/resources/review_reply_file_attachment_comment.py
	reviewboard/webapi/resources/review_reply_general_comment.py
	13 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to release-5.0.x (5e3863c)