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

_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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in 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