Enforce that API resources have unique URI template names.
Review Request #12682 — Created Oct. 14, 2022 and submitted
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 | ID |
---|---|
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 … |
chipx86 | |
This'll be coming in 4.0.1. Should also have a Type in the docs. |
chipx86 | |
Same comment regarding including the type in the signature. |
chipx86 | |
Same comments regarding the version and Type. |
chipx86 | |
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 … |
chipx86 | |
We definitely want to catch this during development, but one bad extension can bring down a whole server if this … |
chipx86 | |
Same here as above. In fact, what could be done is you could have this gather a list of URI … |
chipx86 | |
And here. |
chipx86 | |
4.0.1. |
chipx86 | |
Here and the string below, can we put quotes around the first %s, to help ensure it reads as a … |
chipx86 | |
The % (...) should go on the next line. |
chipx86 | |
Sorry I didn't catch this before, but an important point when localizing text: If there's more than one format string … |
chipx86 | |
For logging, we actually don't want to localize. Otherwise, if a user using, say, Mandarin were to trigger a loggable … |
chipx86 | |
To give a bit more room and make editing easier, multi-line strins are better formatted as: expected_message = ( '...' … |
chipx86 |
-
-
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]: ...
-
-
-
-
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.
-
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.
-
-
-
-
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:
-
Summary ID 5eb90d2ff54066bd8d5da56bfdd1835fab354517 818ccd09aa86ee95610524d9293931c62f87d3a3
Checks run (2 succeeded)
- Change Summary:
-
- Put quotes around template names in error/log messages.
- Moved a
% (...)
so that its properly formatted.
- Commits:
-
Summary ID 818ccd09aa86ee95610524d9293931c62f87d3a3 77ebdaff181fb54c1505b1b86c0dfb347da4196a
Checks run (2 succeeded)
-
-
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': ..., }
-
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.
-
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:
-
Summary ID 77ebdaff181fb54c1505b1b86c0dfb347da4196a d43f52950b61b1f7e6ae09d8f3071653857acac2