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

Add context to Response for including file and file_not_found to be identified by other shelf Middleware. #436

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

Add context to Response for including file and file_not_found to be identified by other shelf Middleware. #436

wants to merge 19 commits into from

Conversation

gmpassos
Copy link
Contributor

With the Response.context populated with the processed file, other middleware can manipulate the response in an optimized way and ensure the correct processed file. Without this, another middleware would need to guess the processed file and, in some cases, re-resolve File.stat and fix the directory path to the "index.html" path.

This is the continuation of the PR:
#395

(This was needed to release my shelf/master fork for another PR).

  • [✅ ] I’ve reviewed the contributor guide and applied the relevant portions to this PR.

@gmpassos
Add context to Response.notFound and Response.notModified for inc…
54608a7 
…luding `file` and `file_not_found` to be identified by other Shelf `Middleware`.
@gmpassos
Add context to Response for including file and file_not_found t…
7923a7f 
…o be identified by other Shelf `Middleware`.
@gmpassos
Merge remote-tracking branch 'origin/master'
9c0e03c
@gmpassos
Remove unnecessary context build when fileFound == null.
c449717
@gmpassos
Build fileNotFound and ensure to not expose a file path outside of …
688969f 
…the original fileSystemPath. Add `Response.context` documentation.
@gmpassos
createStaticHandler: update Response.context documentation.
9d22a81
@gmpassos
Merge branch 'dart-lang:master' into master
8ff6728
@gmpassos
Merge branch 'dart-lang:master' into master
136a921
@gmpassos
Merge branch 'dart-lang:master' into master
9f6aaad
@gmpassos
Merge branch 'dart-lang:master' into master
20677e6
@gmpassos
Merge branch 'dart-lang:master' into master
2a6b270
@gmpassos gmpassos mentioned this pull request Jun 14, 2024
Closed
@kevmoo
Copy link
Member

kevmoo commented Jun 14, 2024

Need some tests here, sir.

@gmpassos
* Response:
b6a704d 
  * Populate `context` with the file system paths used for resolution.
    * Possible entries: `shelf_static:file`, `shelf_static:file_not_found`, and `shelf_static:directory`.
@gmpassos
test Response.context.
ad79fd4
@gmpassos
Improve documentation
fbd5279
@gmpassos
Copy link
Contributor Author

gmpassos commented Jun 14, 2024 •
edited
Loading

Need some tests here, sir.

  • Implemented context for directories.
  • Tests: done ✅

@kevmoo
Copy link
Member

kevmoo commented Jun 14, 2024

@gmpassos – look at the windows failures. Looks like some path mundging needs to be done

@gmpassos
Copy link
Contributor Author

It's the path separator

test\symbolic_link_test.dart: access outside of root disabled access real file (failed)
  Expected: {
              'shelf_static:file': 'C:\\Users\\runneradmin\\AppData\\Local\\Temp\\dart_test_ee1d415b\\originals/index.html'
            }
    Actual: {
              'shelf_static:file': 'C:\\Users\\runneradmin\\AppData\\Local\\Temp\\dart_test_ee1d415b\\originals\\index.html'
            }
     Which: at location ['shelf_static:file'] is 'C:\\Users\\runneradmin\\AppData\\Local\\Temp\\dart_test_ee1d415b\\originals\\index.html' instead of 'C:\\Users\\runneradmin\\AppData\\Local\\Temp\\dart_test_ee1d415b\\originals/index.html'

@kevmoo
Copy link
Member

kevmoo commented Jun 14, 2024

Yep. You'll need to normalize those to handle windows. Always fun!

@gmpassos
Copy link
Contributor Author

gmpassos commented Jun 14, 2024 •
edited
Loading

Should be fixed now.

I was using path.join in a completely wrong way:

p.join(d.sandbox, 'foo/file.txt')

😅

@gmpassos
Copy link
Contributor Author

...needed an extra fix for createFileHandler, since it derivates the File path from the url.path

@kevmoo
Copy link
Member

kevmoo commented Jun 14, 2024

@devoncarew @natebosch – thoughts?

@gmpassos
Copy link
Contributor Author

Is it ready for publishing?

@kevmoo
Copy link
Member

kevmoo commented Jun 24, 2024

I'd like other eyes on this change first @gmpassos

@gmpassos
Copy link
Contributor Author

I'd like other eyes on this change first @gmpassos

Does the code need any adjustments?

Best regards.

@gmpassos
Copy link
Contributor Author

Any update?

Best regards.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kevmoo kevmoo kevmoo left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
package:shelf_static
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@gmpassos @kevmoo