From fcb337670db0b0493905e4641bf91693cc38de74 Mon Sep 17 00:00:00 2001 From: soyuka Date: Thu, 19 Nov 2020 11:10:57 +0100 Subject: [PATCH 1/8] Subresource definition ADR proposal --- docs/adr/0000-subresources-definition.md | 112 +++++++++++++++++++++++ 1 file changed, 112 insertions(+) create mode 100644 docs/adr/0000-subresources-definition.md diff --git a/docs/adr/0000-subresources-definition.md b/docs/adr/0000-subresources-definition.md new file mode 100644 index 00000000000..2e7a9db0307 --- /dev/null +++ b/docs/adr/0000-subresources-definition.md @@ -0,0 +1,112 @@ +# Subresource definition + +* Status: proposed +* Deciders: @dunglas, @vincentchalamon, @soyuka, @GregoireHebert, @Deuchnord + +## Context and Problem Statement + +Subresources introduced in 2017 (#904) introduced the `ApiSubresource` annotation. This definition came along with its own set of issues (#2706) and needs a refreshment. On top of that, write support on subresources is a wanted feature and it is hard to implement currently (#2598) (See [0001-subresource-write-support](./0001-subresource-write-support.md)). How can we revamp subresources to improve the developer experience and reduce the complexity? + +## Considered Options + +* Fix the actual `ApiSubresource` annotation +* Use multiple `ApiResource` to declare subresources and deprecate `ApiSubresource` +* Deprecate subresources + +## Decision Outcome + +We choose to use multiple `ApiResource` annotations to declare subresources on a given Model class: + +* Subresource declaration is an important feature and removing it would harm the software. +* The `ApiSubresource` annotation is declared on a Model's properties, which was identified as the root of several issues. For example, finding what class it is defined on (#3458). Having multiple `ApiResource` would improve a lot the declaration of our internal metadata and would cause less confusion for developers. +* The `path` of these multiple `ApiResource` needs to be implicitly described. + +### Examples + +A Company resource with a Company Users subresource on (`/companies/1/users`); + +```php +/** + * @ApiResource() + * @ApiResource(path="/companies/{companyId}/users") + */ +class Company { + public int $id; + public array $users = []; +} +``` + +With explicit identifiers: + +```php +/** + * @ApiResource() + * @ApiResource(path="/companies/{companyId}/users", identifiers={"companyId": {Company::class, "id"}}) + */ +class Company { + public int $id; + public array $users = []; +} +``` + +Two-level subresource to get the users belonging to the company 1 located in France `/countries/fr/companies/1/users`: + +```php +/** + * @ApiResource() + * @ApiResource(path="/countries/{countryId}/companies/{companyId}/users") + */ +class Country { + public int $id; + public array $companies = []; +} +``` + +With explicit identifiers: + +```php +/** + * @ApiResource() + * @ApiResource(path="/countries/{countryId}/companies/{companyId}/users", identifiers={"companyId": {Company::class, "id"}, "countryId": {Country::class, "shortName"}}) + */ +class Country { + public string $shortName; + public array $companies = []; +} +``` + +Get the company employees or administrators `/companies/1/administrators`: + +```php +/** + * @ApiResource() + * @ApiResource(path="/companies/{companyId}/administrators") + * @ApiResource(path="/companies/{companyId}/employees") + */ +class Company { + public int $id; + public Users $employees; + public Users $administrators; +} +``` + +With explicit identifiers: + +```php +/** + * @ApiResource() + * @ApiResource(path="/companies/{companyId}/administrators", identifiers={"companyId": {Company::class, "id"}, "*": {Company::class, "administrators"}}) + * @ApiResource(path="/companies/{companyId}/employees", identifiers={"companyId": {Company::class, "id"}, "*": {Company::class, "employees"}}) + */ +class Company { + public int $id; + public Users $employees; + public Users $administrators; +} +``` + +## TODO: + +* Without explicit identifiers, how do we map `companyId` to Company->id ? +* Do we parse the path to find `administrators` and map it to the property ? +* The Tuple `identifiers={pathParameter: {Class, property}}` should be redefined / validated (and what about `*` for collection?) From a18abb35c55955b75151d2d9637f6be97c6e4a38 Mon Sep 17 00:00:00 2001 From: soyuka Date: Thu, 19 Nov 2020 11:23:05 +0100 Subject: [PATCH 2/8] fix links --- docs/adr/0000-subresources-definition.md | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/docs/adr/0000-subresources-definition.md b/docs/adr/0000-subresources-definition.md index 2e7a9db0307..0949dc847ea 100644 --- a/docs/adr/0000-subresources-definition.md +++ b/docs/adr/0000-subresources-definition.md @@ -5,7 +5,7 @@ ## Context and Problem Statement -Subresources introduced in 2017 (#904) introduced the `ApiSubresource` annotation. This definition came along with its own set of issues (#2706) and needs a refreshment. On top of that, write support on subresources is a wanted feature and it is hard to implement currently (#2598) (See [0001-subresource-write-support](./0001-subresource-write-support.md)). How can we revamp subresources to improve the developer experience and reduce the complexity? +Subresources introduced in 2017 ([#904][pull/904]) introduced the `ApiSubresource` annotation. This definition came along with its own set of issues ([#2706][issue/2706]) and needs a refreshment. On top of that, write support on subresources is a wanted feature and it is hard to implement currently ([#2598][pull/2598]) (See [0001-subresource-write-support](./0001-subresource-write-support.md)). How can we revamp subresources to improve the developer experience and reduce the complexity? ## Considered Options @@ -18,7 +18,7 @@ Subresources introduced in 2017 (#904) introduced the `ApiSubresource` annotatio We choose to use multiple `ApiResource` annotations to declare subresources on a given Model class: * Subresource declaration is an important feature and removing it would harm the software. -* The `ApiSubresource` annotation is declared on a Model's properties, which was identified as the root of several issues. For example, finding what class it is defined on (#3458). Having multiple `ApiResource` would improve a lot the declaration of our internal metadata and would cause less confusion for developers. +* The `ApiSubresource` annotation is declared on a Model's properties, which was identified as the root of several issues. For example, finding what class it is defined on ([#3458][issue/3458]). Having multiple `ApiResource` would improve a lot the declaration of our internal metadata and would cause less confusion for developers. * The `path` of these multiple `ApiResource` needs to be implicitly described. ### Examples @@ -110,3 +110,14 @@ class Company { * Without explicit identifiers, how do we map `companyId` to Company->id ? * Do we parse the path to find `administrators` and map it to the property ? * The Tuple `identifiers={pathParameter: {Class, property}}` should be redefined / validated (and what about `*` for collection?) + +## Links + +* [Subresource refactor][pull/3689] + + +[pull/904]: https://github.com/api-platform/core/pull/904 "Subresource feature" +[issue/2706]: https://github.com/api-platform/core/issues/2706 "Subresource RFC" +[pull/2598]: https://github.com/api-platform/core/pull/2598 "Subresource write support" +[issue/3458]: https://github.com/api-platform/core/pull/3458 "Subresource poor DX" +[pull/3689]: https://github.com/api-platform/core/pull/3689 "Revamp subresource" From 3af682c15a39444b263bfe27aac086983b99d5b4 Mon Sep 17 00:00:00 2001 From: soyuka Date: Thu, 19 Nov 2020 12:17:05 +0100 Subject: [PATCH 3/8] Review --- docs/adr/0000-subresources-definition.md | 39 +++++++++++++++++------- 1 file changed, 28 insertions(+), 11 deletions(-) diff --git a/docs/adr/0000-subresources-definition.md b/docs/adr/0000-subresources-definition.md index 0949dc847ea..2a0496ef4c9 100644 --- a/docs/adr/0000-subresources-definition.md +++ b/docs/adr/0000-subresources-definition.md @@ -20,19 +20,19 @@ We choose to use multiple `ApiResource` annotations to declare subresources on a * Subresource declaration is an important feature and removing it would harm the software. * The `ApiSubresource` annotation is declared on a Model's properties, which was identified as the root of several issues. For example, finding what class it is defined on ([#3458][issue/3458]). Having multiple `ApiResource` would improve a lot the declaration of our internal metadata and would cause less confusion for developers. * The `path` of these multiple `ApiResource` needs to be implicitly described. +* An `ApiResource` is always defined on the Resource it represents: `/companies/1/users` outputs Users and should be defined on the `User` model. ### Examples -A Company resource with a Company Users subresource on (`/companies/1/users`); +Get Users belonging to the company on (`/companies/1/users`); ```php /** * @ApiResource() * @ApiResource(path="/companies/{companyId}/users") */ -class Company { - public int $id; - public array $users = []; +class User { + public array $companies = []; } ``` @@ -43,23 +43,25 @@ With explicit identifiers: * @ApiResource() * @ApiResource(path="/companies/{companyId}/users", identifiers={"companyId": {Company::class, "id"}}) */ -class Company { - public int $id; - public array $users = []; +class User { + public array $companies = []; } ``` -Two-level subresource to get the users belonging to the company 1 located in France `/countries/fr/companies/1/users`: +Two-level subresource to get the Users belonging to the Company #1 located in France `/countries/fr/companies/1/users`: ```php /** * @ApiResource() * @ApiResource(path="/countries/{countryId}/companies/{companyId}/users") */ -class Country { - public int $id; +class Users { public array $companies = []; } + +class Company { + public array $countries = []; +} ``` With explicit identifiers: @@ -69,9 +71,16 @@ With explicit identifiers: * @ApiResource() * @ApiResource(path="/countries/{countryId}/companies/{companyId}/users", identifiers={"companyId": {Company::class, "id"}, "countryId": {Country::class, "shortName"}}) */ +class Users { + public array $companies = []; +} + +class Company { + public array $countries = []; +} + class Country { public string $shortName; - public array $companies = []; } ``` @@ -83,6 +92,10 @@ Get the company employees or administrators `/companies/1/administrators`: * @ApiResource(path="/companies/{companyId}/administrators") * @ApiResource(path="/companies/{companyId}/employees") */ +class Users { + public array $companies; +} + class Company { public int $id; public Users $employees; @@ -98,6 +111,10 @@ With explicit identifiers: * @ApiResource(path="/companies/{companyId}/administrators", identifiers={"companyId": {Company::class, "id"}, "*": {Company::class, "administrators"}}) * @ApiResource(path="/companies/{companyId}/employees", identifiers={"companyId": {Company::class, "id"}, "*": {Company::class, "employees"}}) */ +class Users { + public array $companies; +} + class Company { public int $id; public Users $employees; From 1ddd7c6bd3109cf382d60f45ca827f95427a95b9 Mon Sep 17 00:00:00 2001 From: soyuka Date: Thu, 19 Nov 2020 12:18:48 +0100 Subject: [PATCH 4/8] Review --- docs/adr/0000-subresources-definition.md | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/docs/adr/0000-subresources-definition.md b/docs/adr/0000-subresources-definition.md index 2a0496ef4c9..8f77e8e4a53 100644 --- a/docs/adr/0000-subresources-definition.md +++ b/docs/adr/0000-subresources-definition.md @@ -5,7 +5,7 @@ ## Context and Problem Statement -Subresources introduced in 2017 ([#904][pull/904]) introduced the `ApiSubresource` annotation. This definition came along with its own set of issues ([#2706][issue/2706]) and needs a refreshment. On top of that, write support on subresources is a wanted feature and it is hard to implement currently ([#2598][pull/2598]) (See [0001-subresource-write-support](./0001-subresource-write-support.md)). How can we revamp subresources to improve the developer experience and reduce the complexity? +Subresources introduced in 2017 ([#904][pull/904]) the `ApiSubresource` annotation. This definition came along with its own set of issues ([#2706][issue/2706]) and needs a refreshment. On top of that, write support on subresources is a wanted feature and it is hard to implement currently ([#2598][pull/2598]) (See [0001-subresource-write-support](./0001-subresource-write-support.md)). How can we revamp the Subresource definition to improve the developer experience and reduce the complexity? ## Considered Options @@ -28,7 +28,7 @@ Get Users belonging to the company on (`/companies/1/users`); ```php /** - * @ApiResource() + * @ApiResource(path="/users") * @ApiResource(path="/companies/{companyId}/users") */ class User { @@ -40,7 +40,7 @@ With explicit identifiers: ```php /** - * @ApiResource() + * @ApiResource(path="/users") * @ApiResource(path="/companies/{companyId}/users", identifiers={"companyId": {Company::class, "id"}}) */ class User { @@ -52,7 +52,7 @@ Two-level subresource to get the Users belonging to the Company #1 located in Fr ```php /** - * @ApiResource() + * @ApiResource(path="/users") * @ApiResource(path="/countries/{countryId}/companies/{companyId}/users") */ class Users { @@ -68,7 +68,7 @@ With explicit identifiers: ```php /** - * @ApiResource() + * @ApiResource(path="/users") * @ApiResource(path="/countries/{countryId}/companies/{companyId}/users", identifiers={"companyId": {Company::class, "id"}, "countryId": {Country::class, "shortName"}}) */ class Users { @@ -88,7 +88,7 @@ Get the company employees or administrators `/companies/1/administrators`: ```php /** - * @ApiResource() + * @ApiResource(path="/users") * @ApiResource(path="/companies/{companyId}/administrators") * @ApiResource(path="/companies/{companyId}/employees") */ @@ -107,7 +107,7 @@ With explicit identifiers: ```php /** - * @ApiResource() + * @ApiResource(path="/users") * @ApiResource(path="/companies/{companyId}/administrators", identifiers={"companyId": {Company::class, "id"}, "*": {Company::class, "administrators"}}) * @ApiResource(path="/companies/{companyId}/employees", identifiers={"companyId": {Company::class, "id"}, "*": {Company::class, "employees"}}) */ From 6350ea095e418c3c883baffd13a891e70024efa1 Mon Sep 17 00:00:00 2001 From: soyuka Date: Thu, 19 Nov 2020 12:37:01 +0100 Subject: [PATCH 5/8] Review --- docs/adr/0000-subresources-definition.md | 85 ++++++++++++++---------- 1 file changed, 51 insertions(+), 34 deletions(-) diff --git a/docs/adr/0000-subresources-definition.md b/docs/adr/0000-subresources-definition.md index 8f77e8e4a53..88ff47a91a0 100644 --- a/docs/adr/0000-subresources-definition.md +++ b/docs/adr/0000-subresources-definition.md @@ -5,7 +5,7 @@ ## Context and Problem Statement -Subresources introduced in 2017 ([#904][pull/904]) the `ApiSubresource` annotation. This definition came along with its own set of issues ([#2706][issue/2706]) and needs a refreshment. On top of that, write support on subresources is a wanted feature and it is hard to implement currently ([#2598][pull/2598]) (See [0001-subresource-write-support](./0001-subresource-write-support.md)). How can we revamp the Subresource definition to improve the developer experience and reduce the complexity? +Subresources introduced in 2017 ([#904][pull/904]) the `ApiSubresource` annotation. This definition came along with its own set of issues ([#2706][issue/2706]) and needs a refreshment. On top of that, write support on subresources is a wanted feature and it is hard to implement currently ([#2598][pull/2598]) (See [ADR-0001-subresource-write-support](./0001-subresource-write-support.md)). How can we revamp the Subresource definition to improve the developer experience and reduce the complexity? ## Considered Options @@ -21,6 +21,7 @@ We choose to use multiple `ApiResource` annotations to declare subresources on a * The `ApiSubresource` annotation is declared on a Model's properties, which was identified as the root of several issues. For example, finding what class it is defined on ([#3458][issue/3458]). Having multiple `ApiResource` would improve a lot the declaration of our internal metadata and would cause less confusion for developers. * The `path` of these multiple `ApiResource` needs to be implicitly described. * An `ApiResource` is always defined on the Resource it represents: `/companies/1/users` outputs Users and should be defined on the `User` model. +* PropertyInfo and Doctrine metadata can be used to define how is the Resource identified according to the given path. ### Examples @@ -32,18 +33,26 @@ Get Users belonging to the company on (`/companies/1/users`); * @ApiResource(path="/companies/{companyId}/users") */ class User { + /** @ApiProperty(identifier=true) */ + public int $id; + + /** @var Company[] */ public array $companies = []; } ``` -With explicit identifiers: +With explicit identifiers, the tuple is explained in [ADR-0002-identifiers](./0002-identifiers) `{parameterName: {Class, property}}`: ```php /** - * @ApiResource(path="/users") - * @ApiResource(path="/companies/{companyId}/users", identifiers={"companyId": {Company::class, "id"}}) + * @ApiResource(path="/users", identifiers={"id": {User::class, "id"}}) + * @ApiResource(path="/companies/{companyId}/users", identifiers={"companyId": {Company::class, "id"}, "id": {User::class, "id"}}) */ class User { + /** @ApiProperty(identifier=true) */ + public int $id; + + /** @var Company[] */ public array $companies = []; } ``` @@ -55,31 +64,53 @@ Two-level subresource to get the Users belonging to the Company #1 located in Fr * @ApiResource(path="/users") * @ApiResource(path="/countries/{countryId}/companies/{companyId}/users") */ -class Users { +class User { + /** @ApiProperty(identifier=true) */ + public int $id; + + /** @var Company[] */ public array $companies = []; } class Company { + /** @ApiProperty(identifier=true) */ + public int $id; + + /** @var Country[] **/ public array $countries = []; } + +class Country { + /** @ApiProperty(identifier=true) */ + public string $shortName; +} ``` With explicit identifiers: ```php /** - * @ApiResource(path="/users") - * @ApiResource(path="/countries/{countryId}/companies/{companyId}/users", identifiers={"companyId": {Company::class, "id"}, "countryId": {Country::class, "shortName"}}) + * @ApiResource(path="/users", identifiers={"id": {User::class, "id"}}) + * @ApiResource(path="/countries/{countryId}/companies/{companyId}/users", identifiers={"companyId": {Company::class, "id"}, "countryId": {Country::class, "shortName"}, "id": {User::class, "id"}}) */ -class Users { +class User { + /** @ApiProperty(identifier=true) */ + public int $id; + + /** @var Company[] */ public array $companies = []; } class Company { + /** @ApiProperty(identifier=true) */ + public int $id; + + /** @var Country[] **/ public array $countries = []; } class Country { + /** @ApiProperty(identifier=true) */ public string $shortName; } ``` @@ -92,41 +123,27 @@ Get the company employees or administrators `/companies/1/administrators`: * @ApiResource(path="/companies/{companyId}/administrators") * @ApiResource(path="/companies/{companyId}/employees") */ -class Users { - public array $companies; -} - -class Company { +class User { + /** @ApiProperty(identifier=true) */ public int $id; - public Users $employees; - public Users $administrators; -} -``` - -With explicit identifiers: -```php -/** - * @ApiResource(path="/users") - * @ApiResource(path="/companies/{companyId}/administrators", identifiers={"companyId": {Company::class, "id"}, "*": {Company::class, "administrators"}}) - * @ApiResource(path="/companies/{companyId}/employees", identifiers={"companyId": {Company::class, "id"}, "*": {Company::class, "employees"}}) - */ -class Users { - public array $companies; + /** @var Company[] */ + public array $companies = []; } class Company { + /** @ApiProperty(identifier=true) */ public int $id; - public Users $employees; - public Users $administrators; + + /** @var User[] **/ + public array $employees; + + /** @var User[] **/ + public array $administrators; } ``` -## TODO: - -* Without explicit identifiers, how do we map `companyId` to Company->id ? -* Do we parse the path to find `administrators` and map it to the property ? -* The Tuple `identifiers={pathParameter: {Class, property}}` should be redefined / validated (and what about `*` for collection?) +This example will require a custom DataProvider as the discriminator needs to be explicit. ## Links From 9d6fad40eb2d9a5a711cf8132c18b9182f8edd38 Mon Sep 17 00:00:00 2001 From: soyuka Date: Thu, 19 Nov 2020 13:54:03 +0100 Subject: [PATCH 6/8] explicit --- docs/adr/0000-subresources-definition.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/adr/0000-subresources-definition.md b/docs/adr/0000-subresources-definition.md index 88ff47a91a0..6d047d50206 100644 --- a/docs/adr/0000-subresources-definition.md +++ b/docs/adr/0000-subresources-definition.md @@ -19,7 +19,7 @@ We choose to use multiple `ApiResource` annotations to declare subresources on a * Subresource declaration is an important feature and removing it would harm the software. * The `ApiSubresource` annotation is declared on a Model's properties, which was identified as the root of several issues. For example, finding what class it is defined on ([#3458][issue/3458]). Having multiple `ApiResource` would improve a lot the declaration of our internal metadata and would cause less confusion for developers. -* The `path` of these multiple `ApiResource` needs to be implicitly described. +* The `path` of these multiple `ApiResource` needs to be explicitly described. * An `ApiResource` is always defined on the Resource it represents: `/companies/1/users` outputs Users and should be defined on the `User` model. * PropertyInfo and Doctrine metadata can be used to define how is the Resource identified according to the given path. From 0ca717cf675a78a7a784b773de12b00f10a92a3c Mon Sep 17 00:00:00 2001 From: soyuka Date: Thu, 19 Nov 2020 15:39:44 +0100 Subject: [PATCH 7/8] review --- docs/adr/0000-subresources-definition.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/adr/0000-subresources-definition.md b/docs/adr/0000-subresources-definition.md index 6d047d50206..0045548b601 100644 --- a/docs/adr/0000-subresources-definition.md +++ b/docs/adr/0000-subresources-definition.md @@ -9,7 +9,7 @@ Subresources introduced in 2017 ([#904][pull/904]) the `ApiSubresource` annotati ## Considered Options -* Fix the actual `ApiSubresource` annotation +* Fix the current `ApiSubresource` annotation * Use multiple `ApiResource` to declare subresources and deprecate `ApiSubresource` * Deprecate subresources From a0093405e5b3172030d48cf4b6d072bc1af58c2c Mon Sep 17 00:00:00 2001 From: Antoine Bluchet Date: Wed, 25 Nov 2020 09:54:36 +0100 Subject: [PATCH 8/8] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Kévin Dunglas --- docs/adr/0000-subresources-definition.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/adr/0000-subresources-definition.md b/docs/adr/0000-subresources-definition.md index 0045548b601..72cccb17e03 100644 --- a/docs/adr/0000-subresources-definition.md +++ b/docs/adr/0000-subresources-definition.md @@ -1,4 +1,4 @@ -# Subresource definition +# Subresource Definition * Status: proposed * Deciders: @dunglas, @vincentchalamon, @soyuka, @GregoireHebert, @Deuchnord