Skip to content
New issue

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

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

Already on GitHub? # to your account

_get_sidecar_url should probably use django.templatetags.static.static instead of STATIC_URL #718

Closed
glennmatthews opened this issue Apr 26, 2022 · 7 comments
Labels
enhancement New feature or request fix confirmation pending issue has been fixed and confirmation from issue reporter is pending

Comments

@glennmatthews
Copy link
Contributor

Describe the bug

def _get_sidecar_url(package):
    return f'{settings.STATIC_URL}drf_spectacular_sidecar/{package}'

This may fail when using STATICFILES_STORAGE with a non-default backend for which STATIC_URL is unused or is insufficient to describe the static file location. A more robust implementation would possibly be to use django.templatetags.static, i.e.:

from django.templatetags.static import static

def _get_sidecar_url(package):
    return static(f'drf_spectacular_sidecar/{package}')

To Reproduce
Install django-storages and configure STATICFILES_STORAGE = "storages.backends.s3boto3.S3Boto3Storage" (etc.) in your project settings. Do not set STATIC_URL in your project settings. Verify that other static files are correctly loaded from S3, but SpectacularSwaggerView is attempting incorrectly to load the sidecar files from the relative path of /static/ instead of from the S3 server.

Expected behavior
When STATICFILES_STORAGE is configured, use staticfiles_storage.url() instead of STATIC_URL to construct the static file paths for sidecar files (just as django.templatetags.static.static() does).

@tfranzel tfranzel added the enhancement New feature or request label Apr 29, 2022
@tfranzel tfranzel added the fix confirmation pending issue has been fixed and confirmation from issue reporter is pending label Jun 3, 2022
@tfranzel
Copy link
Owner

tfranzel commented Jun 3, 2022

excellent point @glennmatthews! thanks for pointing it out.

@paulsmirnov
Copy link

Hmm... For me this PR actually breaks the correct behavior 😀 The code now references a subdirectory, but directories are missing in django.contrib.staticfiles.storage.ManifestStaticFilesStorage. A directory is not a file, so it is not tracked and has no URL. Now the code crashes:

Traceback (most recent call last):
  File "/opt/conda/lib/python3.7/site-packages/django/core/handlers/exception.py", line 47, in inner
    response = get_response(request)
  File "/opt/conda/lib/python3.7/site-packages/django/core/handlers/base.py", line 181, in _get_response
    response = wrapped_callback(request, *callback_args, **callback_kwargs)
  File "/opt/conda/lib/python3.7/site-packages/django/views/decorators/csrf.py", line 54, in wrapped_view
    return view_func(*args, **kwargs)
  File "/opt/conda/lib/python3.7/site-packages/django/views/generic/base.py", line 70, in view
    return self.dispatch(request, *args, **kwargs)
  File "/opt/conda/lib/python3.7/site-packages/rest_framework/views.py", line 509, in dispatch
    response = self.handle_exception(exc)
  File "/opt/conda/lib/python3.7/site-packages/rest_framework/views.py", line 469, in handle_exception
    self.raise_uncaught_exception(exc)
  File "/opt/conda/lib/python3.7/site-packages/rest_framework/views.py", line 480, in raise_uncaught_exception
    raise exc
  File "/opt/conda/lib/python3.7/site-packages/rest_framework/views.py", line 506, in dispatch
    response = handler(request, *args, **kwargs)
  File "/opt/conda/lib/python3.7/site-packages/drf_spectacular/views.py", line 135, in get
    'dist': self._swagger_ui_dist(),
  File "/opt/conda/lib/python3.7/site-packages/drf_spectacular/views.py", line 176, in _swagger_ui_dist
    return _get_sidecar_url('swagger-ui-dist')
  File "/opt/conda/lib/python3.7/site-packages/drf_spectacular/views.py", line 117, in _get_sidecar_url
    return static(f'drf_spectacular_sidecar/{package}')
  File "/opt/conda/lib/python3.7/site-packages/django/templatetags/static.py", line 167, in static
    return StaticNode.handle_simple(path)
  File "/opt/conda/lib/python3.7/site-packages/django/templatetags/static.py", line 118, in handle_simple
    return staticfiles_storage.url(path)
  File "/opt/conda/lib/python3.7/site-packages/django/contrib/staticfiles/storage.py", line 147, in url
    return self._url(self.stored_name, name, force)
  File "/opt/conda/lib/python3.7/site-packages/django/contrib/staticfiles/storage.py", line 126, in _url
    hashed_name = hashed_name_func(*args)
  File "/opt/conda/lib/python3.7/site-packages/django/contrib/staticfiles/storage.py", line 417, in stored_name
    raise ValueError("Missing staticfiles manifest entry for '%s'" % clean_name)
ValueError: Missing staticfiles manifest entry for 'drf_spectacular_sidecar/swagger-ui-dist'

@tfranzel
Copy link
Owner

tfranzel commented Oct 3, 2022

A directory is not a file

Unix begs to differ 😆

Not quite sure on how to go from here. I don't really use this myself so I would need to take a closer look. @glennmatthews any comments?

@paulsmirnov what do you think would be the correct handling? I mean that change seemed more in line with what Django is expecting.

@paulsmirnov
Copy link

paulsmirnov commented Oct 3, 2022

@tfranzel for my project I will try to create a (temporary?) workaround. I planned to inherit from ManifestStaticFilesStorage and add directories to the manifest (if it is possible) or/and add the files as exemptions. For your library... Well, I think Django expects you to wrap each file with the static() method, not a common subpath. This storage changes file names (adds a hash), so joining path parts shouldn't work. Sorry, I haven't checked your sources in details to understand the best way to deal with the problem.

@paulsmirnov
Copy link

I hope I'll have more info when I finish my workaround.

@tfranzel
Copy link
Owner

tfranzel commented Oct 4, 2022

@paulsmirnov let me know your findings. I will set aside some time too.

@paulsmirnov
Copy link

I went with a workaround for now. Just in case anybody stumbles upon this discussion, here is the code:

from django.contrib.staticfiles.storage import ManifestFilesMixin, StaticFilesStorage


class MyFilesMixin(ManifestFilesMixin):
    passthrough_names = [
        'drf_spectacular_sidecar/swagger-ui-dist',
        'drf_spectacular_sidecar/redoc',
    ]

    def stored_name(self, name):
        if name in self.passthrough_names:
            return name
        return super().stored_name(name)


class MyStaticFilesStorage(MyFilesMixin, StaticFilesStorage):
    pass

and then in settings.py use this class instead of ManifestStaticFilesStorage:

-STATICFILES_STORAGE = 'django.contrib.staticfiles.storage.ManifestStaticFilesStorage'
+STATICFILES_STORAGE = 'path.to.storage.MyStaticFilesStorage'

As for the possible fix in the library, @tfranzel I think you should consider changing the way you refer to static files in the templates. See https://docs.djangoproject.com/en/4.1/howto/static-files/ and https://stackoverflow.com/questions/16655851/django-1-5-how-to-use-variables-inside-static-tag for the details. E.g., for drf_spectacular/templates/drf_spectacular/swagger_ui.html something like this could work:

-<link rel="stylesheet" href="{{ dist }}/swagger-ui.css">
+<link rel="stylesheet" href="{% with dist|add:'/swagger-ui.css' as swagger_css %}{% static swagger_css %}{% endwith %}">

This will also require changing _swagger_ui_dist() method to avoid calling static(), and also _swagger_ui_favicon() to call static() and avoid concatenation. Maybe something else---need to track down all the consequenses.

I could try this and prepare a PR in a while, though I'm afraid it will require thorough testing.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
enhancement New feature or request fix confirmation pending issue has been fixed and confirmation from issue reporter is pending
Projects
None yet
Development

No branches or pull requests

3 participants