api-platform / core

The server component of API Platform: hypermedia and GraphQL APIs in minutes
https://api-platform.com
MIT License
2.45k stars 878 forks source link

HAL links of embedded resources not consistent with standard representation #1291

Open bendavies opened 7 years ago

bendavies commented 7 years ago

Considering using the demo app: https://github.com/api-platform/demo

Modifying the Book Entity so Reviews are embedded. A review looks like:

{
    "_links": {
        "book": {
            "href": "/books/3"
        },
        "self": {
            "href": "/reviews/275"
        }
    },
    "author": "Herbert Reinger I",
    "body": "Voluptas hic ipsum cumque aliquid. Vero iusto sit atque hic non voluptate dignissimos velit. In reprehenderit accusamus cum porro. Doloremque eligendi architecto quia omnis eaque vel officiis.",
    "id": 275,
    "publicationDate": "1970-07-11T14:10:52+00:00",
    "rating": 5
}

A book (with embedded review) looks like this:

{
    "_embedded": {
        "reviews": [
            {
                "_links": {
                    "self": {
                        "href": "/reviews/275"
                    }
                },
                "author": "Herbert Reinger I",
                "body": "Voluptas hic ipsum cumque aliquid. Vero iusto sit atque hic non voluptate dignissimos velit. In reprehenderit accusamus cum porro. Doloremque eligendi architecto quia omnis eaque vel officiis.",
                "id": 275,
                "publicationDate": "1970-07-11T14:10:52+00:00",
                "rating": 5
            }
        ]
    },
    "_links": {
        "reviews": [
            {
                "href": "/reviews/275"
            }
        ],
        "self": {
            "href": "/books/3"
        }
    },
    "author": "Prof. Joelle Doyle",
    "description": "Cupiditate quae ut sit illo. Accusamus ex voluptatem et eveniet nemo voluptatibus necessitatibus. Veniam odit ducimus consequuntur voluptatem id quod pariatur distinctio.",
    "id": 3,
    "isbn": "9783899289190",
    "publicationDate": "1996-01-02T00:00:00+00:00",
    "title": "Excepturi quo dolor et officiis minima et consequatur earum."
}

Note that the embedded Review does not have a book in its _links. How can the embedded representation be made to match the non embedded? I.e they both should have book link. the representation must be consistent.

Thanks

Simperfit commented 7 years ago

What is the output that you would like to see @bendavies ?

bendavies commented 7 years ago

@Simperfit

I.e they both should have book link. the representation must be consistent.

{
    "_embedded": {
        "reviews": [
            {
                "_links": {
                    "book": {
                        "href": "/books/3"
                    },
                    "self": {
                        "href": "/reviews/275"
                    }
                },
                "author": "Herbert Reinger I",
                "body": "Voluptas hic ipsum cumque aliquid. Vero iusto sit atque hic non voluptate dignissimos velit. In reprehenderit accusamus cum porro. Doloremque eligendi architecto quia omnis eaque vel officiis.",
                "id": 275,
                "publicationDate": "1970-07-11T14:10:52+00:00",
                "rating": 5
            }
        ]
    },
    "_links": {
        "reviews": [
            {
                "href": "/reviews/275"
            }
        ],
        "self": {
            "href": "/books/3"
        }
    },
    "author": "Prof. Joelle Doyle",
    "description": "Cupiditate quae ut sit illo. Accusamus ex voluptatem et eveniet nemo voluptatibus necessitatibus. Veniam odit ducimus consequuntur voluptatem id quod pariatur distinctio.",
    "id": 3,
    "isbn": "9783899289190",
    "publicationDate": "1996-01-02T00:00:00+00:00",
    "title": "Excepturi quo dolor et officiis minima et consequatur earum."
}
bendavies commented 7 years ago

@soyuka any comment? Can you see my point?

soyuka commented 7 years ago

@bendavies I'm really not familiar with HAL, and we should check the specs about that.

If specs are representing data like you said, we can easily add this here.

bendavies commented 7 years ago

I'm unsure about the specs either, but i would imagine that consistency of _links is a good thing, whether items are embedded or not (at level N or at level 0 (root))

Simperfit commented 7 years ago

I think we are following the specs, I have to take a look before doing anything.

bendavies commented 7 years ago

@Simperfit i think the key part is this: https://tools.ietf.org/html/draft-kelly-json-hal-08#section-4.1.2

Embedded Resources MAY be a full, partial, or inconsistent version of the representation served from the target URI.

So it seems api-platform does follow the spec, but it would be good to have a way of configuring when specific _links appear, either when the item is embedded, and/or if it is the main serialized resource

soyuka commented 7 years ago

So it seems api-platform does follow the spec, but it would be good to have a way of configuring when specific _links appear, either when the item is embedded, and/or if it is the main serialized resource

