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 name to embedded entities #31

Closed
wants to merge 8 commits into from

Conversation

gxxcastillo
Copy link

Following up on this thread

Entities need a name, only then can you reference a particular entity. So long as entities do not have names, the only option we have for referencing a particular entity is by filtering. While filtering does produce a result, it will always return a set of results (either an empty set, a set of 1 or a set of greater than 1) when all we want is one.

On my Siren client, I've currently implemented a couple of workarounds for this:

  1. add a non-standard "name" property to each entity
    or
  2. add a rel of "name:<entity_name>"

Ultimately, however, these are just workarounds for something that I believe belongs as part of the Siren spec.

I had considered getting rid of entities all together (and just having everything be a property) , however, I see how they are critical in order for clients to parse.

I had also considered just adding the name property but then felt awkward about veering too far away from the "standard entity".

So, while more verbose, it seemed best to wrap each sub-entity and add an optional "name" attribute there. Similarly pull the href and rel alttributes out into the wrapper so that the sub-entity does not get modified wether it is a sub-entity or the root entity:

entities: [
    {name: 'items', href: 'api.io/order/1234/items', rel: 'some-rel', entity: {}}
    {name: 'customer', rel: 'some-other-rel', entity: {}}
]

Sample user-case
I want to be able to do user->purchases or user->address. While I believe that the semantics are something that get set up by a Siren client, a Siren client can only achieve this if the underlying Siren spec provides for an entity to have a name.

(Apologies for the funky commit history, clearly I did some git funkiness)

uglymunky added 6 commits April 8, 2013 21:10
* Add an optional name to each sub-entity
* Sub-entity representations retain all the same characteristics of a standard entity.  eg, no added rel or href attribute as those now go on a wrapper.
"class": [ "items", "collection" ],
"rel": [ "http://x.io/rels/order-items" ],
{
"name": items,
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't items be in quotes?

Copy link
Author

Choose a reason for hiding this comment

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

yep, thanks :)

@ericelliott
Copy link
Contributor

I already frequently filter on an "id" property. In my server implementation, I have a filter function that can filter on any property of any entity or sub-entity. It seems to me that rather than simplifying things, making this "name" option part of the spec would impose a strange implementation restriction that is probably not needed.

That said, HTML elements can have IDs...

@ericelliott
Copy link
Contributor

I can see a use case where a name property would be a useful way to look up sidelined entities, so multiple entities can internally link to a single entity to avoid inlining the same data repeatedly.

@gxxcastillo
Copy link
Author

I agree that it is useful for a siren client to filter by property name and/or values. However, I see an id property as totally different from what I am proposing by having a name attribute on a sub-entity.

If I have a user and that user has an image, it is useful to know which image that user has and for that we use the id (e.g., image.id = 123).

However, how do we know which sub-entity is the image?

(filtering by property values can be tricky, even when using ids. What if the user has a contact record sub-entity and it has the same id as the image? e.g., contact_record.id = 123)

The only ways I can currently do this is by filtering. I can filter by class, rel, self-link, or any other sub-entity top level attribute.

An example of how a client might do this today would be:

user.filterEntities({rel: 'user-image'})

However, filtering inherently returns a set of results and therein lies the problem; I don't want a set. Instead I want my client to be able to do:

user.image

@ericelliott
Copy link
Contributor

My client has .links and .entities objects:

var imageLink = user.entities.getBy('class', 'user-image')[0];

I can also do:

var user.links.getBy('rel', 'user-image')[0];

I know that it's a tiny bit awkward that it returns an array, but you could easily specify in your API that only one is allowed, and add a .first() method that just returns the first match:

var imageLink = user.entities.first('class', 'user-image');

or..

var user.links.first('rel', 'user-image');

Your suggestion does NOT restrict the data structure from returning more than one of the same name. You'd have to make entities an object (would you do the same for links?) and force people to come up with names for every entity / link. I don't see that as a better solution. I guess I'm just not feeling the pain.

@gxxcastillo
Copy link
Author

Your suggestion does NOT restrict the data structure from returning more than one of the same name.

You are correct. It would still be on the person implementing to make sure there is not more than one sub-entity of the same name.

I see adding a "name" as a way to get around the fact that entities are separate from properties. Ideally, there would be no entities array and instead you would do something like:

// Note "customer" and "items" are sub-entities
{
 "properties": {
      "orderNumber": 42,
      "itemCount": 3,
      "status": "pending"
      "items": {
            ... 
      },
      "customer": {
           ...
      }
  },
}

But then, a client is left without having a reliable way to differentiate between what is a "sub-entity" and what is a property that just happens to be an object.

I also agree that there are workarounds for the lack of having a name. I am suggesting that sub-entities are an important feature that Siren offers and they should be treated as such - just like properties and we should not have to rely on workarounds for accessing them.

@ericelliott
Copy link
Contributor

Since the data structure doesn't limit the number of properties with the same name, you'd still need to implement some sort of client-side mechanism like the one I demonstrate above. I'm still not sure I see the point.

@gxxcastillo
Copy link
Author

The difference would be that as a client, you would always expect only one. Thus, getting only the first result would more of a check in case the endpoint was incorrectly constructed. Getting the first result when filtering by any properties, as you demonstrated, would be arbitrary and no different than getting the last result.

Still, I hear you in that my implementation of this does not provide any way of enforcing this restriction. Would making the entities array into an object be preferred?

example:

{
 "properties": {
      "orderNumber": 42,
      "itemCount": 3,
      "status": "pending"
  }
  "entities": {
      "items": {
            ... 
      },
      "customer": {
           ...
      }
  },
}

@ericelliott
Copy link
Contributor

I like that even less. =)

@ericelliott
Copy link
Contributor

I guess I just don't see what's wrong with var imageLink = user.entities.first('class', 'user-image');

Since you're only expecting one user image, does it matter that it's arbitrary whether you use .first(), .last(), or whatever? Should the client even care if it gets sent more than one, or should it just use the first one and be happy? If the service is working properly, it will only ever send one, and it doesn't matter to the client. Servers can implement a very simple unit test to ensure that whatever constraints are set on the service are respected.

Basically, I don't think this is the job of the media type. You're getting into application scope here, and the most flexible solution from the media type perspective is to allow many-to-many relations using arrays and allow clients to select by whatever attribute they want, and allow the application to impose restrictions on itself.

@ericelliott
Copy link
Contributor

Put another way, if you create another attribute called "name" it still isn't guaranteed unique, and you still need the same kind of client-side mechanism to select it, so you're in the same boat implementation-wise as you are with using rel or class, instead. It doesn't simplify selection from a code perspective.

What it does do is say, "there should be only one of this thing", but maybe there's a better way to do that?

@gxxcastillo
Copy link
Author

What if we don't restrict entities to be an array and allow it to optionally be an object? That's the only way I can think of enforcing this constraint.

The use-case I can think of for keeping it as an array (and thus, not having a name) is for collections. This is because entities in a collection just need indexes and not names (e.g., user.images[0], user.images[1], user.images[2], ...). In this use-case, we agree, the client doesn't care, so long as it gets an image.

I guess I just don't see what's wrong with var imageLink = user.entities.first('class', 'user-image');

Depending on the use-case, there is nothing wrong with this. However, what if you have 3 entities with a class of user-image: "profile-image", "cover-image", and "public-image"?

ex:

entities:  [
     {class: ['user-image'], href: 'api.io/image/123'}   // This is the "profile-image"
     {class: ['user-image'], href: 'api.io/image/765'}   // This is the "cover-image"
     {class: ['user-image'], href: 'api.io/image/555'}   // This is the "public-image"
]

If you are a client, how do you get the "cover-image"?

I am proposing that you can not, unless you can ensure that only one of these user-images has a class or rel of "cover-image" and in effect what you would be doing in that case is giving it a name.

@ericelliott
Copy link
Contributor

entities:  [
     {class: ['user-image'], href: 'api.io/image/123'}   // This is the "profile-image"
     {class: ['user-image'], href: 'api.io/image/765'}   // This is the "cover-image"
     {class: ['user-image'], href: 'api.io/image/555'}   // This is the "public-image"
]

Why aren't these links with rel: ['profile-image'], etc...? I'm still not seeing a use case where name gives you something that rel and class can't... and making entities switch between array and object makes even less sense that making it one or the other... it imposes the same semantics on all the entities.. what if later on, one of them needs that uniqueness constraint, but none of the other entities have names?

@ericelliott
Copy link
Contributor

In short, I'm not seeing a solution to the problem that is cleaner than what we already have.

@apsoto
Copy link

apsoto commented Feb 21, 2014

I believe the 'correct' modelling would be to add a rel: ['profile-image'], etc.

However, given I had less experience with Siren when I started my API, I did a similar thing but used the properties instead:

  "properties": {
    "id": 18144818,
    "pubDate": "2013-09-27T15:21:05Z",
    "thumbnails": {
      "default": {
        "url": "http://static...-a-glass-window--jpg"
      },
      "small": {
        "url": "http://static...-a-glass-window--jpg"
      },
      "large": {
        "url": "http://static...-a-glass-window--jpg"
      }

    },

However, if I was to implement this again now, I would move to the entities with rel names.

@ericelliott
Copy link
Contributor

@apsoto You're right. Fixed.

BTW, if you're just trying to specify a URL for something, I'd put those in the links collection and use rel.

@kevinswiber
Copy link
Owner

I'm all for this issue being resolved. A big use case I've heard previously and am currently running into is the ability of responses to communicate a local reference to the client. In the browsing world, we use URI fragments and HTML element IDs.

The PR looks pretty good. After much thought, I think I'd prefer to use ID. HTML5 is moving away from the anchor name approach and is recommending using IDs instead.

Same as the PR, just changing name to id:

{
  "entities": [
    {
      "id": "items",
      "rel": [ "http://x.io/rels/order-items" ],
      "href": "http://api.x.io/orders/42/items"
    },
    {
      "id": "customer",
      "rel": [ "http://x.io/rels/customer" ],
      "entity": {
        "class": [ "info", "customer" ],
        "properties": {
          "customerId": "pj123",
          "name": "Peter Joseph"
        },
        "links": [
          { "rel": [ "self" ], "href": "http://api.x.io/customers/pj123" }
        ]
      }
    }
  ]
}

This looks fine to me. http://api.x.io/orders/42#customer would refer to the entity with id="customer" in the Siren response.

Enforcing uniqueness can be done by a specification MUST that API designers will have to take into consideration. The language should specify this is a Siren-level document ID, not a URL template parameter, persistence-level identifier or anything like that.

Here's what our good friend HTML has to say:

id = ID
A unique identifier for the element.
There must not be multiple elements in a document that have the same id value.
Value: Any string, with the following restrictions:

  • must be at least one character long
  • must not contain any space characters

Good stuff.

@gxxcastillo
Copy link
Author

@kevinswiber +1

Should I update the pull request?

@ericelliott
Copy link
Contributor

I'm all for id because the name suggests that it must be unique. I suggested it in my first response.

I also like the idea of being able to add something in the client like CSS selectors... speaking of which, I'll probably update my client to do this:

var collection = siren(response); // convert JSON to siren object
var entity = collection.find('#someId'); // extract entity by id
var someLink = collection.find('.someClass'); // extract by class

Now... what symbol could we use to find a link by rel?

@kevinswiber
Copy link
Owner

@dilvie Funny you ask that. I went through this exercise about a year ago. Looking at my notes, here is what I was going to implement.

.class
$subentity-rel
!action
%link-rel

@ericelliott
Copy link
Contributor

Have you considered json-ld for internal linking syntax? It seems to have some decent support.

@ericelliott
Copy link
Contributor

BTW, an id and an internal linking syntax together would solve the sidelining problem.

@apsoto
Copy link

apsoto commented Feb 22, 2014

If the 'id' is a MUST on the spec, what about backward compatibility for existing clients/servers?

@ericelliott
Copy link
Contributor

I don't think that every entity MUST have an id, but I do think that an id MUST be unique. I don't think that would break backward compatibility. ;)

@kevinswiber
Copy link
Owner

@apsoto What @dilvie said. No reason to create an ID for every entity all the time.

@apsoto
Copy link

apsoto commented Feb 22, 2014

probably should specify how to deal with duplicate id's (last one wins?) just to have consistency across client implementations.

@ericelliott
Copy link
Contributor

I would say first wins, because there should only ever be one. It doesn't make sense to grant override powers to late arrivals. It might accidentally encourage id abuse. The first id should stubbornly hold on to its dominion.

Also, it's faster if you only have to iterate to the first id match in the collection.

@gxxcastillo
Copy link
Author

@dilvie thanks for patiently hanging in there and providing feedback, I totally misunderstood how you were using id :)

