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

api-defaults will set content-types automatically which is incompatible with wrap-json-response #20

Closed
jiacai2050 opened this Issue Jan 2, 2017 · 10 comments

Comments

Projects
None yet
2 participants
@jiacai2050

jiacai2050 commented Jan 2, 2017

I think most of developers will meet this issue, and solution to this issue is simple after I dig into src of both wrap-json-response and api-defaults

Is it more reasonable to set content-types default to false in api-defaults or add an option to set content-type forcefully in wrap-json-response ?

@weavejester

This comment has been minimized.

Show comment
Hide comment
@weavejester

weavejester Jan 2, 2017

Member

Sorry, I don't understand what you're trying to say. Did you mean "incompatible", and if so, why is it incompatible? wrap-content-types won't set the content type if its already been set by other middleware.

Member

weavejester commented Jan 2, 2017

Sorry, I don't understand what you're trying to say. Did you mean "incompatible", and if so, why is it incompatible? wrap-content-types won't set the content type if its already been set by other middleware.

@jiacai2050 jiacai2050 changed the title from api-defaults will set content-types automatically which is compatible with wrap-json-response to api-defaults will set content-types automatically which is incompatible with wrap-json-response Jan 3, 2017

@jiacai2050

This comment has been minimized.

Show comment
Hide comment
@jiacai2050

jiacai2050 Jan 3, 2017

Oh, sorry. It's "incompatible". I already updated the title.

I will show some codes to reproduce the incompatible issue.

(def app
  (-> handler
      (wrap-defaults api-defaults)
      wrap-json-response))

If I define app like above, wrap-json-response will not work, because Content-Type is already set in wrap-defaults.

I can work around this simply by switching the order of the two middlewares, but demos I search from google always put wrap-defaults, handler/api or handler/site at first then other middlewares following. Orders of middleware do matter a lot than I think at first, so in order to be newbie-friendly, ring-defaults can do something like I said in first comment. What do you think ?

jiacai2050 commented Jan 3, 2017

Oh, sorry. It's "incompatible". I already updated the title.

I will show some codes to reproduce the incompatible issue.

(def app
  (-> handler
      (wrap-defaults api-defaults)
      wrap-json-response))

If I define app like above, wrap-json-response will not work, because Content-Type is already set in wrap-defaults.

I can work around this simply by switching the order of the two middlewares, but demos I search from google always put wrap-defaults, handler/api or handler/site at first then other middlewares following. Orders of middleware do matter a lot than I think at first, so in order to be newbie-friendly, ring-defaults can do something like I said in first comment. What do you think ?

@weavejester

This comment has been minimized.

Show comment
Hide comment
@weavejester

weavejester Jan 3, 2017

Member

wrap-defaults should typically be the outermost middleware applied. I can add a note to the README to emphasize that.

Member

weavejester commented Jan 3, 2017

wrap-defaults should typically be the outermost middleware applied. I can add a note to the README to emphasize that.

@jiacai2050

This comment has been minimized.

Show comment
Hide comment
@jiacai2050

jiacai2050 Jan 3, 2017

Documents are less useful than defaults.

From my point of view, api-defaults should make sure response is in json format. is it more reasonable to add wrap-json-response in api-defaults ?

jiacai2050 commented Jan 3, 2017

Documents are less useful than defaults.

From my point of view, api-defaults should make sure response is in json format. is it more reasonable to add wrap-json-response in api-defaults ?

@weavejester

This comment has been minimized.

Show comment
Hide comment
@weavejester

weavejester Jan 3, 2017

Member

No, because an API is not necessarily going to return JSON, and because wrap-defaults needs to be the outermost middleware for other reasons. For example, wrap-params is part of wrap-defaults, and consumes the body InputStream.

Member

weavejester commented Jan 3, 2017

No, because an API is not necessarily going to return JSON, and because wrap-defaults needs to be the outermost middleware for other reasons. For example, wrap-params is part of wrap-defaults, and consumes the body InputStream.

@jiacai2050

This comment has been minimized.

Show comment
Hide comment
@jiacai2050

jiacai2050 Jan 3, 2017

Got your point.

For your convenience, I have drawn a flow chart to demo how middlewares work. you can add it in README.

how ring middleware work

you can edit this diagram here.

jiacai2050 commented Jan 3, 2017

Got your point.

For your convenience, I have drawn a flow chart to demo how middlewares work. you can add it in README.

how ring middleware work

you can edit this diagram here.

@weavejester

This comment has been minimized.

Show comment
Hide comment
@weavejester

weavejester Jan 3, 2017

Member

Thanks for the diagram, but the Ring-Defaults library isn't the place to be explaining middleware. The Ring wiki might be a better choice. However, your diagrams are also both a little inaccurate, I'm afraid.

Member

weavejester commented Jan 3, 2017

Thanks for the diagram, but the Ring-Defaults library isn't the place to be explaining middleware. The Ring wiki might be a better choice. However, your diagrams are also both a little inaccurate, I'm afraid.

@jiacai2050

This comment has been minimized.

Show comment
Hide comment
@jiacai2050

jiacai2050 Jan 3, 2017

image

Is this accurate ?

jiacai2050 commented Jan 3, 2017

image

Is this accurate ?

@weavejester

This comment has been minimized.

Show comment
Hide comment
@weavejester

weavejester Jan 3, 2017

Member

No, sorry. The diagram should look more like:

 +---------------+
 |  Middleware   |
 |  +---------+  |             +---------+      +--------+
 |  |         |<-- request ----|         |      |        |
 |  | Handler |  |             | Adapter |<---->| Client |
 |  |         |--- response -->|         |      |        |
 |  +---------+  |             +---------+      +--------+
 +---------------+

The middleware should wrap the handler, and there's only one adapter per connection to the client. In more complicated setups you can combine multiple handlers, e.g. Compojure works by combining "routes" (handlers that can return nil), and middleware can be applied to individual routes, as well as the top-level handler.

Member

weavejester commented Jan 3, 2017

No, sorry. The diagram should look more like:

 +---------------+
 |  Middleware   |
 |  +---------+  |             +---------+      +--------+
 |  |         |<-- request ----|         |      |        |
 |  | Handler |  |             | Adapter |<---->| Client |
 |  |         |--- response -->|         |      |        |
 |  +---------+  |             +---------+      +--------+
 +---------------+

The middleware should wrap the handler, and there's only one adapter per connection to the client. In more complicated setups you can combine multiple handlers, e.g. Compojure works by combining "routes" (handlers that can return nil), and middleware can be applied to individual routes, as well as the top-level handler.

@jiacai2050

This comment has been minimized.

Show comment
Hide comment
@jiacai2050

jiacai2050 Jan 4, 2017

Oh, thanks. My understanding is almost the same as what your diagram express. And I do know handlers can be combined just like blueprints in Flask, which is a great feature for modularity.

The point I want to emphasize is

Outmost middleware (middleware 3 in my diagram) will be the first to recevie the request, and the last to receive the response

jiacai2050 commented Jan 4, 2017

Oh, thanks. My understanding is almost the same as what your diagram express. And I do know handlers can be combined just like blueprints in Flask, which is a great feature for modularity.

The point I want to emphasize is

Outmost middleware (middleware 3 in my diagram) will be the first to recevie the request, and the last to receive the response

@jiacai2050 jiacai2050 closed this Jan 4, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment