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.

Sign up for GitHub

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

Jump to bottom

671 enhancment replace starlettes staticfiles with our own implementation #739

Merged
merged 28 commits into from
Nov 5, 2022
Merged

671 enhancment replace starlettes staticfiles with our own implementation #739

merged 28 commits into from
Nov 5, 2022

Conversation

Goldziher
Copy link
Contributor

@Goldziher Goldziher commented Nov 2, 2022 •
edited

PR Checklist

  • Have you followed the guidelines in CONTRIBUTING.md?
  • Have you got 100% test coverage on new code?
  • Have you updated the prose documentation?
  • Have you updated the reference documentation?

@Goldziher Goldziher linked an issue Nov 2, 2022 that may be closed by this pull request
Enhancment: Replace starlette's StaticFiles with our own implementation #671
Closed
@lgtm-com
Copy link

lgtm-com bot commented Nov 2, 2022

This pull request introduces 3 alerts when merging 118b4d5 into 34740bd - view on LGTM.com

new alerts:

  • 3 for Unused import

@provinzkraut
Copy link
Member

I'm not so sure about the hard dependency on fsspec. I think it might be better if we only include an abstract backend in starlite, that can be backed by fsspec.

  1. This would allow users to bring their own, without the fsspec dependency
  2. One less hard dependency for starlite
  3. The basic static-files functionality could relatively easily be implemented without fsspec
  4. Better separation of concerns: fsspec backends go into starlite-storage

@Goldziher Goldziher force-pushed the 671-enhancment-replace-starlettes-staticfiles-with-our-own-implementation branch from 118b4d5 to 2eafc9c Compare November 3, 2022 20:45
@lgtm-com
Copy link

lgtm-com bot commented Nov 3, 2022

This pull request introduces 3 alerts when merging 2eafc9c into 05c0db4 - view on LGTM.com

new alerts:

  • 3 for Unused import

@Goldziher Goldziher marked this pull request as ready for review November 3, 2022 21:27
@lgtm-com
Copy link

lgtm-com bot commented Nov 3, 2022

This pull request introduces 1 alert when merging 075f924 into 05c0db4 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 3, 2022

This pull request introduces 1 alert when merging b6d4b9c into 28ddc84 - view on LGTM.com

new alerts:

  • 1 for Unused import

peterschutt
peterschutt approved these changes Nov 3, 2022
View reviewed changes
Copy link
Contributor

@peterschutt peterschutt left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apologies for the fairly superficial review, I'm not active in this space so will rely on others to weigh in on the technical details.

Cheers!

ETA: one other thing that stood out was that sometimes we use FS and others FileSystem, e.g., fs.py, FSInfo, FileSystemAdapter, BaseLocalFileSystem. You've highlighted your disdain for abbreviations to me before, but also I understand that fs is a fairly ubiquitous/unambiguous abbr, so just thought I'd mention, but happy to go with what you have if everyone happy with it.

Also, made that commit for the typo as that line wasn't available for a suggestion, so figured I'd just sneak the fix in with this PR.

starlite/config/static_files.py Show resolved Hide resolved
starlite/config/static_files.py Outdated Show resolved Hide resolved
starlite/response/file.py Outdated Show resolved Hide resolved
starlite/response/file.py Outdated Show resolved Hide resolved
starlite/static_files/base.py Outdated Show resolved Hide resolved
starlite/static_files/base.py Outdated Show resolved Hide resolved
starlite/types/__init__.py Show resolved Hide resolved
starlite/types/__init__.py Show resolved Hide resolved
starlite/utils/fs.py Outdated Show resolved Hide resolved
starlite/utils/fs.py Outdated Show resolved Hide resolved
Goldziher and others added 8 commits November 4, 2022 18:06
@Goldziher @peterschutt
Update starlite/config/static_files.py
c43b137 
Co-authored-by: Peter Schutt <peter@topsport.com.au>
@Goldziher @peterschutt
Update starlite/response/file.py
96dfdb4 
Co-authored-by: Peter Schutt <peter@topsport.com.au>
@Goldziher @peterschutt
Update starlite/response/file.py
e47500a 
Co-authored-by: Peter Schutt <peter@topsport.com.au>
@Goldziher @peterschutt
Update starlite/static_files/base.py
c44b736 
Co-authored-by: Peter Schutt <peter@topsport.com.au>
@Goldziher @peterschutt
Update starlite/static_files/base.py
74dc148 
Co-authored-by: Peter Schutt <peter@topsport.com.au>
@Goldziher @peterschutt
Update starlite/types/__init__.py
33dc9c9 
Co-authored-by: Peter Schutt <peter@topsport.com.au>
@Goldziher @peterschutt
Update starlite/static_files/base.py
e42b57d 
Co-authored-by: Peter Schutt <peter@topsport.com.au>
@Goldziher @peterschutt
Update starlite/static_files/base.py
1172ce1 
Co-authored-by: Peter Schutt <peter@topsport.com.au>
@lgtm-com
Copy link

lgtm-com bot commented Nov 4, 2022

This pull request introduces 1 alert when merging 1b1c811 into 28ddc84 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 4, 2022

This pull request introduces 1 alert when merging 4444b69 into 28ddc84 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 4, 2022

This pull request introduces 1 alert when merging f7f2a56 into 28ddc84 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 4, 2022

This pull request introduces 1 alert when merging d5641a8 into 28ddc84 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 5, 2022

This pull request introduces 1 alert when merging dfaff3f into 3ee86e9 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 5, 2022

This pull request introduces 1 alert when merging 6e12d21 into 3ee86e9 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 5, 2022

This pull request introduces 1 alert when merging 1eaadef into 3ee86e9 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 5, 2022

This pull request introduces 1 alert when merging 4379747 into 3ee86e9 - view on LGTM.com

new alerts:

  • 1 for Unused import

@lgtm-com
Copy link

lgtm-com bot commented Nov 5, 2022

This pull request introduces 1 alert when merging 9feff05 into 3ee86e9 - view on LGTM.com

new alerts:

  • 1 for Unused import

@sonarcloud
Copy link

sonarcloud bot commented Nov 5, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

98.8% 98.8% Coverage
0.0% 0.0% Duplication

@lgtm-com
Copy link

lgtm-com bot commented Nov 5, 2022

This pull request introduces 1 alert when merging 2cfb02a into 3ee86e9 - view on LGTM.com

new alerts:

  • 1 for Unused import

@Goldziher Goldziher merged commit cf76fa6 into main Nov 5, 2022
@Goldziher Goldziher deleted the 671-enhancment-replace-starlettes-staticfiles-with-our-own-implementation branch November 5, 2022 12:44
provinzkraut pushed a commit that referenced this pull request May 6, 2024
@peterschutt @JacobCoffee
Merge pull request from GHSA-83pv-qr33-2vcf
d4f4095 
Fix for vulnerability introduced in #739, caused by failure to normalize the path part extracted from URL before serving data from a static directory.

See GHSA-83pv-qr33-2vcf for specific details.

Co-authored-by: Jacob Coffee <jacob@z7x.org>
Co-authored-by: Brian Edgar Ré <brian@192.168.1.4>
provinzkraut pushed a commit that referenced this pull request May 6, 2024
@peterschutt @JacobCoffee
Merge pull request from GHSA-83pv-qr33-2vcf
979453a 
Fix for vulnerability introduced in #739, caused by failure to normalize the path part extracted from URL before serving data from a static directory.

See GHSA-83pv-qr33-2vcf for specific details.

Co-authored-by: Jacob Coffee <jacob@z7x.org>
Co-authored-by: Brian Edgar Ré <brian@192.168.1.4>
provinzkraut pushed a commit that referenced this pull request May 6, 2024
@JacobCoffee @peterschutt
Merge pull request from GHSA-83pv-qr33-2vcf
57e706e 
Fix for vulnerability introduced in #739, caused by failure to normalize the path part extracted from URL before serving data from a static directory.

See GHSA-83pv-qr33-2vcf for specific details.

Co-authored-by: Brian Edgar Ré <brian@192.168.1.4>
Co-authored-by: Peter Schutt <peter.github@proton.me>
peterschutt added a commit that referenced this pull request May 6, 2024
@JacobCoffee @peterschutt
Merge pull request from GHSA-83pv-qr33-2vcf
6521014 
Fix for vulnerability introduced in #739, caused by failure to normalize the path part extracted from URL before serving data from a static directory.

See GHSA-83pv-qr33-2vcf for specific details.

Co-authored-by: Brian Edgar Ré <brian@192.168.1.4>
Co-authored-by: Peter Schutt <peter.github@proton.me>
peterschutt added a commit that referenced this pull request May 8, 2024
@JacobCoffee @peterschutt
Merge pull request from GHSA-83pv-qr33-2vcf
8125e6f 
Fix for vulnerability introduced in #739, caused by failure to normalize the path part extracted from URL before serving data from a static directory.

See GHSA-83pv-qr33-2vcf for specific details.

Co-authored-by: Brian Edgar Ré <brian@192.168.1.4>
Co-authored-by: Peter Schutt <peter.github@proton.me>
peterschutt added a commit that referenced this pull request May 8, 2024
@peterschutt @JacobCoffee
chore(release): prepare release v2.8.3 (#3474)
47b6540 
* fix: pass branch to `create_draft_release()`

* Merge pull request from GHSA-83pv-qr33-2vcf

Fix for vulnerability introduced in #739, caused by failure to normalize the path part extracted from URL before serving data from a static directory.

See GHSA-83pv-qr33-2vcf for specific details.

Co-authored-by: Brian Edgar Ré <brian@192.168.1.4>
Co-authored-by: Peter Schutt <peter.github@proton.me>

* chore(release): prepare release v2.8.3

---------

Co-authored-by: Jacob Coffee <jacob@z7x.org>
Co-authored-by: Brian Edgar Ré <brian@192.168.1.4>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peterschutt peterschutt peterschutt approved these changes

@jtraub jtraub Awaiting requested review from jtraub

@cofin cofin Awaiting requested review from cofin

@seladb seladb Awaiting requested review from seladb

@provinzkraut provinzkraut Awaiting requested review from provinzkraut

@jonadaly jonadaly Awaiting requested review from jonadaly

@infohash infohash Awaiting requested review from infohash

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Enhancment: Replace starlette's StaticFiles with our own implementation
3 participants
@Goldziher @provinzkraut @peterschutt