Summary:

Fix regressions, add future-proofing for URI templates and new APIs.

Review Request #12726 — Created Nov. 16, 2022 and submitted Jan. 9, 2023, 8:09 p.m.

Information
Owner:	chipx86
Repository:	Review Board
Branch:	release-5.0.x
Bugs:
Depends On:
Reviewers
Groups:	reviewboard
People:

Description

We recently had some conflicts with the new root resources for reviews
and comments, and added collision detection for URI template
registrations.

Since these went in, a couple regressions were discovered:

The root review/comment APIs had registered item APIs, which don't
actually work, due to the lack of uri_object_key = None.
The root diff comment API had the wrong URL
(/api/review-diff-comment/ instead of /api/diff-comments/) and
URI template (review_diff_comment instead of all_diff_comments).
The review request file attachment comments API mistakenly had a
all_reviews_file_attachment_comments URI template, which doesn't
represent that resource.
Some of the URI templates were pointing to the wrong API resources,
based on a comparison to the Review Board 3 API on RBCommons.

Some of these were just wrong resources or attributes being set, which
regressed in 5.0.1. However, most of this is just due to the shaky
foundation that URI templates have historically been on. We didn't have
proper management of any of this, and any kind of testing is brand-new.
So a big part of this change is simply an audit of the URI templates.

To address the problems we've hit, we're now deprecating the older
confusing URI templates that triggered collisions before, and are
instead replacing them with namespaced versions. These are all ones that
pertain to diffs, reviews, review replies, and review requests.

The complete list of URI templates have been added to the RootResource
documentation, as have the legacy names. These are versioned (starting
with 4.0) to help users and us.

All legacy names are now considered deprecated, but will still point to
the old locations pertaining to the Review Board 3 API.

Legacy URI templates are also registered in a way that doesn't create
an entry per API root resource location, which will save memory for
installs using Local Sites.

Testing Done

Unit tests pass.

Diffed the list against the 5.0.1 and 3.0.x releases to verify
that the resulting URI templates seem to be compatible with what we
intended before and what we're wanting to point to now, and that
all legacy URLs correctly match.

Built the docs and checked that the resulting Root Resource's docs
on URI templates correctly linked to each target resource page.

Commits

Summary
	Fix regressions, add future-proofing for URI templates and new APIs.

Screenshots

Issues

Description	From	Last Updated
I went through the resources that exist in our expected_uri_templates reference and found some that are missing from the RootResource …	maubin	Nov. 18, 2022, 5:38 p.m.
'typing.Dict' imported but unused Column: 1 Error code: F401	reviewbot	Nov. 16, 2022, 7:41 p.m.
I'm thinking maybe file_diff would be a better name? Since it matches the policy_id here and would follow the URI …	maubin	Dec. 13, 2022, 3:05 p.m.
Should move this one to the deprecated section since you're replacing it with review_request_draft.	maubin	Jan. 8, 2023, 10:42 p.m.
This key should be extensions instead of extension-list.	maubin	Nov. 18, 2022, 3:47 p.m.
These were actually added in 5.0.1.	maubin	Nov. 18, 2022, 3:48 p.m.
I think it could be worthwhile to add comments here to show which URI templates and names are legacy/deprecated. Or …	maubin	Dec. 13, 2022, 3:05 p.m.
review_request_draft will be added in 5.0.2.	maubin	Jan. 8, 2023, 10:43 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/root.py (Diff revision 1)

Show all issues

'typing.Dict' imported but unused

Column: 1
Error code: F401

Change Summary:

Removed an unused import.

Description:

		We recently had some conflicts with the new root resources for reviews
		and comments, and added collision detection for URI template
		registrations.

		Since these went in, a couple regressions were discovered:

		The root review/comment APIs had registered item APIs, which don't actually work, due to the lack of `uri_object_key = None`.
		The root diff comment API had the wrong URL (`/api/review-diff-comment/` instead of `/api/diff-comments`/) and URI template (`review_diff_comment` instead of `all_diff_comments`).
		The review request file attachment comments API mistakenly had a `all_reviews_file_attachment_comments` URI template, which doesn't represent that resource.
		Some of the URI templates were pointing to the wrong API resources, based on a comparison to the Review Board 3 API on RBCommons.

		Some of these were just wrong resources or attributes being set, which
		regressed in 5.0.1. However, most of this is just due to the shaky
		foundation that URI templates have historically been on. We didn't have
		proper management of any of this, and any kind of testing is brand-new.
		So a big part of this change is simply an audit of the URI templates.

		To address the problems we've hit, we're now deprecating the older
		confusing URI templates that triggered collisions before, and are
		instead replacing them with namespaced versions. These are all ones that
		pertain to diffs, reviews, review replies, and review requests.

		The complete list of URI templates have been added to the `RootResource`
		documentation, as have the legacy names. These are versioned (starting
		with 4.0) to help users and us.

		All legacy names are now considered deprecated, but will still point to
		the old locations pertaining to the Review Board 3 API.
	+
	+	Legacy URI templates are also registered in a way that doesn't create
	+	an entry per API root resource location, which will save memory for
	+	installs using Local Sites.

Commits:

	Summary
-		Fix regressions, add future-proofing for URI templates and new APIs.
+		Fix regressions, add future-proofing for URI templates and new APIs.

Diff:

Revision 2 (+1558 -120)

Show changes

	reviewboard/webapi/resources/change.py
	reviewboard/webapi/resources/diffcommit.py
	reviewboard/webapi/resources/file_attachment.py
	reviewboard/webapi/resources/file_attachment_comment.py
	reviewboard/webapi/resources/filediff.py
	reviewboard/webapi/resources/original_file.py
	reviewboard/webapi/resources/patched_file.py
	reviewboard/webapi/resources/repository_branches.py
	16 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Good stuff, thanks for doing this! Really helps to clear things up with the URI templates while keeping legacy behaviour. And good catch for the root reviews/comments fixes.

chipx86 Nov. 18, 2022, 3:55 p.m.

Thanks for the review! Yeah I figured we might as well do this now, and give ourselves a better foundation for later when we rethink all this.

I have some thoughts on a future successor to uri_templates when we finally do a new API revision, based on what we've encountered this release.

Show all issues

I went through the resources that exist in our expected_uri_templates reference and found some that are missing from the RootResource documentation:
Resource | URI template name
---------|-------------------
File Diff Comment List | file_diff_comments
Diff Context | diff_context
File Attachment | file_attachment
File Attachment List | file_attachments

chipx86 Nov. 18, 2022, 3:55 p.m.
```
Ah, good catches! Thanks!
```

reviewboard/webapi/resources/filediff.py (Diff revision 2)

Show all issues

I'm thinking maybe file_diff would be a better name? Since it matches the policy_id here and would follow the URI naming for the FileDiffCommentResource (which I set to file_diff_comment in 5.0.1). Or you could change the FileDiffCommentResource URI name to diff_file_comment.

chipx86 Nov. 18, 2022, 3:55 p.m.

Yeah, this one's tricky.

We can't change the URI name or we break API compatibility. Same with the policy ID.

I was aiming to keep all diff-related resources under the same prefix, but it's probably better to keep consistent with the rest of the naming.

I'll make this change, and I think I'll move the original_file/patched_file under this name as well.

maubin Nov. 21, 2022, 9:28 a.m.
```
That sounds good to me.
```

reviewboard/webapi/resources/root.py (Diff revision 2)
Show all issues
```
This key should be extensions instead of extension-list.
```
reviewboard/webapi/resources/root.py (Diff revision 2)
Show all issues
```
These were actually added in 5.0.1.
```

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

Show all issues

I think it could be worthwhile to add comments here to show which URI templates and names are legacy/deprecated. Or move them so that they have their own section in the dict, like how you did it in the RootResource documentation. But then again we could always just refer to the documentation for this info.

chipx86 Nov. 18, 2022, 3:55 p.m.
```
Good idea! I'll go ahead and do this.
```

Change Summary:


Standardized naming for filediff templates.
Added some missing URI templates and fixed up versioning for others.
Fixed some ReST syntax errors.
Added comments in the unit tests to help see which URI templates are deprecated and as of when.

Commits:

	Summary
-		Fix regressions, add future-proofing for URI templates and new APIs.
+		Fix regressions, add future-proofing for URI templates and new APIs.

Diff:

Revision 3 (+1874 -352)

Show changes

	reviewboard/webapi/resources/change.py
	reviewboard/webapi/resources/diffcommit.py
	reviewboard/webapi/resources/file_attachment.py
	reviewboard/webapi/resources/file_attachment_comment.py
	reviewboard/webapi/resources/filediff.py
	reviewboard/webapi/resources/original_file.py
	reviewboard/webapi/resources/patched_file.py
	reviewboard/webapi/resources/repository_branches.py
	17 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/webapi/resources/root.py (Diff revisions 2 - 3)

Show all issues

Should move this one to the deprecated section since you're replacing it with review_request_draft.

reviewboard/webapi/tests/test_root.py (Diff revisions 2 - 3)
Show all issues
```
review_request_draft will be added in 5.0.2.
```

Change Summary:

Updated deprecation info for draft vs. review_request_draft.

Commits:

	Summary
-		Fix regressions, add future-proofing for URI templates and new APIs.
+		Fix regressions, add future-proofing for URI templates and new APIs.

Diff:

Revision 4 (+1876 -352)

Show changes

	reviewboard/webapi/resources/change.py
	reviewboard/webapi/resources/diffcommit.py
	reviewboard/webapi/resources/file_attachment.py
	reviewboard/webapi/resources/file_attachment_comment.py
	reviewboard/webapi/resources/filediff.py
	reviewboard/webapi/resources/original_file.py
	reviewboard/webapi/resources/patched_file.py
	reviewboard/webapi/resources/repository_branches.py
	17 more

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-5.0.x (dd2e8ee)