As a side note, actions have a name. Is it expected to be unique (the spec doesn't say it is) but if it is expected to be unique, would it make sense to change that to id as well?

@kevinswiber
Copy link
Owner

@uglymunky This is a valid point. We should change action.name to action.id. Since this breaks compatibility, it will require a version change. We're currently on 0.6.0. I just added semantic versioning to the spec. Honestly, I never anticipated as much Siren usage as there is prior to a 1.0 release. This is a good thing, but we do have to be kind to existing implementations.

Let's add entity.id in this PR and submit another one for changing action.name to action.id. I can create a 0.6.1 release and add the action switch to a 0.7.0 roadmap.

Sound good?

@kevinswiber
Copy link
Owner

@apsoto I know you were concerned about compatibility. Thoughts on the above proposal?

@kevinswiber
Copy link
Owner

Taking a step back... Do we really need to change action.name to action.id? It would be more consistent, but do they actually represent something different? Hmm. I think I'll just write a new issue where we can discuss. :)

@kevinswiber
Copy link
Owner

Okay. New issue: #32.

@apsoto
Copy link

apsoto commented Feb 23, 2014

Now that there is version, this sounds better. Should move to a separate thread, but the media type doesn't have examples distinguishing versions, so needs a bit more work.

@@ -127,7 +130,11 @@ The URI of the linked sub-entity. Required.

####Embedded Representation

Embedded sub-entity representations retain all the characteristics of a standard entity, but MUST also contain a `rel` attribute describing the relationship of the sub-entity to its parent.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a good reason to remove the rel requirement for sub-entities?

Copy link
Author

Choose a reason for hiding this comment

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

rel is still required for both "linked" sub entities and "embedded" sub entities. That section addressed the difference between a "standard" entity and an "embedded" sub-entity, however, that difference no longer exists so I thought it would make sense to remove it.

There is still the section that defines what a rel is and it states that it is required for sub entities. (it just doesn't show up in the diff)

Is there a better way to communicate the rel requirement?

@mgomezch
Copy link

If actions and sub-entities should be addressable with a fragment using their id, the same should be possible for properties (and perhaps even their nested elements using some JSON addressing syntax) and links.

{ "links":
  [ { "rel": ["self"], "href": "https://api.example.com/foo" }
  , { "rel": ["bar"], "href": "https://api.example.com/bar", "id": "l_bar" }
  ]
, "actions":
  [ { "id": "a_baz", }
  ]
, "properties":
  { "quux": 42
  }
}
  • https://api.example.com/foo#l_bar should identify the link to https://api.example.com/bar.
  • https://api.example.com/foo#a_baz should identify the action.
  • Possibly, something like https://api.example.com/foo#quux should identify the property.

Unless it’s decided that some sigil should go in the fragment, these ids (and possibly property names) should all share one namespace. An alternative is to use something line #.quux for a property’s fragment, and possibly something else for the rest.

This suggestion is for consistency with the general idea of fragments in Siren, but I’m skeptical of the desirability of the idea itself for a REST API. If other representations exist for the state of the resource at https://api.example.com/foo, should they also have some notion of the specified fragment, even if it doesn’t make sense in the context of the other representation’s format? Suppose, for example, that I also provide HAL representations, which don’t support anything like actions — what does https://api.example.com/foo#a_baz even mean for a client that somehow got that URI but only speaks application/hal+json? HAL is just an arbitrary example, of course.

Edit, just to clarify: I’m not saying ids are a bad idea — I’m just not sure it’s a good idea to think of them as fragment identifiers in the specific context of a REST API. For reference, from RFC 3986:

Individual media types may define their own restrictions on or
structures within the fragment identifier syntax for specifying
different types of subsets, views, or external references that are
identifiable as secondary resources by that media type. If the
primary resource has multiple representations, as is often the case
for resources whose representation is selected based on attributes of
the retrieval request (a.k.a., content negotiation), then whatever is
identified by the fragment should be consistent across all of those
representations. Each representation should either define the
fragment so that it corresponds to the same secondary resource,
regardless of how it is represented, or should leave the fragment
undefined (i.e., not found).

@ericelliott
Copy link
Contributor

@mgomezch I agree it should apply to the others as well. Also agreed re: shared namespace. That would be consistent with our experience with HTML.

I think that hash fragments in api's is both useful and necessary.

@gxxcastillo
Copy link
Author

Let's add entity.id in this PR and submit another one for changing action.name to action.id. I can create a 0.6.1 release and add the action switch to a 0.7.0 roadmap.

Sounds good. Note that the structure proposed in this PR is also a pretty significant change. If you think it makes sense, I can close this pull request and create a new one that only adds the entity.id field and does not propose any structural change to the sub-entities.

I can make a separate pull request for the structural stuff and it can be discussed and merged at a later point?

@kevinswiber
Copy link
Owner

@uglymunky Yes, sounds good. Let's focus on adding id and discuss the other changes separately. Thanks for your work on this.

@kevinswiber
Copy link
Owner

@mgomezch Not sure how I feel about fragments linking to properties. I'm having second thoughts on actions, as well.

There's a difference here in my mind. One is a pointer to an entity in context (e.g., the representation of an album within a list of albums). The other is more of a JSON query of the document.

@gxxcastillo gxxcastillo mentioned this pull request Feb 25, 2014
@ericelliott
Copy link
Contributor

@kevinswiber I don't think it should apply to properties. I don't think we should impose any semantic meaning on any property -- I think those should be an application concern, not a media type concern... though I'd be fine with a selector token that would let you select the value of a property by property name.

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

Successfully merging this pull request may close these issues.

5 participants