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

Relax entity requirements #45

Closed
wants to merge 3 commits into from

Conversation

ericelliott
Copy link
Contributor

The purpose of this PR is to relax sub-entity requirements.

  • Allows more efficient encoding of sub-entities.
  • Does not force the representation to tightly couple child entities to parent entities via required rel.

If this change is not implemented in Siren, I will fork Siren.

Rationale

Eric Elliott and others added 3 commits February 16, 2015 11:51
* Allows more efficient encoding
* Does not force the representation to tightly couple child entities to parent entities via required `rel`.
@ericelliott
Copy link
Contributor Author

An OPTIONAL entityAttributes collection could replace the rel requirement in embedded sub-entity representations by delegating the requirement to the parent collection, which IMO is a better place to store parent-child entity relationships, anyway.

@rmorrise
Copy link

Can entities collection be made an associative array?

This prevents the "rel" attribute from having to be included in the entity's own JSON representation and moves the responsibility of specifying the relationship to the parent object. It also makes the data easier for JavaScript clients to work with IMHO.

Ex:

"entities": {
    "order-items": { 
      "class": [ "items", "collection" ], 
      "href": "http://api.x.io/orders/42/items"
    },
    "customer": {
      "class": [ "info", "customer" ],
      "properties": { 
        "customerId": "pj123",
        "name": "Peter Joseph"
    },
    "inline-order-items": [ 
        {
            "class": [ "item"], 
            "properties": { 
                "itemId": "a123",
                "name": "Something"
            }
        },
        {
            "class": [ "item"], 
            "properties": { 
                "itemId": "a456",
                "name": "Something Else"
            }
        }
    ]
  }

@ericelliott
Copy link
Contributor Author

Two pretty serious issues:

  1. There's no such thing as an associative array in JSON. That's just called an "object". Your example is not valid JSON.
  2. I don't see how the rel gets associated with the item. Is the key the rel? So in cases where multiple items share the same rel, they have to be represented as a collection, and then each individual item gets assigned the plural-form rel collection name? So, using your example, item a123 has the rel inline-order-items? Would the client need to depluralize? Would you instead not use plural-form rel values? Would that be as readable?

@rmorrise
Copy link

Thanks for pointing out my syntax error. I edited my above comment and fixed the example JSON.

Yes, rel is the key. My thinking about rel is that the client will typically use it in one of two ways (in the current Siren model):
A) Get the entity with rel == 'customer' and use its data
B) Get all entities with rel == 'inline-order-item' and use their data in some aggregate or list operation.

In case A, what I really wanted to write was:

var someValue = myObj.entities.customer.properties.myProperty

...but what I end up doing instead is something like:

var someValue
var myCustomers = myObj.entities.filter(function(el) { ... })
if (myCustomers && myCustomers.size > 0) {
    someValue = myCustomers[0].properties.myProperty
}

In case B, I have to write a loop to collect up the values, then (for example) display them as data items in my HTML. Using the proposed style, I can write:

var myItems = myObj.entities['inline-order-items']
for (var i = 0; i < myItems.length; i++) {
 ...
}

Using the original syntax, I have to do the same thing but I have to write additional code to filter the list, first:

var myItems = myObj.entities.filter(function(el) { ... })
for (var i = 0; i < myItems.length; i++) {
 ...
}

In my thinking, if there are multiple child objects with the same rel, the client has to be smart enough to treat them like a collection anyway. Why not make this association explicit?

If the multiplicity of the relationship is unknown, the client can simply test whether the property is an array value or an object value.

@ericelliott
Copy link
Contributor Author

A couple years ago I wrote a client that lets you query siren responses much like you'd use jQuery.

for example, get all entities with class foo:

var foos = siren('.foo');

It's pretty trivial to do the same thing for rels, etc...

I wanted to make it open-source, but my employer at the time did not have a great open source policy.

Still, it's not hard to implement, and I've heard of other people doing the same thing with Siren.

@ericelliott
Copy link
Contributor Author

@kevinswiber may have a project available that does something similar. I am pretty sure I remember discussing this type of siren client with him a while back.

@apsoto
Copy link

apsoto commented Feb 18, 2015

In general I like the idea of moving the rel as a concern of the parent.

That said, a couple of comments on the format @rmorrise provided:

  • The entities property is currently an array, so it's a breaking change to convert it to an object. That would require a new media type (application/vnd.siren.v2+json)
  • In addition, current spec requires the rels to be an array. Using the example above means there is only one rel per child entity. I don't think I'd miss that requirement very much, but I only have the perspective of my specific API's problem domain.
  • I like treating everything as a collection instead of having the possibility of it being an object or an array. So I'd prefer that any child entities are always wrapped in an array.

@gxxcastillo
Copy link

In case A, what I really wanted to write was......but what I end up doing instead is something like...

see: #17 (comment)

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.

4 participants