Skip to content

Move serialization logic into Serializer and CollectionSerializer #1766

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Jun 6, 2016
Merged

Move serialization logic into Serializer and CollectionSerializer #1766

merged 6 commits into from
Jun 6, 2016

Conversation

bf4
Copy link
Member

@bf4 bf4 commented Jun 1, 2016 •
edited
Loading

  • Remove most Attributes adapter recursion
  • Moving Attributes#serializable_hash_for_single_resource to Serializer#serialize

Extracted from #1684

Benchmark on my mbp just now looks good

# bin/bench

Benchmark results:
{
  "commit_hash": "7254d34",
  "version": "0.10.0",
  "rails_version": "4.2.6",
  "benchmark_run[environment]": "2.2.3p173",
  "runs": [
    {
      "benchmark_type[category]": "caching on: caching serializers: gc off",
      "benchmark_run[result][iterations_per_second]": 1039.028,
      "benchmark_run[result][total_allocated_objects_per_iteration]": 1277
    },
    {
      "benchmark_type[category]": "caching on: fragment caching serializers: gc off",
      "benchmark_run[result][iterations_per_second]": 716.661,
      "benchmark_run[result][total_allocated_objects_per_iteration]": 1729
    },
    {
      "benchmark_type[category]": "caching on: non-caching serializers: gc off",
      "benchmark_run[result][iterations_per_second]": 943.671,
      "benchmark_run[result][total_allocated_objects_per_iteration]": 1243
    },
    {
      "benchmark_type[category]": "caching off: caching serializers: gc off",
      "benchmark_run[result][iterations_per_second]": 841.05,
      "benchmark_run[result][total_allocated_objects_per_iteration]": 1277
    },
    {
      "benchmark_type[category]": "caching off: fragment caching serializers: gc off",
      "benchmark_run[result][iterations_per_second]": 575.214,
      "benchmark_run[result][total_allocated_objects_per_iteration]": 1729
    },
    {
      "benchmark_type[category]": "caching off: non-caching serializers: gc off",
      "benchmark_run[result][iterations_per_second]": 774.576,
      "benchmark_run[result][total_allocated_objects_per_iteration]": 1243
    }
  ]
}

bf4
bf4 reviewed Jun 1, 2016
View reviewed changes
lib/active_model/serializer.rb
@@ -98,6 +98,16 @@ def self.get_serializer_for(klass)
end
end

def self.include_directive_from_options(options)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

missing @api private. These methods are being held here for now, but may move around, be renamed, or disappear

@bf4 bf4 force-pushed the serializer_cleanup_4 branch 2 times, most recently from 13b7b9d to bbcb7bc Compare June 1, 2016 08:19
@NullVoxPopuli
Copy link
Contributor

@bf4 are you wanting to add a changelog entry?

anywho, the change looks good. I'd forgotten we had some serializer stuff in an adapter.

bf4
bf4 reviewed Jun 1, 2016
View reviewed changes
lib/active_model/serializer.rb
adapter.serializable_hash(adapter_opts)
adapter_opts = { include: '*' }.merge!(adapter_opts)
adapter_instance = ActiveModelSerializers::Adapter::Attributes.new(self, adapter_opts)
serialize(adapter_opts, {}, adapter_instance)
Copy link
Member Author

@bf4 bf4 Jun 1, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It really irritates me that this requires an adapter_instance (else I get a stack overflow).

The used adapter_instance interface is

  • cached_name : used in making cache key
  • class : used by fragment caching in fragment serializers
  • fragment_cache : used by fragment caching

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, all of those can be class methods, now that I think about it.

@bf4 bf4 force-pushed the serializer_cleanup_4 branch from bbcb7bc to caf4910 Compare June 4, 2016 20:00
bf4
bf4 reviewed Jun 4, 2016
View reviewed changes
lib/active_model/serializer.rb
adapter_opts = { include: '*', adapter: :attributes }.merge!(adapter_opts)
adapter = ActiveModelSerializers::Adapter.create(self, adapter_opts)
adapter.serializable_hash(adapter_opts)
adapter_opts = { include: '*' }.merge!(adapter_opts)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should use the JSONAPI default include directive, I think

@bf4 bf4 force-pushed the serializer_cleanup_4 branch from 99dc11a to 7254d34 Compare June 6, 2016 04:34
bf4
bf4 reviewed Jun 6, 2016
View reviewed changes
lib/active_model/serializer.rb
@@ -98,6 +98,22 @@ def self.get_serializer_for(klass)
end
end

# @api private
def self.include_directive_from_options(options)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should probably go in AMS or some other serialization-related class

@bf4 bf4 merged commit d191342 into rails-api:master Jun 6, 2016
@bf4 bf4 deleted the serializer_cleanup_4 branch June 6, 2016 04:56
@bf4
Copy link
Member Author

bf4 commented Jun 6, 2016

Merging just to keep momentum going

@y-yagi y-yagi mentioned this pull request Jun 6, 2016
Closed
@bf4 bf4 mentioned this pull request Jun 7, 2016
Merged
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
C: Cleanup V: 0.10.x
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@bf4 @NullVoxPopuli