Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: get param's regex pattern from part path #510

Closed

Conversation

jtamm-red
Copy link
Contributor

urlpatterns = [
    url('^data/(?P<code>[a-zA-Z0-9_\\-]+)/(?P<version>[0-9.]+)', get_data)
]

I got warning log:
could not derive type of path parameter "code" because because it is untyped and obtaining queryset from the viewset failed. Consider adding a type to the path (e.g. <int:code>) or annotating the parameter type with @extend_schema. Defaulting to "string

@tfranzel
Copy link
Owner

tfranzel commented Sep 10, 2021

hi @jtamm-red let me get you some quick feedback before you continue working on this.

some time ago i implemented a state machine to extract the regex: plumbing.analyze_named_regex_pattern. so this has been possible for some time now, but that wasn't the core problem. the issue is in what order to do this without breaking existing users and still leveraging the model auto lookup. all the failed test show you this.

so the issue is not using the regex, but when exactly to use the regex and not the other methods. since this has implications, i'm currently weighing the options.

your implementation is a duplication of existing functionality, which is heavily tested. at the moment this PR has a very low chance to be merged, but the underlying problem may get tackled soon.

@codecov
Copy link

codecov bot commented Sep 10, 2021

Codecov Report

Merging #510 (0adf285) into master (66cc94b) will decrease coverage by 0.03%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #510      +/-   ##
==========================================
- Coverage   98.58%   98.54%   -0.04%     
==========================================
  Files          57       57              
  Lines        5724     5785      +61     
==========================================
+ Hits         5643     5701      +58     
- Misses         81       84       +3     
Impacted Files Coverage Δ
drf_spectacular/plumbing.py 97.85% <100.00%> (+0.05%) ⬆️
tests/test_regressions.py 100.00% <100.00%> (ø)
tests/test_versioning.py 100.00% <100.00%> (ø)
tests/test_warnings.py 100.00% <100.00%> (ø)
tests/test_fields.py 97.32% <0.00%> (-2.68%) ⬇️
tests/__init__.py 98.24% <0.00%> (+3.80%) ⬆️

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 66cc94b...0adf285. Read the comment docs.

@jtamm-red
Copy link
Contributor Author

Hello, @tfranzel

I removed duplicated function and solved tests (unit).

But one problem: parameter 'pk' type cannot be string. Some tests are broken, when parameter 'pk' type is string.

Unfortunately tests (unit) failed on other django version. Can you check and fix it?

@tfranzel
Copy link
Owner

@jtamm-red, as i said:

so the issue is not using the regex, but when exactly to use the regex and not the other methods. since this has implications, i'm currently weighing the options.

Unfortunately tests (unit) failed on other django version. Can you check and fix it?

the failing tests are part of the problem. it changes the processing order and that side effect may potentially break a lot of existing users. it is a change that needs to be carefully weighed.

the change itself is easy. this snipped would theoretically be the only change:

def resolve_regex_path_parameter(path_regex, variable, available_formats):
   for match in _PATH_PARAMETER_COMPONENT_RE.finditer(path_regex):
         UNCHANGED_CODE

   regex_groups = analyze_named_regex_pattern(path_regex)
   if variable in regex_groups and regex_groups[variable] != '[^/.]+':
        return build_parameter_type(
            name=variable,
            schema={
                **build_basic_type(OpenApiTypes.STR),
                'pattern': regex_groups[variable]
            },
            location=OpenApiParameter.PATH,
        )

    return None

@tfranzel
Copy link
Owner

@jtamm-red please have a look at #511 . it addresses this issue and a bunch of related problems.

#511 supersedes this PR, but nonetheless thank you for your initiative and input.

@tfranzel tfranzel closed this Sep 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants