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

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

Closed
wants to merge 11 commits into from

Conversation

gmpassos
Copy link
Contributor

@gmpassos gmpassos commented Dec 5, 2023

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

Updated:

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

…luding `file` and `file_not_found` to be identified by other Shelf `Middleware`.
…o be identified by other Shelf `Middleware`.
@kevmoo
Copy link
Member

kevmoo commented Dec 5, 2023

i see where you're going here.

I'd want to chat with @natebosch or @devoncarew on their thoughts

This change should be documented and tested, too!

@gmpassos
Copy link
Contributor Author

gmpassos commented Dec 5, 2023

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

@gmpassos
Copy link
Contributor Author

gmpassos commented Dec 5, 2023

In my use case, I maintain an in-memory cache of shelf Responses. Then I need to verify if the cached Response File has changed using File.stat.

However, my Middleware currently has to recalculate the Response File before caching the Response. This leads to a doubling of the initial Response time due to the additional File resolution, involving calls to .existsSync and statSync, as well as the resolution of the Directory to index.html.

Keep in mind that the primary purpose of a cache is to enhance response time, and the negative impact of these extra calls to statSync is significant.

@gmpassos
Copy link
Contributor Author

What's the next step? Regards.

@kevmoo
Copy link
Member

kevmoo commented Dec 19, 2023

need changelog entry and tests

@natebosch
Copy link
Member

Is this still something you're interested in landing - can you take a look at adding tests?

@gmpassos
Copy link
Contributor Author

gmpassos commented Apr 2, 2024

This is important.

I will try to add some tests, but I don't have time for that in the next 15 days.

@natebosch
Copy link
Member

Can you give an example of how you plan to use this? Did you consider any alternative patterns, such as always storing the key shelf_static:file, and storing a separate boolean field for whether it was found?

Also cc @brianquinlan - can you think of any risks in keeping the File object in the heap for longer than we would have previously.

@gmpassos
Copy link
Contributor Author

gmpassos commented Apr 3, 2024

Can you give an example of how you plan to use this? Did you consider any alternative patterns, such as always storing the key shelf_static:file, and storing a separate boolean field for whether it was found?

You can take a look at real-world code that is waiting for this:
https://github.com/Colossus-Services/bones_api/blob/master/lib/src/bones_api_server.dart#L1788

Note that if 'shelf_static:file' and 'shelf_static:file_not_found' are not present, we need to recalculate the File path, validate it, and ensure (again) that it's inside the "root" directory (simulating the same shelf:createStaticHandler logic).

The context['file'] is then used by the cache "middleware", which will use the File instance to validate the cached files and dispose of the cached data if the File changes:
https://github.com/Colossus-Services/bones_api/blob/master/lib/src/bones_api_server_cache.dart#L351

@brianquinlan
Copy link
Contributor

Can you give an example of how you plan to use this? Did you consider any alternative patterns, such as always storing the key shelf_static:file, and storing a separate boolean field for whether it was found?

Also cc @brianquinlan - can you think of any risks in keeping the File object in the heap for longer than we would have previously.

A File object doesn't represent any native resources so keeping it alive shouldn't do anything except cost a bit of memory.

@gmpassos
Copy link
Contributor Author

gmpassos commented Jun 7, 2024

@natebosch @brianquinlan Next step?

@gmpassos
Copy link
Contributor Author

This PR will continue at PR
#436

@gmpassos gmpassos closed this Jun 14, 2024
@gmpassos gmpassos deleted the master branch June 14, 2024 05:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants