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 resource-level meta. #1340

Merged
merged 3 commits into from
Feb 9, 2016

Conversation

beauby
Copy link
Contributor

@beauby beauby commented Nov 22, 2015

Untested, but should work. Would love for somebody to provide a few tests, as I am rather busy myself at the moment.
Ref: #1339, #1235.

@kurko
Copy link
Member

kurko commented Nov 25, 2015

@gaganawhad following your offer in

I would be happy to try submitting a PR For Resource-level meta. Does that work? Any reason not to ? Let me know! Thanks. #1235

Do you think you could give a hand here? All help is welcome :)

@beauby
Copy link
Contributor Author

beauby commented Nov 25, 2015

Thanks @kurko! @gaganawhad in case you'd like to give a hand, stuff is happening on my branch.

@beauby beauby force-pushed the resource-level-meta branch 3 times, most recently from 77c14a2 to ee8c0e0 Compare December 10, 2015 22:38
@beauby
Copy link
Contributor Author

beauby commented Dec 10, 2015

Waiting for travis, but otherwise looks good to me. @bf4 @NullVoxPopuli, thoughts?

@post,
serializer: MetaHashPostSerializer,
adapter: :json_api
).serializable_hash
Copy link
Member

Choose a reason for hiding this comment

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

fwiw, there's a test support helper serializable but it wouldn't make things much shorter. Do you think we should test Ruby hashes made from as_json instead of serializable_hash in the future?

@bf4
Copy link
Member

bf4 commented Dec 13, 2015

Just thinking out loud:

Given that Meta is JSON API object that can appear in a number of places (https://github.com/rails-api/active_model_serializers/blob/master/docs/jsonapi/schema.md#json-api-document-schema), perhaps we can re-use a JSON API meta object? I'm not quite sure how it would look.

Where meta is used:

  • topLevel.meta
  • topLevel.jsonapi.meta
  • resource.meta
  • resource.links; link.linkObject.meta
  • resource.relationships.meta
  • resource.relationships.relationshipToOne.linkage.meta
  • resource.relationships.relationshipToMany.linkages; linkage.meta
  • errors; error.meta

I'm a little iffy with defining the meta on the serializer, but I'm not sure where it would otherwise go. JSONAPI-Resources puts the meta in the Resource, so, that's good enough for me. Should we consider defining it at the instance level? _serializer_meta?

@beauby
Copy link
Contributor Author

beauby commented Dec 20, 2015

@bf4 The way I understand the spec, resource-level meta is a legitimate part of a resource's representation (unlike the toplevel meta which contains data related to the document itself, and therefore belongs in the adapter), which in our case is specified by the serializers, so I think it does belong in the serializer.

@@ -233,6 +248,14 @@ def links
self.class._links
end

def meta
Copy link
Member

Choose a reason for hiding this comment

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

Should we prefer _meta even though there shouldn't be attribute method conflicts anymore?

@@ -50,6 +50,8 @@ def self.digest_caller_file(caller_line)
serializer.class_attribute :_type, instance_reader: true
serializer.class_attribute :_links # @api private : links definitions, @see Serializer#link
self._links ||= {}
class_attribute :_meta # @api private : meta definition, @see Serializer#meta
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should probably be serializaer.class_attribute :_meta.

@beauby beauby force-pushed the resource-level-meta branch from ee8c0e0 to 207c85f Compare January 19, 2016 23:49
@beauby
Copy link
Contributor Author

beauby commented Jan 20, 2016

Resource-level meta tests are expected to fail after last commit, since the handling of the option itself (instance_evaluating the block) is now left to the adapter.

@joaomdmoura joaomdmoura mentioned this pull request Jan 20, 2016
11 tasks
@kurko
Copy link
Member

kurko commented Jan 25, 2016

@beauby what does that mean? Expected to fail, which means this PR is WIP?

@beauby
Copy link
Contributor Author

beauby commented Jan 25, 2016

@kurko I meant that those 2 failing tests were testing implementation-specific stuff that does not exist anymore. I'll update them (or get rid of them if they don't make sense at all anymore) tonight if I get a chance, or tomorrow. Thanks for the ping!

@bf4 bf4 merged commit 701404f into rails-api:master Feb 9, 2016
bf4 added a commit that referenced this pull request Feb 9, 2016
- Add changelog entry
- Remove superseded and incorrect tests
- Fix array serialization test
@bf4
Copy link
Member

bf4 commented Feb 9, 2016

Merged. Added followup issues and addressed more urgent ones

@rafael
Copy link
Contributor

rafael commented Apr 13, 2016

Hey guys - question that I'm not sure from the jsonapi spec. Should the meta be rendered when you include a resource in a request? Let's say you have something like this:

GET /users/:id

=> 

    {
      "id": "e877c0cb-83a9-4058-a033-aa67d3683feb",
      "type": "users",
      "attributes": {
        "username": "test"
      },
      "meta": {
        "test_meta": "test
      }
    }

Should the following request include the meta information for the user?

GET /products/:id?include=users

{
  "data": {
    "id": "ce3402d7-23c8-4c27-bc0f-08b47a5edc39",
    "type": "products",
    "attributes": {
      "title": "asdfsaf"
    },
    "relationships": {
      "user": {
        "data": {
          "id": "6dbf0bb8-b9f5-4658-ab5d-f16663b1f3c2",
          "type": "users"
        }
      }
    }
  },
  "included": [
    {
      "id": "6dbf0bb8-b9f5-4658-ab5d-f16663b1f3c2",
      "type": "users",
      "attributes": {
        "username": "test"
      },
      "meta": {
        "test_meta": "test
      }
    }
  ]
}


@groyoh
Copy link
Member

groyoh commented Apr 13, 2016

@rafael Since the meta is used for custom information, I guess that's "up to you". Using AMS, I don't think there is a way of removing it or providing a different meta when the resource is in included though.

Since you asked about the spec itself, you would probably get a more appropriate answer at discuss.jsonapi.org. 😉

@beauby beauby deleted the resource-level-meta branch April 22, 2016 09:08
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.

5 participants