Yes but this improvement is allowed by the spec! This example correctly adds the _links for _embedded no?

bendavies commented 7 years ago

Indeed, but i meant it would be good if api-platform allowed configuring this, not the spec :)

Simperfit commented 7 years ago

Yeah I guess we could add them ;) so if someone wants / has the time to do it :)

soyuka commented 7 years ago

@bendavies wdyt we add the links without additional configuration?

bendavies commented 7 years ago

what do you mean @soyuka? Always add links if the association is an api resource?

soyuka commented 7 years ago

Yes IMO it's a good idea and it will not be so much of a cost regarding performances! Though maybe we could give the ability to disable it just in case.

bendavies commented 7 years ago

I'm unsure. It seems like there would be a performance hit. Think about non owning side relations. they require an extra query or a join. what about collections? There are many things to think about.

Though maybe we could give the ability to disable it just in case.

So you are suggesting making it opt out.

I would flip that and suggest making it opt in, per association. Obviously that needs extra configuration. It looks to me like something extra is required to be able to configure:

  1. embedding relation
{
    "_embedded": {
        "reviews": [
            {
                "_links": {
                    "self": {
                        "href": "/reviews/275"
                    }
                },
                "author": "Herbert Reinger I",
                "body": "Foo",
                "id": 275,
                "publicationDate": "1970-07-11T14:10:52+00:00",
                "rating": 5
            }
        ]
    },
    "_links": {
        "self": {
            "href": "/books/3"
        }
    },
    "author": "Prof. Joelle Doyle",
    "description": "Bar",
    "id": 3,
    "isbn": "9783899289190",
    "publicationDate": "1996-01-02T00:00:00+00:00",
    "title": "Excepturi quo dolor et officiis minima et consequatur earum."
}

https://api-platform.com/docs/core/serialization-groups-and-relations#embedding-relations

  1. providing _links to relations
    {
    "_links": {
        "reviews": [
            {
                "href": "/reviews/275"
            }
        ],
        "self": {
            "href": "/books/3"
        }
    },
    "author": "Prof. Joelle Doyle",
    "description": "Bar",
    "id": 3,
    "isbn": "9783899289190",
    "publicationDate": "1996-01-02T00:00:00+00:00",
    "title": "Excepturi quo dolor et officiis minima et consequatur earum."
    }
  2. or both
    {
    "_embedded": {
        "reviews": [
            {
                "_links": {
                    "self": {
                        "href": "/reviews/275"
                    }
                },
                "author": "Herbert Reinger I",
                "body": "Foo",
                "id": 275,
                "publicationDate": "1970-07-11T14:10:52+00:00",
                "rating": 5
            }
        ]
    },
    "_links": {
        "reviews": [
            {
                "href": "/reviews/275"
            }
        ],
        "self": {
            "href": "/books/3"
        }
    },
    "author": "Prof. Joelle Doyle",
    "description": "Bar",
    "id": 3,
    "isbn": "9783899289190",
    "publicationDate": "1996-01-02T00:00:00+00:00",
    "title": "Excepturi quo dolor et officiis minima et consequatur earum."
    }

    What seems hard is that this only seems to apply to the json hal abstraction?

soyuka commented 7 years ago

I was more thinking about adding the links for relations that are serialized already (hence the no-perf cost if we already have the data). If you want a per-association configuration for this, it'll induce complexity (use of PropertyMetadataFactory).

For example, I'd always output the third example you gave. Still I'm not using HAL nor am I familiar with its use cases. And yes, this is specific to HAL normalization.

bendavies commented 7 years ago

Quite, but the case of an embedded relation, I would usually want to see an a iri link back to the parent, or some other relation.

{
    "_embedded": {
        "reviews": [
            {
                "_links": {
                    "self": {
                        "href": "/reviews/275"
                    },
vvvvv HERE
                    "book": {
                        "href": "/book/3"
                    }
^^^^^ HERE
                },
                "author": "Herbert Reinger I",
                "body": "Foo",
                "id": 275,
                "publicationDate": "1970-07-11T14:10:52+00:00",
                "rating": 5
            }
        ]
    },
    "_links": {
        "reviews": [
            {
                "href": "/reviews/275"
            }
        ],
        "self": {
            "href": "/books/3"
        }
    },
    "author": "Prof. Joelle Doyle",
    "description": "Bar",
    "id": 3,
    "isbn": "9783899289190",
    "publicationDate": "1996-01-02T00:00:00+00:00",
    "title": "Excepturi quo dolor et officiis minima et consequatur earum."
}
soyuka commented 7 years ago

Indeed, and this has no performance cost because we know of the parent IRI already!

bendavies commented 7 years ago

Note that it would be any other relation, author for example.