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

Add support for relationship-level links and meta attributes. #1454

Merged
merged 2 commits into from
Feb 8, 2016

Conversation

beauby
Copy link
Contributor

@beauby beauby commented Jan 21, 2016

Based on top of #1447.
Allows the following:

has_many :posts do
  link :self do
    href '//example.com/link_author/relationships/posts'
    meta stuff: 'value'
  end
  link :related do
    href '//example.com/link_author/posts'
    meta count: object.posts.count
  end
  include_data false
end

@beauby beauby force-pushed the association-blocks branch from 4239e50 to 061f1c0 Compare January 21, 2016 01:37
@beauby
Copy link
Contributor Author

beauby commented Jan 21, 2016

Travis is failing because of the rubocop config (https://github.com/rails-api/active_model_serializers/blob/master/.rubocop.yml#L40 – we should allow with_first_parameter).
Current:

hash[association.key] = JsonApi::Association.new(serializer,
                                                 association.serializer,
                                                 association.options,
                                                 association.links,
                                                 association.meta)

Expected by rubocop:

hash[association.key] = JsonApi::Association.new(serializer,
  association.serializer,
  association.options,
  association.links,
  association.meta)


hash
[@primary, @included]
Copy link
Member

Choose a reason for hiding this comment

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

any reason for the instance variables here?

@bf4
Copy link
Member

bf4 commented Jan 28, 2016

Looks pretty good, 😄 though, since both there are boundary changes along with refactoring and new behavior, I'll want to come back and look at what has 'changed'

@groyoh
Copy link
Member

groyoh commented Feb 4, 2016

Any update on this PR? Is there something needed to merge this PR? I could work on it if needed.

end
end

def id_for(serializer)
Copy link
Member

Choose a reason for hiding this comment

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

should be serializer.read_attribute_for_serialization(:id)

Copy link
Member

Choose a reason for hiding this comment

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

To be on the same page here, that would change the current behaviour, right? (see https://github.com/beauby/active_model_serializers/blob/master/lib/active_model/serializer/adapter/json_api.rb#L125)
And I guess you meant serializer.object.read_attribute_for_serialization(:id), right?

Copy link
Member

Choose a reason for hiding this comment

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

nope, I did indeedy mean to call it on the serializer, but @groyoh thanks for pointing out my error that this was just a direct port of the method

Copy link
Member

Choose a reason for hiding this comment

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

Oh you're right, I didn't realized that read_attribute_for_serialization was now defined on the serializer.

@bf4
Copy link
Member

bf4 commented Feb 4, 2016

@groyoh that's be great. Would you want to make a PR into @beauby 's branch? I can review and merge that. I think he's not around right now.

@bf4 bf4 merged commit 061f1c0 into rails-api:master Feb 8, 2016
@bf4
Copy link
Member

bf4 commented Feb 8, 2016

Merged. Added Needs follow up tag.

bf4 added a commit that referenced this pull request Feb 9, 2016
groyoh pushed a commit to groyoh/active_model_serializers that referenced this pull request Feb 10, 2016
PR rails-api#1454 was merged with some missing fixes. All these fixes are
addressed by this commit:
- Rename ActiveModel::Serializer::Adapter::JsonApi::Association to
ActiveModel::Serializer::Adapter::JsonApi::Relationship
- Move ActiveModel::Serializer::Adapter:: JsonApi::Relationship and
ActiveModel::Serializer::Adapter::JsonApi::ResourceIdentifier to
ActiveModel::Serializer::Adapter::JsonApi::ApiObjects module
- Add unit test for
ActiveModel::Serializer::Adapter::JsonApi::Relationship
- Add unit test for
ActiveModel::Serializer::Adapter::JsonApi::ResourceIdentifier
groyoh pushed a commit to groyoh/active_model_serializers that referenced this pull request Feb 10, 2016
PR rails-api#1454 was merged with some missing fixes. All these fixes are
addressed by this commit:
- Rename ActiveModel::Serializer::Adapter::JsonApi::Association to
ActiveModel::Serializer::Adapter::JsonApi::Relationship
- Move ActiveModel::Serializer::Adapter:: JsonApi::Relationship and
ActiveModel::Serializer::Adapter::JsonApi::ResourceIdentifier to
ActiveModel::Serializer::Adapter::JsonApi::ApiObjects module
- Add unit test for
ActiveModel::Serializer::Adapter::JsonApi::Relationship
- Add unit test for
ActiveModel::Serializer::Adapter::JsonApi::ResourceIdentifier
groyoh pushed a commit to groyoh/active_model_serializers that referenced this pull request Feb 10, 2016
PR rails-api#1454 was merged with some missing fixes. All these fixes are
addressed by this commit:
- Rename ActiveModel::Serializer::Adapter::JsonApi::Association to
ActiveModel::Serializer::Adapter::JsonApi::Relationship
- Move ActiveModel::Serializer::Adapter:: JsonApi::Relationship and
ActiveModel::Serializer::Adapter::JsonApi::ResourceIdentifier to
ActiveModel::Serializer::Adapter::JsonApi::ApiObjects module
- Add unit test for
ActiveModel::Serializer::Adapter::JsonApi::Relationship
- Add unit test for
ActiveModel::Serializer::Adapter::JsonApi::ResourceIdentifier
@groyoh groyoh mentioned this pull request Feb 10, 2016
groyoh pushed a commit to groyoh/active_model_serializers that referenced this pull request Feb 10, 2016
PR rails-api#1454 was merged with some missing fixes. All these fixes are
addressed by this commit:
- Rename ActiveModel::Serializer::Adapter::JsonApi::Association to
ActiveModel::Serializer::Adapter::JsonApi::Relationship
- Move ActiveModel::Serializer::Adapter:: JsonApi::Relationship and
ActiveModel::Serializer::Adapter::JsonApi::ResourceIdentifier to
ActiveModel::Serializer::Adapter::JsonApi::ApiObjects module
- Add unit test for
ActiveModel::Serializer::Adapter::JsonApi::Relationship
- Add unit test for
ActiveModel::Serializer::Adapter::JsonApi::ResourceIdentifier
groyoh pushed a commit to groyoh/active_model_serializers that referenced this pull request Feb 10, 2016
PR rails-api#1454 was merged with some missing fixes. All these fixes are
addressed by this commit:
- Rename ActiveModel::Serializer::Adapter::JsonApi::Association to
ActiveModel::Serializer::Adapter::JsonApi::Relationship
- Move ActiveModel::Serializer::Adapter:: JsonApi::Relationship and
ActiveModel::Serializer::Adapter::JsonApi::ResourceIdentifier to
ActiveModel::Serializer::Adapter::JsonApi::ApiObjects module
- Add unit test for
ActiveModel::Serializer::Adapter::JsonApi::Relationship
- Add unit test for
ActiveModel::Serializer::Adapter::JsonApi::ResourceIdentifier
@janvarljen
Copy link

is there a way to use Rails URL helpers in links?

like this:

has_many :posts do
  link :related do
    href api_author_posts_url(author: object)
  end
  include_data false
end

@sheldonbaker
Copy link

@janvarljen I did the following hack to get what you're after.

module ActiveModel
  class Serializer
    module Adapter
      class JsonApi
        class Link
          def routes
            Rails.application.routes.url_helpers
          end
        end
      end
    end
  end
end
has_many :posts do
  link :related do
    href routes.api_author_posts_url(author: object)
  end
  include_data false
end

@groyoh
Copy link
Member

groyoh commented Feb 23, 2016

@sheldonnbbaker Nice hack but I'd recommend:

UrlHelpers = Rails.application.routes.url_helpers
has_many :posts do
  link :related do
    href UrlHelpers.api_author_posts_url(author: object)
    # or
    # href Rails.application.routes.url_helpers.api_author_posts_url(author: object)
  end
  include_data false
end

if you want to avoid monkey patching AMS.

@janvarljen I don't think there is a cleaner way at the moment.

@bf4
Copy link
Member

bf4 commented Feb 23, 2016

@sheldonnbbaker @janvarljen @remear @groyoh the url_helpers issue is #1269 just needs a PR.. good stuff above

@bf4 bf4 deleted the association-blocks branch February 23, 2016 23:22
@remear remear self-assigned this Feb 24, 2016
@janvarljen
Copy link

@sheldonnbbaker @groyoh thx for the ideas. I'm doing the same thing atm but wanted to discuss a more elegant solution.

One more thing I think we're missing in these solutions is url_options context like :host, :protocol etc... we need for url_helpers to work as expected.

They way I do it:

in api_controller

serialization_scope :scope

def scope
  {
    url_options: url_options
  }
end

in base_serializer

include Rails.application.routes.url_helpers

def url_options
  scope[:url_options]
end

then I can use url helpers in serializer methods but not inside link block because we don't have access to the serializer methods

@remear
Copy link
Member

remear commented Feb 25, 2016

I'm working on adding the url_helpers to 0.10 so you won't need a workarounds like these. If all goes well, that means you should have the ability to do things like:

link :self { user_url(object) }
link :self { url_for([object, :users, @user]) }
link :self { "http://example.com/posts/#{object.id}" }
link :self, "http://example.com/user"

@janvarljen
Copy link

@remear that would be great. Ping me if you need any help

@bf4
Copy link
Member

bf4 commented Feb 26, 2016

ref: #1269

@bf4
Copy link
Member

bf4 commented Feb 28, 2016

Maybe a 'wip' pr?

On Thu, Feb 25, 2016 at 12:37 PM Jan Varljen notifications@github.com
wrote:

@remear https://github.com/remear that would be great. Ping me if you
need any help


Reply to this email directly or view it on GitHub
#1454 (comment)
.

@remear remear mentioned this pull request Mar 2, 2016
@remear
Copy link
Member

remear commented Mar 2, 2016

@janvarljen, @sheldonnbbaker Feedback is welcome on #1550, which aims to add url_helpers for links.

@rjsteen
Copy link

rjsteen commented Mar 10, 2016

Does anyone know when this functionality will be available in active_model_serializers gem?

@NullVoxPopuli
Copy link
Contributor

you can install from our master branch until we cut another release -- would that work?

@rjsteen
Copy link

rjsteen commented Mar 10, 2016

I am not seeing these changes on active_model_serializers:master

@remear
Copy link
Member

remear commented Mar 10, 2016

This PR and #1550, which adds Rails url_helpers, have been merged into the master branch. They are not yet available in a version available from RubyGems. You'll need to have your application use the master branch of this repository until the next release. If you're having other troubles, I suggest opening a new issue with more details.

@rjsteen
Copy link

rjsteen commented Mar 11, 2016

👍

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.

8 participants