Enforce that API resources have unique URI template names.
Review Request #12682 — Created Oct. 14, 2022 and submitted
Information | |
---|---|
maubin | |
Djblets | |
release-3.x | |
12681 | |
Reviewers | |
djblets | |
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
anduri_template_name_plural
attributes for use as the names of resources in the URI templates list, instead
of using thename
attribute. This defaults to thename
, but can be set to
something else to ensure that resource names in the templates list are unique.
This can also be set toNone
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 |
---|
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 … |
|
|
This'll be coming in 4.0.1. Should also have a Type in the docs. |
|
|
Same comment regarding including the type in the signature. |
|
|
Same comments regarding the version and Type. |
|
|
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 … |
|
|
We definitely want to catch this during development, but one bad extension can bring down a whole server if this … |
|
|
Same here as above. In fact, what could be done is you could have this gather a list of URI … |
|
|
And here. |
|
|
4.0.1. |
|
|
Here and the string below, can we put quotes around the first %s, to help ensure it reads as a … |
|
|
The % (...) should go on the next line. |
|
|
Sorry I didn't catch this before, but an important point when localizing text: If there's more than one format string … |
|
|
For logging, we actually don't want to localize. Otherwise, if a user using, say, Mandarin were to trigger a loggable … |
|
|
To give a bit more room and make editing easier, multi-line strins are better formatted as: expected_message = ( '...' … |
|
-
-
djblets/webapi/resources/base.py (Diff revision 1) 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]: ...
-
djblets/webapi/resources/base.py (Diff revision 1) This'll be coming in 4.0.1.
Should also have a
Type
in the docs. -
djblets/webapi/resources/base.py (Diff revision 1) Same comment regarding including the type in the signature.
-
-
djblets/webapi/resources/root.py (Diff revision 1) 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.
-
djblets/webapi/resources/root.py (Diff revision 1) 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.
-
-
-
-
djblets/webapi/resources/base.py (Diff revision 1) 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
.

Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+544 -64) |
Checks run (2 succeeded)
-
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 :)
-
djblets/webapi/resources/root.py (Diff revision 2) 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? -

Change Summary:
- Put quotes around template names in error/log messages.
- Moved a
% (...)
so that its properly formatted.
Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+546 -64) |
Checks run (2 succeeded)
-
-
djblets/webapi/resources/root.py (Diff revision 3) 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': ..., }
-
djblets/webapi/resources/root.py (Diff revision 3) 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.
-
djblets/webapi/tests/test_root_resource.py (Diff revision 3) To give a bit more room and make editing easier, multi-line strins are better formatted as:
expected_message = ( '...' '...' '...' )

Change Summary:
- Removed text localization in logged messages.
- Made sure ordering of formatted strings is consistent in localized texts.
- Made singleton resources have their
uri_template_name_plural
set touri_template_name
ifuri_template_name
is set. - Added unit tests for
uri_template_name
anduri_template_name_plural
. - Made tests for
RootResource.get_uri_templates
more comprehensive and added clarifying comments.
Commits: |
|
||||||
---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+812 -64) |