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

Underscore incoming params before filtering during Deserialization #1986

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

Conversation

aliaporci
Copy link

@aliaporci aliaporci commented Nov 22, 2016

Purpose

TL;DR Ensure general use case of using snake case for permitted params i.e. jsonapi_parse(params, only: [:first_name]) even when deserializing dasherized or camel cased params.

Long version: In Rails-land, we generally default to snake case when describing variables, symbols, etc. Right now, if we have incoming params with kebab or camel cased fields, we have to match the case when deserializing in the controller.

E.g.

If we send dasherized parameters such as

{
  'data' => {
    'type' => 'users',
    'id' => 'blah',
    'attributes' => {
      'first-name' => 'Freddie',
      'last-name' => 'Mercury'
    }
  }
}

to a UserController containing this (fairly standard) method

def user_params
  ActiveModelSerializers::Deserialization.jsonapi_parse(params,
    only: [:first_name, :last_name])
end

the result will be empty user_params as both 'first-name' and 'last-name' are filtered out by the only option before the params are ever parsed.

The current (ugly and unsatisfying) workaround is to list out the kebabed or camel cased parameters as strings (i.e. only: ['first-name', 'last-name'] or to convince whoever is building a front end for your API to patch their Ember adapter or whatever to match whatever you're using (snake case FTW).

I'd much rather be able to use snake case for permitted params consistently across all of my Rails apps and not worry about remembering how one particular front end is configured to send params over (and not worry about having to change the casing of all of my permitted params in all of my controllers in the event the status quo changes!).

Changes

Call existing underscore function on incoming parameter field keys when filtering against options passed into the parse method

Caveats

Might be overkill.

Related GitHub issues

Possible made obsolete by #1927 if there's ever any movement on it.

Additional helpful information

Here's the blog post that let me know that I and the developers I talked to about this weren't alone: http://kyleshevlin.com/all-the-steps-needed-to-get-active-model-serializers-to-work-with-jsonapiadapter-and-jsonapiserializer-in-ember/

@mention-bot
Copy link

@aliaporci, thanks for your PR! By analyzing the history of the files in this pull request, we identified @domitian, @lawitschka and @remear to be potential reviewers.

@bf4
Copy link
Member

bf4 commented Nov 27, 2016

@aliaporci Thanks so much for the writeup and link!

I'm kind of surprised this issue hasn't come up before. This must have been a problem for all sorts of people. (I've generally been of the opinion that Ember does a better job at the adapter layer of handling Rails snake case than Rails does in producing and consuming kebab-case to avoid these issues :))

@bf4
Copy link
Member

bf4 commented Nov 27, 2016

we'll need a before/after benchmark of this too. Do you feel comfortable running bin/bench locally on master and on your branch?

@axsuul
Copy link

axsuul commented Mar 16, 2017

As an Ember developer, this needs to be implemented!

@bf4
Copy link
Member

bf4 commented Mar 16, 2017

Needs tests

@hardywu
Copy link

hardywu commented Apr 13, 2017

using this piece of codes for now.

props = ActiveModelSerializers::Deserialization.jsonapi_parse params
props = ActionController::Parameters.new(**props)
props.permit :attr1, :attr2

Hope the pull request #1928 get merged soon.

@hardywu
Copy link

hardywu commented Aug 21, 2019

any news?

@bf4
Copy link
Member

bf4 commented Aug 21, 2019

@hardywu I guess not :(

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.

5 participants