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

JSON API nicer include syntax #1128

Closed
willcosgrove opened this issue Sep 8, 2015 · 24 comments
Closed

JSON API nicer include syntax #1128

willcosgrove opened this issue Sep 8, 2015 · 24 comments

Comments

@willcosgrove
Copy link

I have a lot of nested associations in my app, and was getting tired of writing really long strings/arrays for the include option. So I wrote a little helper that turns an include that looks like this:

render json: @project_group, include: [projects: [project_tasks: [:part, :specification_revisions, :technique, :estimated_by, :comments, task: [:area]]]]

into an include value that looks like that

render json: @project_group, include: ["projects", "projects.project_tasks", "projects.project_tasks.task", "projects.project_tasks.task.area", "projects.project_tasks.part", "projects.project_tasks.specification_revisions", "projects.project_tasks.technique", "projects.project_tasks.estimated_by", "projects.project_tasks.comments"]

I tried to follow the format of what you can pass in to ActiveRecord::Relation#includes method.

It's a little messy but something along these lines:

  def sanitize_includes(includes, nest: [])
    case includes
    when Array
      includes.flat_map { |include|
        case include
        when Hash
          include.map { |key, value|
            [
              (nest + [key]).join('.'),
              sanitize_includes(value, nest: nest.push(key)),
            ]
          }
        else
          (nest + [include]).join('.')
        end
      }.flatten
    else
      includes
    end
  end

Does this sound helpful to anyone else? I wouldn't mind submitting a PR if it sounds like people are interested.

@NullVoxPopuli
Copy link
Contributor

I can work on this once my json adapter implementation is merged

@NullVoxPopuli
Copy link
Contributor

See #1113 and #1127

@beauby
Copy link
Contributor

beauby commented Sep 9, 2015

I like the idea of a better syntax for include, especially since nested resources are by essence a tree structure. However, I think it would make more sense to actually transform the text syntax (the current one) into a tree (i.e. an array of hash/symbols), than the opposite. Will open a PR for JSONAPI.

@NullVoxPopuli
Copy link
Contributor

But why use the string syntax? what does it get us?

@beauby
Copy link
Contributor

beauby commented Sep 9, 2015

@NullVoxPopuli I would like to get rid of the string syntax but

  1. it might be a concern to people who already use it
  2. for some simple cases (like no nesting), people might arguably prefer 'author, comments' to [ :author, :comments ]

@beauby
Copy link
Contributor

beauby commented Sep 9, 2015

Also, the string syntax makes it easy to sideload a "grand child" resource without sideloading the "child resource", which I don't really see a use case of, but still.

@NullVoxPopuli
Copy link
Contributor

@beauby we could support both?, esp if the string is gonna get converted to the array/hash syntax anyway.
The include array/hash could be used to do grandchild stuff, but there would need to be a defined way to skip the child's full serialization. maybe adding a relationships_only: true or something similar?

like:

[
  :author, 
  comments: {
    relationships_only: true, 
    associations: [:author]
  }
]

