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.

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

Add support for resource-level JSON API links. #1246

Merged
merged 2 commits into from
Nov 4, 2015
Merged

Add support for resource-level JSON API links. #1246

merged 2 commits into from
Nov 4, 2015

Conversation

beauby
Copy link
Contributor

@beauby beauby commented Oct 6, 2015

The syntax is as follows:

class PostSerializer < ActiveModel::Serializer
  # Block syntax for `meta` and/or dynamic value
  link :self do
    href "//example.com/link_author/#{object.id}"
    meta stuff: 'value'
  end

  # Direct syntax for plain strings without `meta`
  link :other, '//example.com/resource'
end

Update: new syntax.

@beauby beauby force-pushed the jsonapi-resource-links branch 2 times, most recently from e03110e to 9c5a9a3 Compare October 6, 2015 17:47
@joaomdmoura joaomdmoura mentioned this pull request Oct 6, 2015
Open
11 tasks
bf4
bf4 reviewed Oct 7, 2015
View reviewed changes
lib/active_model/serializer.rb
@@ -50,6 +50,10 @@ def self.type(type)
self._type = type
end

def self.links(&block)
self._links_block = block
end
Copy link
Member

Choose a reason for hiding this comment

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

My gut is to avoid long-lived blocks, as there may be a lot of these, and the memory would never be released

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alternative? You actually need this block every time to instantiate a serializer.

Copy link
Member

Choose a reason for hiding this comment

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

only if we design it this way. simple solution is just pass in links to the adapter until we need more

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I don't understand your last comment. Could you elaborate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note also that there would be at most 1 block per serializer class.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

An other option would be to declare each link as a lambda, but that would make much more stored procs...

Copy link
Member

Choose a reason for hiding this comment

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

I just don't think we need this code at all until someone needs it.

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'm not sure I understand. Are you saying we should not support JSON API resource-level links?

@bf4 bf4 mentioned this pull request Oct 7, 2015
Closed
@beauby beauby force-pushed the jsonapi-resource-links branch 2 times, most recently from 40c7839 to e1274b2 Compare October 7, 2015 20:24
@beauby beauby force-pushed the jsonapi-resource-links branch 4 times, most recently from d346c1e to 47ed229 Compare October 14, 2015 23:18
NullVoxPopuli
NullVoxPopuli reviewed Oct 15, 2015
View reviewed changes
lib/active_model/serializer.rb
@@ -159,6 +166,12 @@ def json_key
root || object.class.model_name.to_s.underscore
end

def links
Copy link
Contributor

Choose a reason for hiding this comment

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

can you add some documentation to this method since it's publicly available?

@antondomratchev
Copy link

@beauby Any updates on resolving the conflicts on this? I think this is a great missing feature! Good job on this.

@beauby
Copy link
Contributor Author

beauby commented Oct 20, 2015

@antondomratchev Thanks! The current issue is not so much about rebasing this, but about choosing the right way to handle links (since they are being declared pre-instantiation, but might rely on some stuff, such as object or scope that is only available in an instance).

@beauby beauby force-pushed the jsonapi-resource-links branch 3 times, most recently from e33eeac to 4cd5761 Compare October 22, 2015 01:43
@beauby beauby self-assigned this Oct 22, 2015
@beauby beauby added this to the 0.10 milestone Oct 22, 2015
@NullVoxPopuli
Copy link
Contributor

I think this way of doing links is pretty good. And it's consistent with all the block stuff you've been pushing.

@beauby beauby force-pushed the jsonapi-resource-links branch 2 times, most recently from cf246bc to b838cf9 Compare October 26, 2015 01:50
NullVoxPopuli
NullVoxPopuli reviewed Nov 3, 2015
View reviewed changes
lib/active_model/serializer/adapter/json_api.rb
@@ -142,13 +143,24 @@ def resource_identifier_for(serializer)
{ id: id.to_s, type: type }
end

def attributes_for(serializer, fields)
attributes = serializer.attributes.except(:id)
attributes.slice!(*fields) if fields
Copy link
Contributor

Choose a reason for hiding this comment

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

slick

@NullVoxPopuli
Copy link
Contributor

any objections to this? I'd like to get it merged.

NullVoxPopuli added a commit that referenced this pull request Nov 4, 2015
@NullVoxPopuli
Merge pull request #1246 from beauby/jsonapi-resource-links
0200d89 
Add support for resource-level JSON API links.
@NullVoxPopuli NullVoxPopuli merged commit 0200d89 into rails-api:master Nov 4, 2015
bf4
bf4 reviewed Nov 5, 2015
View reviewed changes
lib/active_model/serializer.rb
# Used by JsonApi adapter to build resource links.
def links
self.class._links
end
Copy link
Member

Choose a reason for hiding this comment

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

adding an instance method is still kind of dangerous, no?

@groyoh groyoh mentioned this pull request Dec 3, 2015
Merged
11 tasks
@bf4 bf4 mentioned this pull request Jan 29, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@beauby beauby

Labels
C: Feature C: JSON API V: 0.10.x
Projects
None yet
Milestone
0.10
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@beauby @antondomratchev @NullVoxPopuli @steveklabnik @bf4