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

Move meta/meta_key handling inside adapter. #1233

Merged
merged 1 commit into from
Oct 5, 2015

Conversation

beauby
Copy link
Contributor

@beauby beauby commented Oct 5, 2015

The (toplevel) meta handling is clearly an adapter concern.

@beauby beauby force-pushed the move-root-to-adapter2 branch 2 times, most recently from 22c0bd0 to 56b5fb1 Compare October 5, 2015 04:57
assert_equal @serializer.meta, 'the meta'
assert_equal @serializer.meta_key, 'the meta key'
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Those readers simply do not exist anymore.

Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@beauby beauby force-pushed the move-root-to-adapter2 branch from 56b5fb1 to c4a137a Compare October 5, 2015 05:48
adapter = ActiveModel::Serializer::Adapter::Attributes.new(serializer)
actual = ActiveModel::SerializableResource.new([@blog], adapter: :attributes,
meta: { total: 10 })
.as_json
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting?

.as_json

feels so lonely

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you rather have it?

Copy link
Contributor

Choose a reason for hiding this comment

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

ActiveModel::SerializableResource.new(
  [@blog],
  adapter: :attributes,
  meta: { total: 10 }
).as_json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I could live with that. Will update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@beauby beauby force-pushed the move-root-to-adapter2 branch from c4a137a to fab6f3f Compare October 5, 2015 17:00
@NullVoxPopuli
Copy link
Contributor

can you rebase?

@NullVoxPopuli
Copy link
Contributor

thanks!

@beauby
Copy link
Contributor Author

beauby commented Oct 5, 2015

Wait, I'll rebase.

@beauby beauby force-pushed the move-root-to-adapter2 branch 2 times, most recently from 49f0c88 to 503bfe9 Compare October 5, 2015 17:06
@beauby
Copy link
Contributor Author

beauby commented Oct 5, 2015

Rebased.

@NullVoxPopuli
Copy link
Contributor

woo!

NullVoxPopuli added a commit that referenced this pull request Oct 5, 2015
Move `meta`/`meta_key` handling inside adapter.
@NullVoxPopuli NullVoxPopuli merged commit d02cd30 into rails-api:master Oct 5, 2015
@@ -37,11 +37,11 @@ def cache_check(serializer)
private

def meta
serializer.meta if serializer.respond_to?(:meta)
instance_options.fetch(:meta, nil)
Copy link
Member

Choose a reason for hiding this comment

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

👍

@trek
Copy link
Contributor

trek commented Oct 28, 2015

Did this break existing examples for supplying meta from with a CollectionSerializer? It doesn't seem like that's possible now. Should those be updated to show to override the adapter for this behavior?

@beauby
Copy link
Contributor Author

beauby commented Oct 28, 2015

@trek Not sure I understand what you meant. Could you sketch some code to show what you have in mind?

@trek
Copy link
Contributor

trek commented Oct 28, 2015

The pagination example here doesn't work after moving meta to the Adapter (where it does obviously better fit).

It looks like we didn't also add the documentation for centrally overriding serialization to include meta. The JSON API Adapter, for example, will include links automatically but the only way we could figure out how to get extra keys into the top level object (in this case for pagination) was creating our own adapter and overriding serializable_hash_for_collection

class JsonApiWithMetaAdapter < ActiveModel::Serializer::Adapter::JsonApi
  def serializable_hash_for_collection(options)
    hash = super
    object = serializer.object # object here is a collection, using CollectionSerializer

    if serializer.paginated?
      hash[:meta] = hash[:meta] || {}
      hash[:meta][:pagination] = {
        current_page: object.current_page,
        total_pages: object.total_pages,
        total_entries: object.total_entries
      }
    end

    hash
  end
end

There might be a better way, but we couldn't find an interface for it on master. It'd be good to document getting keys into the top level hash.

@beauby
Copy link
Contributor Author

beauby commented Oct 28, 2015

@trek You are right. Note that, in master, you can always provide the meta option to your render call, which will be forwarded to the adapter, and included in the response document (render json: posts, meta: my_pagination_hash). It is probably worth thinking about the possibility of including a resource/serializer-level meta directive for the Json adapter, but the example in the docs was clearly a hack.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants