jpbochi / halberd

JavaScript implementation of HAL format
Other
8 stars 3 forks source link

embedded resources treated like array #13

Open JohanHenin opened 9 years ago

JohanHenin commented 9 years ago

With this sample code:

var representation = new hal.Resource({
    property1: 1,
    property2: 2
},'link');

var representation2 = new hal.Resource({
    property1: 1,
    property2: 2
},'link');

representation.embed("representation2",representation2);
console.log(JSON.stringify(representation.toJSON(),null,4));

Output:

{
    "_links": {
        "self": {
            "href": "link"
        }
    },
    "_embedded": {
        "representation2": [
            {
                "_links": {
                    "self": {
                        "href": "link"
                    }
                },
                "property1": 1,
                "property2": 2
            }
        ]
    },
    "property1": 1,
    "property2": 2
}

representation2 is an array while it should be an object:

jpbochi commented 9 years ago

@JohanHenin according to the specs it should be an array. Each key in the "_embedded" object is a relation. In each relation, there always should be a list of embedded objects/entities.

Refer to http://stateless.co/hal_specification.html and https://phlyrestfully.readthedocs.org/en/latest/halprimer.html#resources.

JohanHenin commented 9 years ago

Thank you for your quick answer. I had already seen these documents, but in the provided examples, embedded resources are actually arrays ("ea:order" in the first link, and "contacts" in the second one). But if you look right after the contacts Embedded resource you can see a "website" embedded resource which is an object. Moreover, here is the hal RFC extract where it's said embedded resources are either "a Resource Object or an array of Resource Objects". https://tools.ietf.org/html/draft-kelly-json-hal-06#section-4.1.2 Sorry for my bad English, I hope I'm understandable.

jpbochi commented 9 years ago

"ea:order" and "contacts" were the relation on those examples. In your case, "representation2" is the equivalent thing. The link you sent confirms exactly that.

It is an object whose property names are link relation types (as defined by [RFC5988]) and values are either a Resource Object or an array of Resource Objects.

The highlights above are mine. The link relation types is the "representation2", as I said, and it contains an array of resource objects, just as define there. It could be a single resource object, but it does not have to be an object.

JohanHenin commented 9 years ago

I agree with you, embedded values can either be an array or an object. And I think it would be useful if I could chose which one I want to.

jpbochi commented 9 years ago

ok, I'm glad we reached the same page. How do you suggest the interface for that should look like?

One option could be to send some extra argument to the toJSON function. Like this: entity.toJSON({ "single-embed-objects-when-possible": true })

If you can send a pull request, I'd gladly review, merged, and publish it.

JohanHenin commented 9 years ago

It can be a good solution. I had another idea: what about using the type of the provided embedded resource to define its representation ? I would be really happy to contribute but I have to confess, I ve jus started learning javascript a few weeks ago. I m not sure to have the required skills. If you don't mind spending time on checking what I would have done I 'd be really glad to do it.

jpbochi commented 9 years ago

I'm not sure I understand what you mean by "using the type of the provided embedded resource to define its representation". Are you referring to link relation type? Something like add a knowledge that an entity can have only one "mother" (or "father")?

As for contributing, please go ahead. I'd happy to review the code changes. It's part of being a nice open source citizen.

JohanHenin commented 9 years ago

I don't know if it's possible, but here is how I see this: Since the relation name is unique and provided in first parameter, if second parameter is an array or there are several additional parameters, the representation of the embedded resource should be an array. Else the second parameter is a single objet and should be represented as such.

jpbochi commented 9 years ago

The thing is that the embed method can be called multiple times, even with the same link relation. In this case, it append the resource from the second call into the array.

You could implement it in a way that, on the first call to embed with a given link relation, if there's no embedded resource with that relation yet, add one as an object instead of an array.

One alternative that might be easier to implement is to make such verification on the toJSON method.

One last thing to consider is that, regardless of how this gets implemented, we should be aware of breaking changes to people already using this library. The best is to avoid it (by making arrays the default). If a change will prefer objects, then there must be an easy way to force arrays, so that codebases that assume arrays could still work without much hassle.