SWI-Prolog / packages-http

The SWI-Prolog HTTP server and client libraries
24 stars 23 forks source link

Unexpected redirect for route with variable to same route with slash (/) #148

Closed spl closed 3 years ago

spl commented 3 years ago

Suppose I declare a route handler as follows:

:- http_handler(root(say/Message), say(Message), [method(get)]).

say(Message, _Request) :-
    format('Content-type: text/plain~n~n', []), 
    format('"~w"~n', [Message]).

I'm primarily interested in what the HTTP request put in Message (and possibly _Request in a more interesting example).

Now, suppose somebody sends a HTTP request to /say (without the final /). The server responds with a 301 Moved Permanently with Location: /say/, thus redirecting the requester to /say/. I find this surprising because I didn't explicitly define a route handler for /say. It's even more surprising when the routes have more than one variable:

:- http_handler(root(say2/Message1/Message2), say2(Message1, Message2), [method(get)]).

If I don't want these redirect routes to exist, I can do something like this:

:- http_handler(root(say), http_404([]), []).

However, I may have a lot of routes, and this adds extra work to ensure I cover them all with these clauses. This increases my maintenance and testing burden.

I think a better approach in these cases is to not redirect /say to /say/, because it's not obvious that that is always a desired result. That is, unless a specific handler for /say exists, the response should be the same for every other non-existent route (404).

P.S. I think this redirect is found in find_handler/3.

JanWielemaker commented 3 years ago

I see. This comes from the usual file system browsing. Just removing the redirect is probably a bad idea for compatibility as well as because it is quite often just what you want to happen. With two variables this is clearly a wrong idea as the handler we redirect to does not match. With a single variable that is less clear. Your solution with an additional 404 handler is of course fine.

Other solutions I see are not to redirect if target handler doesn't end with plain /, i.e., be more restrictive. We could also mark handlers as "no not redirect to me". Finally we could add a hook that allows formulating a redirect or not. Ideas?

spl commented 3 years ago

Other solutions I see are not to redirect if target handler doesn't end with plain /, i.e., be more restrictive.

That makes sense, I think. How would that work?

We could also mark handlers as "no not redirect to me".

That's an interesting thought. I don't see it being very useful in general, however.

Finally we could add a hook that allows formulating a redirect or not.

Hmm. Maybe it's not needed.

I just realized I could do something like this:

:- http_handler(/, http_404([]), [prefix]).

This seems to prevent the failed handler search, and I don't need to specify the per-path handler.

Also, if I want to do something with a particular path, I can do this:

:- http_handler(/, handle_unknown_path, [prefix]).                                                        

handle_unknown_path(Request) :-                                                                           
    member(path(Path), Request),
    ...

So, I guess I can work around the issue without a problem.

JanWielemaker commented 3 years ago

That makes sense, I think. How would that work?

Pushed a patch that does this (002b57c7649096ef8f121ff9318e587c4118ecfe)

This seems to prevent the failed handler search, and I don't need to specify the per-path handler.

That makes sense. Actually it is pretty neat as you can use it to control to behavior per sub-tree. Still think that by default redirecting to handlers that defined a pattern is not a great idea and the patch above is a move in the right direction. In the rare cases you want that an explicit redirect is more appropriate.

Thanks for the input.

spl commented 3 years ago

Great. Thanks!