Enforce that API resources have unique URI template names.

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

Information

Djblets
release-3.x

Reviewers

We discovered 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 behavior as
possible.

This change introduces the uri_template_name and uri_template_name_plural
attributes for use as the names of resources in the URI templates list, instead
of using the name attribute. This defaults to the name, but can be set to
something else to ensure that resource names in the templates list are unique.
This can also be set to None to have the resource omitted from the URI
templates list. When building the URI templates list, we assert that the
names used in it are unique, as to avoid issues where a resource's URI
template is being overwritten by another resource. The
RootResource.register_uri_template method does not check whether the
resource already exists in the templates because there are use cases where
we need to overwrite the URI template of an existing resource.

  • Ran all unit tests.
  • Manually tested Review Board's API Root resource, saw that the URI
    templates list appeared properly.
Summary ID
Enforce that API resources have unique URI template names.
d43f52950b61b1f7e6ae09d8f3071653857acac2
Description From Last Updated

Let's start introducing type annotations for all new code. For this function, since there's no typed arguments and just a …

chipx86chipx86

This'll be coming in 4.0.1. Should also have a Type in the docs.

chipx86chipx86

Same comment regarding including the type in the signature.

chipx86chipx86

Same comments regarding the version and Type.

chipx86chipx86

Wonder if it'd be worth doing: elif self.uri_template_name == self.name: return self.name_plural else: return self.uri_template_name + 's' Also, let's pull …

chipx86chipx86

We definitely want to catch this during development, but one bad extension can bring down a whole server if this …

chipx86chipx86

Same here as above. In fact, what could be done is you could have this gather a list of URI …

chipx86chipx86

And here.

chipx86chipx86

4.0.1.

chipx86chipx86

Here and the string below, can we put quotes around the first %s, to help ensure it reads as a …

chipx86chipx86

The % (...) should go on the next line.

chipx86chipx86

Sorry I didn't catch this before, but an important point when localizing text: If there's more than one format string …

chipx86chipx86

For logging, we actually don't want to localize. Otherwise, if a user using, say, Mandarin were to trigger a loggable …

chipx86chipx86

To give a bit more room and make editing easier, multi-line strins are better formatted as: expected_message = ( '...' …

chipx86chipx86
chipx86
  1. 
      
  2. djblets/webapi/resources/base.py (Diff revision 1)
     
     
    Show all issues

    Let's start introducing type annotations for all new code.

    For this function, since there's no typed arguments and just a typed (possibly-None) string return value, the format would be:

    def uri_template_name(self) -> Optional[str]:
        ...
    
  3. djblets/webapi/resources/base.py (Diff revision 1)
     
     
     
    Show all issues

    This'll be coming in 4.0.1.

    Should also have a Type in the docs.

    1. Actually, not 4.0.1. 3.1.

      I was thinking our recent Djblets release was at v4.0 when I saw this, but it's at 3.0. This change will be adding a feature to Djblets 3, so we need to bump the minor version, as per Semantic Versioning. We'd bump the major if we were removing a feature. So the new release will be 3.1.0.

  4. djblets/webapi/resources/base.py (Diff revision 1)
     
     
     
    Show all issues

    Same comment regarding including the type in the signature.

  5. djblets/webapi/resources/base.py (Diff revision 1)
     
     
     
     
    Show all issues

    Same comments regarding the version and Type.

  6. djblets/webapi/resources/root.py (Diff revision 1)
     
     
    Show all issues

    We definitely want to catch this during development, but one bad extension can bring down a whole server if this happens in production.

    I suggest we conditionalize this based on settings.DEBUG. Something like:

    if uri_template_name in templates:
        if settings.DEBUG:
            raise ImproperlyConfigured('some useful message to help them debug this here')
        else:
             logger.error('some useful message here...')
    

    And then, of course, only add to the template if it's not already in there.

  7. djblets/webapi/resources/root.py (Diff revision 1)
     
     
    Show all issues

    Same here as above.

    In fact, what could be done is you could have this gather a list of URI templates, then loop over when all this is done to apply the logic detailed above, so it's not duplicated.

  8. djblets/webapi/resources/root.py (Diff revision 1)
     
     
    Show all issues

    And here.

  9. Show all issues

    4.0.1.

  10. 
      
chipx86
  1. 
      
  2. djblets/webapi/resources/base.py (Diff revision 1)
     
     
     
    Show all issues

    Wonder if it'd be worth doing:

    elif self.uri_template_name == self.name:
        return self.name_plural
    else:
        return self.uri_template_name + 's'
    

    Also, let's pull out uri_template_name into a local variable up-front, so that we don't have to re-call the property every time. Maybe also make this method a @cached_property.

  3. 
      
maubin
chipx86
  1. This looks fantastic. It's going to save us from all manner of problems down the road as the API grows. Thanks for taking this on :)

  2. djblets/webapi/resources/root.py (Diff revision 2)
     
     
    Show all issues

    Here and the string below, can we put quotes around the first %s, to help ensure it reads as a name and not some word in a sentence?

  3. djblets/webapi/resources/root.py (Diff revision 2)
     
     
    Show all issues

    The % (...) should go on the next line.

  4. 
      
maubin
chipx86
  1. 
      
  2. djblets/webapi/resources/root.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
    Show all issues

    Sorry I didn't catch this before, but an important point when localizing text: If there's more than one format string inside, you need to use %(somevar)s rather than %s, and pass in a dictionary fulfilling those variables.

    The reason is that this sentence:

    "Hello %s, how is your %s?"
    

    Could be localized to something structured more like:

    "How is your %s, %s?"
    

    If positional, that could end up being "How is your Bob, day?"

    To solve that, we'd do:

    _('Hello %(name)s, how is your %(subject)s?')
    % {
        'name': ...,
        'subject': ...,
    }
    
  3. djblets/webapi/resources/root.py (Diff revision 3)
     
     
     
    Show all issues

    For logging, we actually don't want to localize. Otherwise, if a user using, say, Mandarin were to trigger a loggable error, and we had localized this to Mandarin, it would be difficult for an English-speaking administrator to debug the error.

    So we can strip localization for logging. We only want it for user-visible messages.

  4. djblets/webapi/tests/test_root_resource.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
    Show all issues

    To give a bit more room and make editing easier, multi-line strins are better formatted as:

    expected_message = (
        '...'
        '...'
        '...'
    )
    
  5. 
      
maubin
chipx86
  1. Ship It!
  2. 
      
maubin
Review request changed
Status:
Completed
Change Summary:
Pushed to release-3.x (a3a6563)