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

Exclude links without any data #1668

Merged
merged 1 commit into from
Jun 9, 2016
Merged

Conversation

sigmike
Copy link
Contributor

@sigmike sigmike commented Apr 7, 2016

Purpose

I couldn't find a way to exclude a link conditionally. So I changed the serializer to exclude links without any data. A link without data is certainly wrong, so it's probably safe to exclude it.

Changes

A link without data is serialized as nil, and links serialized as nil are excluded from the output.

It's only implemented in the json-api adapter.

Caveats

It may not be the best solution to this problem nor the best implementation. Maybe conditionals would be better.

Related GitHub issues

None. Maybe the conditional attributes PR: #1403.

@remear
Copy link
Member

remear commented Apr 7, 2016

With which level of links were you experiencing this?

When last i tested this I recall it omitted links with a nil value by default. Are you saying that it does, in fact, serialize a response similar to:

{
  "data": {
    "attributes": {
      "name": "My Cool Post"
    },
    "links": {
       "comments": "/blogs/4134587815891/comments",
       "author": "/authors/32512",
       "top-comment": null
    },
    "type": "blogs"
  }
}

@sigmike
Copy link
Contributor Author

sigmike commented Apr 7, 2016

It serializes to an empty hash like that:

{
  "data": {
    "attributes": {
      "name": "My Cool Post"
    },
    "links": {
       "comments": "/blogs/4134587815891/comments",
       "author": "/authors/32512",
       "top-comment": {}
    },
    "type": "blogs"
  }
}

@groyoh
Copy link
Member

groyoh commented Apr 11, 2016

Related to #1514.

@bf4
Copy link
Member

bf4 commented Apr 13, 2016

@sigmike would you mind sharing how you'd see a conditional link working? I believe you could do this using the block form if it's an attribute or association link since it's eval'd at runtime, not at load.

@bf4
Copy link
Member

bf4 commented Apr 13, 2016

(I'd prefer not to give 'nil' a meaning)

@sigmike
Copy link
Contributor Author

sigmike commented Apr 13, 2016

how you'd see a conditional link working?

Maybe like on the attributes: link(:foo, if: :bar?) { ... }.

But it would be nice to also be able to pass a block to if: (I think it's not possible on attributes right now).

I believe you could do this using the block form if it's an attribute or association link since it's eval'd at runtime, not at load.

I already use the block form to return the link value. That's why just returning nil to disable to link is very convenient.

@bf4
Copy link
Member

bf4 commented Apr 13, 2016

@sigmike

sorry, what I meant was

has_one :user do 
  link user_url if object.user.admin?
end

I don't totally get the use case

@sigmike
Copy link
Contributor Author

sigmike commented Apr 13, 2016

I'm not sure I get it. My use case is when I use link directly as documented here: https://github.com/rails-api/active_model_serializers/blob/master/docs/general/serializers.md#link

I'm using it to provide links to images attached to the resource, but only if such images exist.

@remear
Copy link
Member

remear commented Apr 13, 2016

Here's another example of when this happens:

Given the following in a serializer:

link(:foo) { object.bunnies }

When object.bunnies evaluates to nil, the response is:

{
    "data": {
        "id": "5",
        "type": "companies",
        "attributes": {
            "created_at": "2016-03-29T19:09:32.754Z",
            "name": "Northern Bank of the South",
            "updated_at": "2016-03-29T19:09:32.754Z"
        },
        "links": {
            "self": "http://localhost:9010/companies/5",
            "foo": {}
        }
    }
}

At minimum, foo should probably be null, not {}. Ideally, foo wouldn't be included in links.

@bf4
Copy link
Member

bf4 commented Apr 13, 2016

@remear you've convinced me when you changed 'make a link conditional' to 'handle a nil link'. :)

link(:foo) { object.bunnies }
#=> "links": { "foo": {} }

is a bug

@bf4
Copy link
Member

bf4 commented Apr 13, 2016

but hmm http://jsonapi.org/format/#document-links

Each member of a links object is a "link". A link MUST be represented as either:

  • a string containing the link's URL.
  • an object ("link object") which can contain the following members:
    • href: a string containing the link's URL.
    • meta: a meta object containing non-standard meta-information about the link.

which says nothing about required object attributes

and the linkage docs imply it should be null http://jsonapi.org/format/#document-resource-object-linkage

@bf4
Copy link
Member

bf4 commented Apr 13, 2016

@remear
Copy link
Member

remear commented Apr 13, 2016

I was just looking for that. Because there's no clear answer, I'm not sure how we should make this behave.

Here's the link schema for reference:

"link": {
  "description": "A link **MUST** be represented as either: a string containing the link's URL or a link object.",
  "oneOf": [
    {
      "description": "A string containing the link's URL.",
      "type": "string",
      "format": "uri"
    },
    {
      "type": "object",
      "required": [
        "href"
      ],
      "properties": {
        "href": {
          "description": "A string containing the link's URL.",
          "type": "string",
          "format": "uri"
        },
        "meta": {
          "$ref": "#/definitions/meta"
        }
      }
    }
  ]
}

Reading this leads me to believe link must either be a string in uri format, or an object containing an href in uri format. I don't see null as an option.

@bf4
Copy link
Member

bf4 commented Apr 13, 2016

@remear #1668 (comment) the schema isn't authoritative :)

@remear
Copy link
Member

remear commented Apr 13, 2016

My concern is that it might become authoritative depending on the implementation of #1162.

@NullVoxPopuli
Copy link
Contributor

has there been a conclusion on this PR?

@remear
Copy link
Member

remear commented May 20, 2016

@sigmike Sorry for the delay. I'd like to get this merged in. Would you please rebase this against master, add an entry to the CHANGELOG for this under Features, and squash back down to a single commit.

@bf4 bf4 merged commit 87bffef into rails-api:master Jun 9, 2016
@bf4
Copy link
Member

bf4 commented Jun 9, 2016

Merged on command line and added change log

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.

None yet

5 participants