ring-clojure / ring

Clojure HTTP server abstraction
MIT License
3.77k stars 520 forks source link

PROPOSAL: Implement all HTTP response types in ring.util.response #448

Open joodie opened 3 years ago

joodie commented 3 years ago

Context

When writing clojure web applications and middleware it's common to need HTTP responses with many different status codes, like 401 Unauthorized, 403 Forbidden and many others. A full list of possible codes can be found at https://developer.mozilla.org/en-US/docs/Web/HTTP/Status

Currently, ring.util.repsonse has functions to generate a limited set of response types:

The problem

The remaining response types are not implemented by ring.util.response, which causes friction.

First, as the programmer you generally forget which response types are implemented in ring.util.response and which are not, so you don't really know when you'll be running into the issue until you do. Also, when a response is implemented as a redirect, it does not match the response type name. That is, there is no found function, you have to use (response/redirect url :found) which may not be expected.

Once you need an unimplemted response type, you have to pull in an additional dependency just for having a 403 Forbidden response or similar, or you have to implement it yourself, risking silly mistakes (using the wrong status number can be a serious issue, and there are just too many status codes that it becomes hard to catch the issue even in code review).

Proposed Solution

It would be very useful to have a complete and correct set of response functions for every status code as part of ring-core, implemented directly in ring.util.response.

These functions will follow the format of the existing response generating functions mentioned above, for example:

(defn forbidden
  [body]
  {:status  403
   :headers {}
   :body    body})

Following this logic there would also be separate functions for moved-permanently, found etc, in addition to the existing redirect and redirect-after-post function.

Implications

This would extend the ring.util.response public interface with a fairly large set of functions, so clashes are possible with existing code that does a :refer-all or :use to import everything in ring.util.response and already defines any of the new names. So the following example would generate a warning:

(ns my.app
  (:use [ring.util.response]))

(defn forbidden
  [text]
  ...)

I consider this acceptable since it would only result in a warning and not break functionality - the above (defn forbidden ...) would shadow the one imported from ring.util.response - and in general in Clojure, use and refer-all are discouraged anyway for the above and other reasons.

In Clojure expanding an API with new vars is normally not considered a breaking change. Code that would break given this change is too clever for its own good IMO.

Alternatives

Vendoring response functions

We currently use this strategy and copy a list of functions and status codes over from project to project. This is simple, but distracting.

Use other libraries

There are a few candidates.

metosin/ring-http-response implements most of what we propose and is even intended as an almost replacement for ring.util.response but that pulls in potemkin (which is a suspicious dependency), and it's incomplete (for instance, 421 Misdirected Request is missing).

There is also macchiato-http which we haven't really investigated yet. And probably more.

The main problem from a developer point of view is that you'd need to find and evaluate these candidates when you're not expecting to have to pull in dependancy at all.

Conclusion

I've tried to make a good case for extending the ring-code library for better developer ergonomics and predictability. In my view, ring-core should include response functions for all HTTP status codes.

If the consensus is that this is a good solution I'm prepared to do the implementation work and prepare a PR.

weavejester commented 3 years ago

I'd rather keep any additional response type functions out of Ring core. The existing functions are being kept for compatibility purposes, but in retrospect they would have been better placed as part of a separate library.

However, I think I'd be fine with putting them in a new "ring/ring-responses" library, as long as the scope is sufficiently narrow and maintenance is minimal.

joodie commented 3 years ago

@weavejester do you mean a new artifact ring/ring-responses, including a new namespace ring.util.responses, as a dependency of the ring/ring meta artifact?

weavejester commented 3 years ago

No, it wouldn't be a dependency of ring/ring. There'd also need to be a better name for the namespace, as ring.util.responses is too similar to ring.util.response.

joodie commented 3 years ago

I think some of the best namespaces are already in use by metosin/ring-http-response.

If any new lib is not going to be included by default, its main advantage over that metosin lib is the status & visibility of being in de ring organization. I’m not convinced that’s enough of an advantage to the overall ecosystem to counter the negative effect of adding another potential dependency to evaluate.

It may make more sense to resolve the technical issues with ring-http-response and possibly have that library “officially blessed” in some way.

@weavejester what do you think?

weavejester commented 3 years ago

The ring-http-response library includes a lot of things that I wouldn't want in a Ring library, and it's not MIT licensed, so I'd rather not make it "official", even if the author gave consent to do so.

I don't believe there's any inherent need to have every official Ring dependency included in the ring/ring package. I also don't understand why adding a new dependency from the same source is somehow a negative. Whether a dependency is included in ring/ring or not, does not make it more or less trustworthy.

Even if we do include an additional dependency in future, I'd want that to be after a long period of evaluation. By including all of the HTTP response functions in ring/ring or ring/ring-core, we are saying that these are useful to the majority of developers. But are they?

joodie commented 3 years ago

I concur that adding a dependency from the same source should in this case not make much of a difference wrt dependency evaluation compared to an automatically included dep in ring/ring (though it makes for a little more friction for the developers who end up using the dep).

Considering names for the namespace and dependency, the main problem I guess is that response is the obviously "right" term to use here, but it's been taken multiple times because it's so obvious. Still I can't really think of a different word that would work as well in context.

Maybe ring.response is the solution?

weavejester commented 3 years ago

Perhaps ring.util.status-responses.