fix Content-Type header set for SVG files

[mbektas]

Content-Type for SVG icon is properly set in GatewayResourceHandler but overridden in the base handler to application/json. This change prevents overriding Content-Type if previously set.

Fixes #1216

[welcome]

Thanks for submitting your first pull request! You are awesome! 🤗

If you haven't done so already, check out Jupyter's Code of Conduct. Also, please make sure you followed the pull request template, as this will help us review your contribution more quickly.

You can meet the other Jovyans by joining our Discourse forum. There is also a intro thread there where you can stop by and say Hi! 👋

Welcome to the Jupyter community! 🎉

[kevin-bates]

Hi @mbektas - thank you for identifying the issue and providing a pull request. In looking into this just now (sorry, I hadn't seen your PR), I came to roughly the same conclusion about where the fix is located. I also added a test case since resources from a gateway are handled a bit differently than local resources (in that they don't go through the static file handler). Would you mind if I pushed the test update to your branch as part of this PR?

[mbektas]

hi @kevin-bates , sure, please feel free to push any updates as necessary.

[kevin-bates]

(Sorry for the delay. I just pushed the test update.)

[codecov]

Codecov Report

Base: 80.44% // Head: 39.37% // Decreases project coverage by -41.07% ⚠️

Coverage data is based on head (cf12df2) compared to base (ce8e7fc).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##             main    #1217       +/-   ##
===========================================
- Coverage   80.44%   39.37%   -41.07%     
===========================================
  Files          68       68               
  Lines        8134     8135        +1     
  Branches     1586     1587        +1     
===========================================
- Hits         6543     3203     -3340     
- Misses       1176     4733     +3557     
+ Partials      415      199      -216     

Impacted Files	Coverage Δ
jupyter_server/base/handlers.py	46.37% <0.00%> (-32.45%)	⬇️
jupyter_server/terminal/__init__.py	0.00% <0.00%> (-100.00%)	⬇️
jupyter_server/terminal/handlers.py	0.00% <0.00%> (-100.00%)	⬇️
jupyter_server/terminal/api_handlers.py	0.00% <0.00%> (-100.00%)	⬇️
jupyter_server/terminal/terminalmanager.py	0.00% <0.00%> (-100.00%)	⬇️
...pyter_server/services/contents/largefilemanager.py	11.11% <0.00%> (-72.73%)	⬇️
jupyter_server/services/contents/fileio.py	21.98% <0.00%> (-68.07%)	⬇️
...upyter_server/services/contents/filecheckpoints.py	22.35% <0.00%> (-67.65%)	⬇️
jupyter_server/gateway/handlers.py	0.00% <0.00%> (-62.28%)	⬇️
jupyter_server/services/sessions/sessionmanager.py	26.72% <0.00%> (-61.64%)	⬇️
... and 44 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[kevin-bates]

Hi @mbektas - I think the test failure is legitimate. When I step through the test_api.py::test_get_status test with a debugger, I see self._headers['Content-Type'] == 'text/html', so application/json never gets set. Given this appears to be the only test failure, it might be okay to set the mime-type in that particular handler.

FWIW, the way I resolved this was to add a set_content_type kwarg that callers set to override the application/json content-type, this way all changes are explicit since the caller (GatewayResourceHandler) has to request the override.

[mbektas]

@kevin-bates I am not that familiar with this codebase and came up with my solution after a quick debugging. Your solution looks more explicit and safe. I wouldn't mind closing this PR and going with your changes.

[kevin-bates]

Ok - that sounds good. I would like to add you as a co-author in my PR, do you mind if I do that?

[mbektas]

Ok - that sounds good. I would like to add you as a co-author in my PR, do you mind if I do that?

sounds great! thank you!

[kevin-bates]

Closing in favor of #1219. Thanks for your help on this @mbektas!