Add setting to disable nested path pk coercion.

[ngnpope]

This is another thing I came across in my conversions from drf-yasg which doesn't have any support for converting path parameters for nested routers, but does handle {pk} to {id} based on SCHEMA_COERCE_PATH_PK.

I wanted to disable this behaviour in drf-spectacular for the following reasons:

1. It is easier to compare the OpenAPI 2.0 schema produced by drf-yasg converted to OpenAPI 3.0 (with a bunch of other clean-ups) against the OpenAPI 3.0 schema produced by drf-spectacular.
2. We are using django-treebeard and the tree-like structures have no self-referencing foreign key to follow, e.g. for a tree of folders, {parent_pk} won't be changed to {parent_id}.
3. We have some relationships that are not direct, for example, something like /{company_pk}/person/{pk} where the relationship is Person → Department → Company. In this case Person.company doesn't exist and there is some custom querying to look up the value.

Where this becomes a mess is when we have something like /x/{a_pk}/y/{b_pk}/z/{pk} that becomes /x/{a_id}/y/{b_pk}/z/{id} which is inconsistent.

I had thought that it would be nice to have something that allows us to forcibly override this for certain models, e.g. we could pretend that Person.company existed. Obviously we'd then need to override the type using @extend_schema(parameters=...) That would looks something like:

# drf_spectacular/settings.py:

SPECTACULAR_DEFAULTS = {
    ...
    'SCHEMA_COERCE_PATH_PK_OVERRIDE': {
        'payroll.models.Person': {'company'},
    },
}

# drf_spectacular/generators.py:

class SchemaGenerator(BaseSchemaGenerator):
    ...

    def coerce_path(self, path, method, view):
        """
        Customized coerce_path which also considers the `_pk` suffix in URL paths
        of nested routers.
        """
        path = super().coerce_path(path, method, view)  # take care of {pk}
        if not spectacular_settings.SCHEMA_COERCE_PATH_PK_NESTED:
            return path
        model = get_view_model(view, emit_warnings=False)
        if not self.coerce_path_pk or not model:
            return path
        lookup = f'{model.__module__}.{model.__qualname__}'
        override = spectacular_settings.SCHEMA_COERCE_PATH_PK_OVERRIDE.get(lookup) or set()
        for match in RELATED_MODEL_PARAMETER_RE.findall(path):
            if hasattr(model, match) or match in override:
                path = path.replace(f'{match}_pk', f'{match}_id')
        return path

Would something like that also be acceptable?

It seems a shame that, when we set SCHEMA_COERCE_PATH_PK_NESTED to False, we're forced to manually specify the parameters as though it forgets that {user_pk} is still the equivalent to {user_id} Maybe I'm missing something...

[codecov]

Codecov Report

Merging #516 (6dc3a62) into master (3bef368) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #516      +/-   ##
==========================================
- Coverage   98.63%   98.63%   -0.01%     
==========================================
  Files          58       58              
  Lines        5944     5987      +43     
==========================================
+ Hits         5863     5905      +42     
- Misses         81       82       +1     

Impacted Files	Coverage Δ
drf_spectacular/settings.py	100.00% <ø> (ø)
drf_spectacular/generators.py	98.01% <100.00%> (+0.02%)	⬆️
drf_spectacular/plumbing.py	97.56% <100.00%> (+<0.01%)	⬆️
tests/contrib/test_drf_nested_routers.py	100.00% <100.00%> (ø)
tests/test_regressions.py	100.00% <100.00%> (ø)
tests/conftest.py	97.43% <0.00%> (-2.57%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bef368...6dc3a62. Read the comment docs.

[tfranzel]

Where this becomes a mess is when we have something like /x/{a_pk}/y/{b_pk}/z/{pk} that becomes /x/{a_id}/y/{b_pk}/z/{id} which is inconsistent.

For me, this is the deal breaker i did not properly think about. Having a mixture of _pk and _id, which cannot be changed atm, is very bad. not sure if i want another setting with SCHEMA_COERCE_PATH_PK_OVERRIDE, but this thing has to be homogenized somehow. i will think about a this.

[tfranzel]

hey @ngnpope, gave this more thought and came to the conlusion it should be disabled by default. i had to fix up a couple of places to make detection robust enough. i think this should now cover almost all cases.

• SCHEMA_COERCE_PATH_PK_SUFFIX enables global _pk coercion.
• coercion or not, django path variables should be detected
• coercion or not, regex pattern should be detected
• coercion or not, if above fails, a sensible model lookup should be attempted

thank you for the PR and getting me started here. could you try out if the current master works for you?

[ngnpope]

Hey @tfranzel, I've given this a go and I get fewer warnings than I had before, so if that is a measure of success then great! 😆

[tfranzel]

i would say so. i actually think of the warnings as a todo list of schema problems. after all, most warning are there to guide you to a better schema.