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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Selectively link or include a relation. #45

Closed
archseer opened this issue Dec 7, 2015 · 11 comments
Closed

Selectively link or include a relation. #45

archseer opened this issue Dec 7, 2015 · 11 comments

Comments

@archseer
Copy link
Contributor

archseer commented Dec 7, 2015

Hi, I've been having fun with this library in combination with Ember. 馃嵒 for having it seamlessly "just work" out of the box!

I'd like to be able to selectively toggle between including or just linking to a relation. For example, in my app, I have an Album, that belongs to an Artist, and has many Tracks. I'd like to just link to the Track relation on GET /albums, and I'd like to sideload it on GET /albums/:id.

I've gotten close by using this:

defmodule Colibri.AlbumView do
  use Colibri.Web, :view

  location "/albums/:id"
  attributes [:title]

  has_one :artist,
    serializer: Colibri.ArtistView,
    include: true

  has_many :tracks,
    serializer: Colibri.TrackView,
    link: "/albums/:id/tracks"
end

and then modifying the render call in the index route:

render(conn, :index, albums: albums, opts: [include: "tracks"])

However this is not quite what I want, because it'll both link to the relationship and include the track ids in the /album/:id show action response (which makes Ember fetch these one-by-one, instead of just using the relation link).

I also tried dropping the serializer option, and manually doing include in the show action render call, however that breaks the code, since there's no serializer available.

I propose the following -- include: false (or maybe embed: false) would disable embedding:

# serializer.ex
  has_many :tracks,
    link: "/albums/:id/tracks",
    serializer: Colibri.TrackView,
    include: false

And then, we'd be able to explicitly opt-in on the render action:

# controller.ex
render(conn, :index, albums: albums, opts: [include: "tracks"])
# this could potentially include the default relations + tracks
render(conn, :index, albums: albums, opts: [include: "default,tracks"])

I've looked into the internals to try and write a patch for this, but it's my first week of using Elixir, so I'm not quite there yet...

@alanpeabody
Copy link
Contributor

Thanks for the feedback and issue report @archseer.

Include should default to false, I would think what you have defined should work.

Then in your show action:

render(conn, :index, albums: albums, opts: [include: "artist,tracks"])

that SHOULD result in a document something like (please excuse my poorly formatted json):

{
  data: {
    id: 1,
    type: 'album',
    attributes: { /* ommited for breviety */ },
    relationships: {
      tracks: {
        links: { related: '/albums/1/tracks' },
        data: [{id: 1, type: 'track'}, {id: 2, type: 'track'}] //resource identifier objects - required for including.
      }
    }
  },
  included: [
    # full track info for each track.
  ]
} 

The included "resource identifier objects" in the album's track relationship data are required for includes to work and should not cause ember to retrieve each track individually. You may need to let ember know to look for the included data with {async: false}.

RE default: Currently the we have includes implemented per my understanding of the spec, I am not sure if a default is acceptable to this section or the spec:

http://jsonapi.org/format/#fetching-includes

"If an endpoint supports the include parameter and a client supplies it, the server MUST NOT include unrequested resource objects in the included section of the compound document."

@archseer
Copy link
Contributor Author

archseer commented Dec 7, 2015

Specifying {async: false} results in "Error: Assertion Failed: You looked up the 'tracks' relationship on a 'album' with id 3 but some of the associated records were not loaded. Either make sure they are all loaded together with the parent record, or specify that the relationship is async (DS.hasMany({ async: true })".

With async, this.get('album.tracks').then(function(tracks) { results in per-id queries (one per each id/type pair inside data).

From what I could tell, Ember will only use the relationship link if there's no data included. That said, it'd be nice in general to be able to selectively specify whether to just include the related link, or whether to load the actual data on a per-route basis.

Another trick I've tried is overriding the getter:

    def tracks(model, conn) do
      case model.tracks do
        %Ecto.Association.NotLoaded{} ->
          nil
        other -> other
      end
    end

That won't work either, because per-spec, an explicit nil or [] data relation signifies an empty relation (and Ember ended up not doing any queries in that scenario).

Regarding default, yeah, my bad on that.

@archseer
Copy link
Contributor Author

archseer commented Dec 7, 2015

Regarding include: false, what I meant was to use false as a way to signify no data whatsoever should be present (link-only relation). From what I can tell, the framework internally defaults it to nil, which will not sideload the data, but will include "resource identifiers" this scenario.

@alanpeabody
Copy link
Contributor

@archseer I do think the behavior of ja_serializer matches the spec and is behaving appropriately[1].

What you are trying to accomplish is, unfortunately, outside the standard "ember way".

Ember allows relationships to be either async or included at a global level not at the request level. It doesn't ship with support for includes (or sparse field-sets) yet, which would help do what you wish to accomplish and would be great for building ember apps in general. I would guess with some digging at the DS.Adapter level you could probable hack something in.

Frankly, if I were building the ember app I would probably just be specifying the link relationship and accepting the extra request to retrieve all the tracks if the /album/:id is hit directly as a small cost.

[1] My belief is that the JSON being returned is as I outlined in my previous comment.

@archseer
Copy link
Contributor Author

archseer commented Dec 7, 2015

Indeed, both of the configurations are valid JSONAPI, but what I'd like to do is be able to switch between these two configurations when I render:

has_many :tracks, include: true, serializer: Colibri.TrackView, link: "/albums/:id/tracks"

and

has_many :tracks, link: "/albums/:id/tracks"

The former setup on the index action GET /albums and the latter on GET /albums/:id. Ember will automatically act correctly for both of these responses, without any sort of special configuration (assuming async: true). If the response includes relationship data, it loads up the track models, if it's a link, it will async request to /albums/:id/tracks when needed.

I agree that the performance hit is negligible though, I guess I could also work around it by creating two different phoenix views, one for each of the two configurations.

Anyway, sorry for being a nuisance! I guess we can now mark this as resolved.

@alanpeabody
Copy link
Contributor

@archseer you are not being a nuisance, I think I am having trouble understanding exactly what JSON you are expecting to be produced at different times.

That said, I think your last comment started to clarify it for me.

You don't want the "resource identifiers" (/data/1/relationships/data/[{:id,:type}]) when include is false (via param nor dsl) if a link is present.

Let me think about that a bit. I am also considering the best to make relationships something that could be completely overridden and defined w/o use of the DSL. This would allow complete customization and make the DSL optional.

@archseer
Copy link
Contributor Author

archseer commented Dec 9, 2015

Yeah sorry, probably should have included a sample of both responses...

So ideally we should be able to configure the serializer's relationship with a set of general settings (sideload the relationship, embedding all the data), but I should be able to override it for certain routes/render calls (don't load or embed the relationship at all, instead just provide the relationship link).

@ianwalter
Copy link

@archseer I'm solving this problem by creating one serializer for most cases lets call it BookSerializer. This serializer is not including relationships. I then have another serializer for a specific route that is including relationships, lets call it CompleteBookSerializer. I then explicitly set the type in CompleteBookSerializer:

def type, do: "book"

I think your proposal is better but just wanted to throw this out there in case others are wondering how to accomplish this.

@alanpeabody
Copy link
Contributor

FWIW, This is still on my radar. I still think removing the link must be some sort of option, as I think the "spirit" of json-api would be to keep the url and include the resource. I will try make a concerted effort focusing on the relationship DSL sometime in the next month.

@archseer
Copy link
Contributor Author

Yeah, so what I'm doing is something similar, I have one serializer specifically for embedding:
https://github.com/archSeer/colibri-server/blob/master/web/views/embedded_artist_view.ex
https://github.com/archSeer/colibri-server/blob/master/web/views/album_view.ex

^ This is used for /album/:id, so that the data is inlined and won't include any of the artist's relations (artist.albums for example).

Then I have a separate serializer for /artists/:id

https://github.com/archSeer/colibri-server/blob/master/web/views/artist_view.ex

^ This one currently provides an album link, but I'm probably going to make an embedded representation of the album model and inline it.

It's a bit of extra work, but it works just fine for me for now

@alanpeabody
Copy link
Contributor

#121 and release 0.10.0 adds some support for this, but it is still really confusing!

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

No branches or pull requests

3 participants