sqlpage / SQLPage

Fast SQL-only data application builder. Automatically build a UI on top of SQL queries.
https://sql.datapage.app
MIT License
1.57k stars 89 forks source link

feat: add 404.sql handler #544

Closed wucke13 closed 1 month ago

wucke13 commented 1 month ago

Fixes #529 .

@lovasoa Is this an acceptable business logic to you? I'm sure the code can be cleaned up, and there might be an argument for using PathBuf directly instead of doing String manipulation, but besides that?

lovasoa commented 1 month ago

Hi @wucke13 ! The logic seems to match exactly what we discussed; everything good for me!

If you can clean up the code, and maybe put it in a separate function, I'll merge and this will be in the next version.

Thank you for your contribution!

lovasoa commented 1 month ago

I'm not sure what the best way to teach this new feature to users is. I think we should have 404.sql files in at least some of the examples on this repository.

We could also mention its existence in the documentation of the json component and the path function. We could also mention it in configuration.md.

wucke13 commented 1 month ago

I definitely plan on adding an example. I'm quite indifferent on one thing: Should we do the path traversal stuff using a String or a PathBuf. What I currently like about using a String, is that we can discern a/b/c/ and /a/b/c. In the former case we want to check /a/b/c/404.sql, but in the latter case we don't. I think this nuance gets lost when converting the String to a PathBuf. @lovasoa Would you agree on using a String?

lovasoa commented 1 month ago

Yes, we should use a string and not a PathBuf. If there are other places where I used a PathBuf to represent a URL path, it is a mistake. PathBufs represent a platform-specific local file path; we want URL path handling to work exactly the same on all platforms.

wucke13 commented 1 month ago

Ok, I now have a cleaner solution. Please let me know if you are happy with it as well. If yes, I'll also push an example, squash the first two commits and hopefully then we are ready for merge :smile: .

Edit: nevermind, there is still a bug in it.
Edit 2: now it is ready!

wucke13 commented 1 month ago

@lovasoa I adjusted again. Most notably, now the serve_fallback call is only made when the error is 404:

// On 404/NOT_FOUND error, fall back to `404.sql` handler if it exists
let response = match maybe_response {
    // File could not be served due to a 404 error. Try to find a user provide 404 handler in
    // the form of a `404.sql` in the current directory. If there is none, look in the parent
    // directeory, and its parent directory, ...
    Err(e) if e.as_response_error().status_code() == StatusCode::NOT_FOUND => {
        serve_fallback(&mut service_request, e).await?
    }

    // Either a valid response, or an unrelated error that shall be bubbled up.
    e => e?,
};

I'm not sure how to test this elegantly, though at least at a superficial glance this seems to be a pretty robust implementation of "only ever try to do something when there is an 404 Not Found error".

I also added an example, that both illustrates the use and explains the fallback mechanism in prose. While I did mention custom error handling and implementing REST APIs as potential use-cases, I intentionally did not pitch the use of this feature as gateway to dynamic routing in general, I feel that goes against the overall spirit of SQLpage. Creative users anyway will do what they want :smile: , but we don't have to encourage it in the examples.

lovasoa commented 1 month ago

Great! Thank you @wucke13 !

You can add a test in tests/index.rs that makes a request to tests/xyz.sql that doesn't exist, and add a tests/404.sql.

Maybe we can still mention using it for APIs in the documentation of the json component.

wucke13 commented 1 month ago

@lovasoa Done. Also squashed a little for a clean history.

wucke13 commented 1 month ago

I'd be glad if we can get this done rather sooner or later, my motivation is somewhat depleted.

lovasoa commented 1 month ago

Thank you @wucke13 ! This PR is going to be really useful! I'll try to reference the new feature more in the docs to make it more discoverable