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 adapter with empty array does not maintain root key #1153

Closed
meltingice opened this issue Sep 15, 2015 · 14 comments
Closed

JSON adapter with empty array does not maintain root key #1153

meltingice opened this issue Sep 15, 2015 · 14 comments

Comments

@meltingice
Copy link

If you attempt to serialize an empty array with the JSON adapter and ArraySerializer, the root key becomes an empty string. This issue is present in 0.10.0.rc2 and the latest master branch commit at this time (17c353826056bc3eac3e6d6a2fd081af175bb6ad).

Example Code

class Api::V1::ArticlesController < Api::V1::BaseController
  def index
    @articles = []
    render json: @articles, each_serializer: Api::V1::ArticleSerializer
  end
end

Expected

{
  "articles": []
}

Actual

{
  "": []
}
@beauby
Copy link
Contributor

beauby commented Sep 15, 2015

Thing is, when you have an empty array, there is no real way for AMS to infer the type of resources you would have had in it (at least not based on the resources themselves since, well there aren't any). Two possible workarounds:

  • explicitly specify the root option
  • make a custom collection serializer that inherits from ArraySerializer that sets the root for you.

@NullVoxPopuli
Copy link
Contributor

AMS could attempt to safe_constantize the model part of {model}Serializer and derive the root key from that?

@meltingice
Copy link
Author

From a design standpoint, is there ever really an instance where you wouldn't have an array of homogenous object types? At least, for the JSON adapter? I don't see why AMS couldn't deduce the root key based on the serializer. The workarounds are fine, but it was definitely a surprise to me how it reacted in this instance. Doesn't seem like a good expected result.

Ryan LeFevre (@meltingice)
HODINKEE, Inc.
Sr. Software Engineer

_____________________________

From: Lucas Hosseini notifications@github.com
Sent: Tuesday, September 15, 2015 5:45 PM
Subject: Re: [active_model_serializers] JSON adapter with empty array does not maintain root key (#1153)
To: rails-api/active_model_serializers active_model_serializers@noreply.github.com
Cc: Ryan LeFevre meltingice8917@gmail.com

Thing is, when you have an empty array, there is no real way for AMS to infer the type of resources you would have had in it (at least not based on the resources themselves since, well there aren't any). Two possible workarounds: explicitly specify the root option make a custom collection serializer that inherits from ArraySerializer that sets the root for you.


Reply to this email directly or view it on GitHub.

@NullVoxPopuli
Copy link
Contributor

@beauby @joaomdmoura how do you feel about inferring the root name in this scenario? Can you think of any down sides?

@beauby
Copy link
Contributor

beauby commented Sep 15, 2015

I think we should do it but only if it cannot be inferred from the resources themselves (mainly because there are none), otherwise it will bring pain when serializing with a custom serializer (like PostPreviewSerializer).

@NullVoxPopuli
Copy link
Contributor

yeah, that's what I was thinking.

@NullVoxPopuli
Copy link
Contributor

Imma get a PR for this

@NullVoxPopuli
Copy link
Contributor

Addressed in #1156

@bf4
Copy link
Member

bf4 commented Sep 16, 2015

@meltingice are we discussing Array.new or Article.none?

If Array.new, AMS does nothing to resources that have no serializer, and there's no serializer for the Array primitive (0.10.0 master, 0.10.0.rc2, 0.9.3, 0.8.3.

The option for manually specifying the serializer is serializer, not each_serializer. Naming 💥 As I commented there:

# Get serializer either explicitly :serializer or implicitly from resource
# Remove :serializer key from serializer_opts
# Replace :serializer key with :each_serializer if present

Without serializer, it couldn't know the name of the instance variable. The best it could do would be given no serializer is found for the argument to json in the controller,

class ArticleController < ApplicationController
  def index
   @articles = []
    render json: @articles, adapter: :json #=> { articles: [] }, where the resource root comes from the current controller.
  end
end

If Article.none (an empty collection), then it could find the serializer implicitly.

@meltingice
Copy link
Author

Sorry, thought I replied to this, but must have forgotten to send the email. In this case it actually is Array.new because I'm not serializing an ActiveRecord::Relation.

@aserafin
Copy link

I've noticed it also doesn't respect the root: false option when we return empty array

@NullVoxPopuli
Copy link
Contributor

For root false, the attributes adapter would cover that

@bf4
Copy link
Member

bf4 commented Oct 19, 2015

@meltingice As I wrote in #1153 (comment) there's no serializer for an Array, so AMS won't do anything and you'll get [].to_json. ref: https://github.com/rails-api/active_model_serializers/blob/architecture/docs/ARCHITECTURE.md in #1253

@aserafin

I've noticed it also doesn't respect the root: false option when we return empty array

Is that related to this issue?

@bf4
Copy link
Member

bf4 commented Dec 9, 2015

Closing as stale. Please re-open if I closed this in error.

@bf4 bf4 closed this as completed Dec 9, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants