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

nested-params supports multiple keys #204

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

nested-params supports multiple keys #204

wants to merge 1 commit into from

Conversation

kochb
Copy link

@kochb kochb commented May 13, 2015

nested-params currently processes only the :params, but wrap-params also assoc's :form-params and :query-params onto the query map. In cases where you need to differentiate between query and body params, wrap-nested-params will be of no use to you.

This change lets you specify which keys you want to process:

(-> app
   (wrap-nested-params {:params-keys [:params :form-params :query-params]})
   (wrap-params))

It may be more proper to process all three by default, because the current behavior of wrap-nested-params might be unexpected. I left it at the more conservative default of just :params for now for consistency across versions.

@weavejester
Copy link
Member

The *-params keys are left alone by design, because they're designed to accurately reflect the parameters parsed from various sources. The :params key is there for convenience only, and therefore can be changed by middleware if it aids convenience.

What's your use-case for wanting to restrict nested parameters to a particular origin?

@kochb
Copy link
Author

kochb commented May 13, 2015

It's not restricting nested params to a particular origin, nested parameter parsing should be applied to data from both. The problem occurs when I want to only retrieve parameters from a particular origin.

In our restful API, content can be submitted for write via the body on write requests, while on read requests query parameters can be used to filter results. They're two different sets of parameters that serve different purposes.

There is a small risk that a misbehaving client includes data in both the body and query params. The query params would contaminate or override the data supplied in the body, since :params is a merge of the two. You can't undo that merge if a key conflict occurred, you have to go back to the source in :form-params, which hasn't been processed by the nested-params middleware.

This all is more of a problem with wrap-params disregarding the differences between the two, but nested-params does not currently provide a way to cope with that.

Wanting to preserve the original data is a fair argument for not touching them by default, but the change introduced is opt-in behavior, by using the middleware you are consenting to have the query map manipulated as you requested anyway.

@weavejester
Copy link
Member

It sounds as if your particular use case might be better served with the introduction of wrap-query-params and wrap-form-params middleware. Then you could choose to limit GET routes to query parameters, and POST routes to form parameters.

@kochb
Copy link
Author

kochb commented May 13, 2015

That seems a bit odd - I should branch my middleware on a per-route basis? I could do it based on the request-method, but how do I support a POST request that uses a query parameter in addition to a request body? Query params and body params aren't an either/or - http bodies are traditionally only on POST/PUT/PATCH, but any http method accepts query params. A resource is permitted to support both.

I might be missing something here, what is the issue that you're seeing? Nested parameters are merely an encoding format, what problems are introduced by asking a middleware to decode a specific parameter?

@weavejester
Copy link
Member

The :*-params keys are currently guaranteed to be a particular format, and I'm somewhat against breaking that expectation. This change would also necessitate adding the same behavior to wrap-keyword-params, and the repetition of that bothers me.

It's possible I could expose more of the internal workings of the nested-params middleware, allowing one to write their own middleware more easily. But I don't like the idea of breaking expected behavior, even as an option.

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

Successfully merging this pull request may close these issues.

2 participants