but that is getting kinda verbose... :-(

@jfelchner
Copy link
Contributor

I think you all need to stick to one or the other. Supporting both adds unneeded complexity for little benefit. I personally think that if strings are passed in, they should be converted over to a Hash. If a Hash is passed in, it is used as-is.

@beauby
Copy link
Contributor

beauby commented Sep 10, 2015

@jfelchner The idea is to support an "hybrid" syntax, which is made of Arrays of Symbols and Hashes of Arrays (in terms of grammar, it would be something along the lines of S → Array[Symbol, Hash[S]])), which is more user-friendly than a pure hash (that would be for instance { comments: { author: {} }, author: {}, posts: {} }, where the hybrid syntax would allow [ :author, :posts, comments: [:author]].

@jfelchner
Copy link
Contributor

Also, whatever does the conversion from the strings to hashes internally needs to be able to be called as a utility method. This way if the user wants to pass in include as a query param, the user can transform it into a Hash and then use some sort of whitelisting to make sure that someone didn't pass in something nefarious.

For example if I have an association called super_secret_stuff which I use for my admins, I don't want a user to be able to include super_secret_stuff.

@jfelchner
Copy link
Contributor

@beauby agreed 👍 That's what I meant by "Hash". I don't know why you didn't catch on. 😉 😄

@beauby
Copy link
Contributor

beauby commented Sep 10, 2015

@jfelchner That seems like an authorization concern I am not sure AMS should be responsible of. Currently you could get away with defining two serializers, one for the Admins, one for the Users, which define different sets of relationships, and only the relationships declared in the serializer can be included.

@beauby
Copy link
Contributor

beauby commented Sep 10, 2015

@jfelchner If we agree on the format of include, I think you will be pleased by #1131 (although it states that the string syntax is removed, I'm actually adding it back since I overlooked the JSONAPI spec on that one).

@jfelchner
Copy link
Contributor

@beauby no no. It definitely shouldn't be responsible for authorization. What I'm saying is something like:

class MyController
  def index
    render json: @project_group,
           include: sanitized_includes
  end

  private

  def sanitized_includes
    hashified_params = ActiveModel::Serializer.convert_string_includes_to_hash(params[:includes])

    ActionController::Parameters.new(hashified_params).permit(:blah, :blah)
  end
end

Obviously just pseudocode. The general gist is that users of AMS won't have to duplicate the logic of how the foo.bar.baz include string gets converted to { foo: { bar: :baz } or whatever. And they can then take that and apply whatever authorization logic is appropriate for that request.

@beauby
Copy link
Contributor

beauby commented Sep 10, 2015

@jfelchner Well, in that case you will be even more pleased with #1131, since I am currently precisely working on that "translator".

@jfelchner
Copy link
Contributor

@beauby Fantastic 😀

@estum
Copy link

estum commented Sep 10, 2015

I believe that include is designed to use with the same request parameter, as described in the jsonapi specs. So, it should stay to take a flatten comma-separated list of associations at least in controllers.

But, I think, there is a related but somehow not so popular opposite issue: when the include parameter is passed, it's expected to preload requested associations. To do that, include list should be translated to format readable by preloader. I.e., in opposite of subject issue, we need to convert flatten list ("comments,posts,posts.author") to a mixed array ([:comments, :posts => :author]). I have more or less fast raw implementation of this behavior, but I'm not sure about the chosen app layer where it happens.

@beauby
Copy link
Contributor

beauby commented Sep 10, 2015

@jfelchner, @estum: I just updated #1131 to handle both formats under the hood in the render call, with a helper function also directly accessible via ActiveModel::Serializers::Adapter.include_options_to_hash() which takes either a string or an array of symbols and hashes.

@jfelchner
Copy link
Contributor

@estum the JSON API spec only specifies what the client should send to the server. Which, as you stated is a string of comma separated values. But what happens to it after that point is up to the backend implementation to decide.

@jfelchner
Copy link
Contributor

@beauby you are awesome. I can't wait to get all this goodness merged in!

@joaomdmoura
Copy link
Member

Nice everyone, indeed we need to keep the string format, ppl have being using it and it looks similiar to what JSON API spec on what clients should send.
I'll check the PR, but I'm okay with the concept here.

@beauby
Copy link
Contributor

beauby commented Sep 13, 2015

@jfelchner It is merged. The following methods are now available as well:

  • ActiveModel::Serializer::Utils.include_string_to_hash: transforms a JSONAPI-style string into a hash;
  • ActiveModel::Serializer::Utils.include_args_to_hash: transforms an array of symbols and hashes into a hash.

Can we close this issue?

@willcosgrove
Copy link
Author

Looks awesome! Great work @beauby!

@joaomdmoura
Copy link
Member

Nice! 😄

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

6 participants