-
Notifications
You must be signed in to change notification settings - Fork 71
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
Conversation
* 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, |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, thanks :)
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... |
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. |
I agree that it is useful for a siren client to filter by property name and/or values. However, I see an 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:
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:
|
My client has
I can also do:
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
or..
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. |
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:
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. |
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. |
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:
|
I like that even less. =) |
I guess I just don't see what's wrong with Since you're only expecting one user image, does it matter that it's arbitrary whether you use 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. |
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 What it does do is say, "there should be only one of this thing", but maybe there's a better way to do that? |
What if we don't restrict 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.,
Depending on the use-case, there is nothing wrong with this. However, what if you have 3 entities with a class of ex:
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. |
Why aren't these links with |
In short, I'm not seeing a solution to the problem that is cleaner than what we already have. |
I believe the 'correct' modelling would be to add a However, given I had less experience with Siren when I started my API, I did a similar thing but used the properties instead:
However, if I was to implement this again now, I would move to the entities with rel names. |
@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 |
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:
Good stuff. |
@kevinswiber +1 Should I update the pull request? |
I'm all for 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:
Now... what symbol could we use to find a link by |
@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.
|
Have you considered json-ld for internal linking syntax? It seems to have some decent support. |
BTW, an id and an internal linking syntax together would solve the sidelining problem. |
If the 'id' is a MUST on the spec, what about backward compatibility for existing clients/servers? |
I don't think that every entity MUST have an |
@apsoto What @dilvie said. No reason to create an ID for every entity all the time. |
probably should specify how to deal with duplicate id's (last one wins?) just to have consistency across client implementations. |
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 Also, it's faster if you only have to iterate to the first |
@dilvie thanks for patiently hanging in there and providing feedback, I totally misunderstood how you were using As a side note, actions have a |
@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? |
@apsoto I know you were concerned about compatibility. Thoughts on the above proposal? |
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. :) |
Okay. New issue: #32. |
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
If actions and sub-entities should be addressable with a fragment using their
Unless it’s decided that some sigil should go in the fragment, these 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 Edit, just to clarify: I’m not saying
|
@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. |
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 I can make a separate pull request for the structural stuff and it can be discussed and merged at a later point? |
@uglymunky Yes, sounds good. Let's focus on adding |
@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. |
@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. |
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:
or
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
andrel
alttributes out into the wrapper so that the sub-entity does not get modified wether it is a sub-entity or the root 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)