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

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

Open
joodie opened this issue Sep 17, 2021 · 7 comments
Open

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

joodie opened this issue Sep 17, 2021 · 7 comments

Comments

@joodie
Copy link

joodie commented Sep 17, 2021

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:

  • redirect with options for :moved-permanently, :found,
    :see-other, :temporary-redirect, and :permanent-redirect
  • redirect-after-post (deprecated)
  • created
  • bad-request
  • not-found

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
Copy link
Member

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
Copy link
Author

joodie commented Sep 17, 2021

@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
Copy link
Member

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
Copy link
Author

joodie commented Sep 18, 2021

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
Copy link
Member

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
Copy link
Author

joodie commented Sep 21, 2021

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
Copy link
Member

Perhaps ring.util.status-responses.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants