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

Image category / properties #3

Open
Afterster opened this issue Nov 2, 2014 · 7 comments
Open

Image category / properties #3

Afterster opened this issue Nov 2, 2014 · 7 comments

Comments

@Afterster
Copy link
Collaborator

In current Images api, images are only identified by an index and associated library item. I believe this will be too restricting on a real use case:

  • clients not always display the same image on a table view and details view. I think image should be categorized and browse by category if requested: cover, fanart, background, banner, ...
  • sometimes it's better to display an image matching better the device screen size/resolution, we should be able to know/query an associated image by width/height.
@sampsyo
Copy link
Member

sampsyo commented Nov 2, 2014

Yes, good point—I was thinking about this too. At the very least there should be a way to distinguish categories of images, and image data like the MIME type and dimensions would be useful too. That would be hard to retrofit onto the current design, so let's change it now.

I can see this going one of three ways:

  1. The images key in each resource is a list of objects describing the images. This is the most straightforward tactic but could be unintentionally expensive: if, for example, the server had to read images from disk to determine their width/height even when the client didn't need it.
  2. More information about images could be found under, for example, /tracks/42/images and not returned by default in /tracks/42. We could provide /tracks/42?include=images to optionally add the image data in a single response.
  3. We could make a separate id-space for images—i.e., /images at the root. Then we'd use "links" to refer from objects to their images. This way, two different objects (even of different kinds) could share the same images. /images/21 would return information about a specific image, and something like /images/21/data would return the actual image file. Image data could again be inlined with ?include=images.

Option 3 is closer to the JSON API spec, but I think I lean toward option 2 for simplicity—since it seems like a pain for servers to supply globally unique ids for all images.

Any thoughts?

@Afterster
Copy link
Collaborator Author

I was thinking about 2. too :). 1. should be avoided and beside 3. complexity, I cannot remember myself real use case on shared image files (each library item generally have his own set of images).

sampsyo added a commit that referenced this issue Nov 5, 2014
@sampsyo
Copy link
Member

sampsyo commented Nov 5, 2014

I just pushed a draft of a more complete spec for images: http://auraspec.readthedocs.org/en/latest/api.html#images

It might be a good idea to add a more complete list of possible "role" values (I just stuck "cover" in there as an example). Do you have a notion of what a somewhat-complete list would consist of, @Afterster?

I'm also not 100% sold on the sub-resource id space for images; an argument could be made that a global image set would actually be easier to implement for both client and server. A server that effectively has image sets per resource could still, for example, use ids like "track-42-1" to internally map to the image 1 for track 42. So I might still come back and change this around. 😇

@sampsyo
Copy link
Member

sampsyo commented Nov 5, 2014

One drawback to what I just proposed: it might be a pain for servers to enumerate all the images in the system at /images, and it certainly seems needless to support pagination and filtering. So if we do go that route, I think it would be a good idea to allow (or even recommend) that the global /images endpoint to be a 404.

@govynnus
Copy link
Member

To continue this discussion after nearly 6 years, I think globally unique image ids are required for inclusion.

Say the client gets a collection of albums /aura/albums?filter[artist]=foo, and that each album has a relationship to it's cover image (which has an id of '1' in the particular album's image collection).

All is well and good if the client looks up metadata about each image individually - it just makes a load of requests in the form /aura/albums/(id)/images/1. However that is hardly a nice or efficient method, hence the need for inclusion.

Now say that the client requests inclusion of images for the same collection: /aura/albums?filter[artist]=foo&include=images. The server is either going to put non-unique type-id pairs in the top level 'included' key (which is useless for the client), or be smart and not include more than 1 resource with a given type-id pair (which is also useless for the client because only 1 image will be included).

This problem is probably the reason behind the JSON API requirement:

Within a given API, each resource object’s type and id pair MUST identify a single, unique resource.

I only noticed this while implementing the current spec for album images; it's not very obvious otherwise. I ended up having to give images globally unique ids anyway for it to work. To do that I just included the 'parent' type and id in the image id, so an image would have an id like 'album-42-cover.jpg' (probably not the nicest way but seems to work).

I do agree with @sampsyo that

it would be a good idea to allow (or even recommend) that the global /images endpoint to be a 404.

@sampsyo
Copy link
Member

sampsyo commented Jul 23, 2020

That's a great analysis, @out-of-range. This is quite convincing, and those "compound IDs" you're generating like album-42-cover.jpg seem like an elegant solution to the problem with very few downsides to me. 😃

@govynnus
Copy link
Member

Cool, I'll see what changes I can come up with for the spec.

govynnus added a commit that referenced this issue Aug 10, 2020
- Changed endpoints for images
- Removed example (other resources don't have examples)
- Added relationship requirements
- Added reason for /aura/images to be a 404
- Made suggestion to use compound ids
- Moved Images section to after Artists (so collections are together)

See #3 for relevant discussion.
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