Skip to content

Conversation

dylanahsmith
Copy link
Contributor

See the included regression test which shows the correct behaviour, and fails without the included fix.

Without this fix, saving nested resources results in a create rather than an update.

@travisbot
Copy link

This pull request fails (merged cb60edc into da3236c).

@dylanahsmith
Copy link
Contributor Author

This pull request fails (merged cb60edc into da3236c).

Wasn't experiencing this failure until I removed my Gemfile.lock, updated to the latest rails/master, rebuild and reinstalled activesupport and activemodel, bundle installed, then re-ran the tests. However, the latest activeresource/master has the same test failures now.

Edit: Ran a regression test repeating the above steps in a script.

ab11a2780fe8e059ffc8824df18037773a4f5200 is the first bad commit
commit ab11a2780fe8e059ffc8824df18037773a4f5200
Author: Francesco Rodriguez lrodriguezsanc@gmail.com
Date: Wed Jun 6 01:11:39 2012 -0500

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.

@mejibyte
Copy link

+1.

jeremy added a commit that referenced this pull request Oct 19, 2012
Mark nested resources as persisted when loaded from a response.
@jeremy jeremy merged commit 8f6a8b3 into rails:master Oct 19, 2012
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

Successfully merging this pull request may close these issues.

4 participants