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

Review Request #12681 — Created Oct. 14, 2022 and submitted

Information

Review Board
release-5.0.x

Reviewers

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.

  • 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.
Summary ID
Get rid of duplicate entries in the root resource's URI templates
list.
0f1b52bf53a0412d78cec927586f867b4b504b97
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 …

chipx86chipx86

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

chipx86chipx86

No indentation here within "Returns".

chipx86chipx86

Should I put this in a settings.py setting? That way people could customize it if they wanted to. Also what …

maubinmaubin

line too long (126 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (143 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (106 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (101 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (121 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (85 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (101 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (110 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (123 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (112 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (103 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (100 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (144 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (142 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (115 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (120 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (103 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (117 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (107 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (100 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (121 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (155 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (88 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (118 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (95 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (113 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (82 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (124 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (91 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (102 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

Let's call this expected_uri_templates. The above comment makes sense for right now, but that's more a technicality, as going forward …

chipx86chipx86
chipx86
  1. 
      
  2. 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.

    1. 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?

    2. Yep! 3.0, same as RBCommons's API.

  3. 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.

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

    No indentation here within "Returns".

  5. 
      
maubin
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

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

    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.

    1. 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.

  3. 
      
maubin
chipx86
  1. 
      
  2. reviewboard/webapi/tests/test_root.py (Diff revision 3)
     
     

    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.

  3. 
      
maubin
chipx86
  1. Ship It!
  2. 
      
maubin
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-5.0.x (5e3863c)
Loading...