change AMS::JSON.include_root_in_json default value to false #6597

Merged
merged 1 commit into from Jun 7, 2012

4 participants

@frodsan

Changes:

  • Update include_root_in_json default value to false for default value to false for ActiveModel::Serializers::JSON.
  • Remove unnecessary change to include_root_in_json option in wrap_parameters template.
  • Update as_json documentation.
  • Fix JSONSerialization tests.

Problem:

It's confusing that AM serializers behave differently from AR,
even when AR objects include AM serializers module.

class User < ActiveRecord::Base; end

class Person
  include ActiveModel::Model
  include ActiveModel::AttributeMethods
  include ActiveModel::Serializers::JSON

  attr_accessor :name, :age

  def attributes
    instance_values
  end
end

user.as_json
=> {"id"=>1, "name"=>"Konata Izumi", "age"=>16, "awesome"=>true}
# root is not included

person.as_json
=> {"person"=>{"name"=>"Francesco", "age"=>22}}
# root is included

ActiveRecord::Base.include_root_in_json
=> false

Person.include_root_in_json
=> true

# different default values for include_root_in_json

Proposal:

Change the default value of AM serializers to false, update
the misleading documentation and remove unnecessary change
to false of include_root_in_json option with AR objects.

Result:

class User < ActiveRecord::Base; end

class Person
  include ActiveModel::Model
  include ActiveModel::AttributeMethods
  include ActiveModel::Serializers::JSON

  attr_accessor :name, :age

  def attributes
    instance_values
  end
end

user.as_json
=> {"id"=>1, "name"=>"Konata Izumi", "age"=>16, "awesome"=>true}
# root is not included

person.as_json
=> {"name"=>"Francesco", "age"=>22}
# root is not included

ActiveRecord::Base.include_root_in_json
=> false

Person.include_root_in_json
=> false

# same behaviour, more consistent

Fixes #6578.

@drogus
Ruby on Rails member

Looks good to me!

@rafaelfranca
Ruby on Rails member

Looks good to me too. But I think we will need a CHANGELOG entry.

@frodsan

done :)

@rafaelfranca rafaelfranca and 1 other commented on an outdated diff Jun 3, 2012
activemodel/CHANGELOG.md
@@ -1,5 +1,32 @@
## Rails 4.0.0 (unreleased) ##
+* Changed `AM::Serializers::JSON.include_root_in_json' default value to false.
+ Now, AM Serializers y AR objects have the same default behaviour. Fixes #6578.
@rafaelfranca
Ruby on Rails member

typo :)

@frodsan
frodsan added a note Jun 3, 2012

haha sorry for the spanglish

@frodsan
frodsan added a note Jun 3, 2012

fixed :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rafaelfranca
Ruby on Rails member
@carlosantoniodasilva
Ruby on Rails member

I'm ok with the changes, I just want to add that I have no idea why the docs are showing the result hashes like this:

user.as_json(root: true)
# => { "user": {"id": 1, "name": "Konata Izumi", "age": 16,
#               "created_at": "2006/08/01", "awesome": true} }

Considering this is invalid hash syntax, should it be changed?

@frodsan

I think it is invalid, i will change it.

Francesco Rodriguez change AMS::JSON.include_root_in_json default value to false
Changes:

* Update `include_root_in_json` default value to false for default value
  to false for `ActiveModel::Serializers::JSON`.
* Remove unnecessary change to include_root_in_json option in
  wrap_parameters template.
* Update `as_json` documentation.
* Fix JSONSerialization tests.

Problem:

It's confusing that AM serializers behave differently from AR,
even when AR objects include AM serializers module.

    class User < ActiveRecord::Base; end

    class Person
      include ActiveModel::Model
      include ActiveModel::AttributeMethods
      include ActiveModel::Serializers::JSON

      attr_accessor :name, :age

      def attributes
        instance_values
      end
    end

    user.as_json
    => {"id"=>1, "name"=>"Konata Izumi", "age"=>16, "awesome"=>true}
    # root is not included

    person.as_json
    => {"person"=>{"name"=>"Francesco", "age"=>22}}
    # root is included

    ActiveRecord::Base.include_root_in_json
    => false

    Person.include_root_in_json
    => true

    # different default values for include_root_in_json

Proposal:

Change the default value of AM serializers to false, update
the misleading documentation and remove unnecessary change
to false of include_root_in_json option with AR objects.

    class User < ActiveRecord::Base; end

    class Person
      include ActiveModel::Model
      include ActiveModel::AttributeMethods
      include ActiveModel::Serializers::JSON

      attr_accessor :name, :age

      def attributes
        instance_values
      end
    end

    user.as_json
    => {"id"=>1, "name"=>"Konata Izumi", "age"=>16, "awesome"=>true}
    # root is not included

    person.as_json
    => {"name"=>"Francesco", "age"=>22}
    # root is not included

    ActiveRecord::Base.include_root_in_json
    => false

    Person.include_root_in_json
    => false

    # same behaviour, more consistent

Fixes #6578.
ab11a27
@frodsan

Hey guys, i know that you are very busy, but i would like to know if there are any news regarding this?

@rafaelfranca rafaelfranca merged commit 6dfcc1b into rails:master Jun 7, 2012
@rafaelfranca
Ruby on Rails member

@frodsan merged, thanks

@frodsan

@rafaelfranca Do you think it will be nice to write something about this in the Upgrading Rails Guide?

@rafaelfranca
Ruby on Rails member

@frodsan yes please.

@senny senny added a commit to senny/rails that referenced this pull request Mar 4, 2013
@senny senny `ActiveRecord::Base.include_root_in_json` is `false` by default.
Closes #9459.

The PR #6597 unified the configuration for `include_root_in_json`
in AM and AR to `false`.
Later on with the refactoring commit: e030f26 the value in AR was
set to `true` but I think this was not on purpose.

With this commit both AM and AR will have the same configuration
for `include_root_in_json`, which is `false`.
8c7d401
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment