Added service contract technical guidelines #3421
Conversation
- Added tech vision description
|
An admin must run tests on this PR before it can be merged. |
|
|
||
| 6.4.2. Types Hierarchy | ||
|
|
||
| 6.4.2.1. If service data interface should be extensible, it must extend from `Magento\Framework\Api\ExtensibleDataInterface`. |
There was a problem hiding this comment.
do we have examples of non-extensible data interfaces?
If I am not wrong we've concluded an agreement on AC some time ago that All the Data interfaces should be extensible.
Potentially just Value-Objects could be kept non-extensible, as an identity of value object is defined by the whole set of nested attributes, so extending the Value Object could break business invariants of such objects.
But in Magento 2 we don't have clear examples of Value Objects.
So, my suggestion here is to say that: "All Data interfaces must extend from Magento\Framework\Api\ExtensibleDataInterface to guarantee extensibility"
|
|
||
| 6.4.4.2. Service data interfaces represent domain entities and can reference related data interfaces. | ||
|
|
||
| 6.4.4.3. Service data interfaces must be implemented as DTOs with a set of symmetric getters and setters. |
There was a problem hiding this comment.
This is a limitation of the existing WebAPI framework implementation, but not the desirable state.
Btw this limitation has been eliminated recently with a code contribution by @phoenix128
magento/magento2#14801
so that new DTOs could be introduced without unnecessary setter methods and being implemented in a pure immutable fashion.
For example, we tried that on MSI track - magento/inventory#1051
Based on the above I would say that precise statement should look like: "Service data interfaces must not contain any business logic. They should represent a container of data transferable over the wire. All the business logic should be moved to services."
and we may add: "It's recommended to use immutable interfaces whenever possible".
|
|
||
| * One-dimensional indexed arrays of scalars or data interfaces: for example `string[]`, `\MyCompany\MyModuleApi\Api\Data\SomeInterface[]`. Hash maps (associative arrays) are not supported | ||
|
|
||
| * Optional scalars or data interfaces: `string|null`. It is prohibited to use just `null` |
There was a problem hiding this comment.
It's better to call this as Nullable types as this term maps to the functionality introduced in PHP 7.1
http://php.net/manual/en/migration71.new-features.php
Also, term "optional" is applicable just for input arguments to methods, but not to return values.
| * One-dimensional indexed arrays of scalars or data interfaces: for example `string[]`, `\MyCompany\MyModuleApi\Api\Data\SomeInterface[]`. Hash maps (associative arrays) are not supported | ||
|
|
||
| * Optional scalars or data interfaces: `string|null`. It is prohibited to use just `null` | ||
|
|
There was a problem hiding this comment.
you should also mention void as an allowed type
There was a problem hiding this comment.
This might not be supported by the web APIs, but I think this is the desired state.
There was a problem hiding this comment.
we fixed that for Web API framework in MSI scope and delivered to 2.3.0 Magento release
So that now all MSI services modifying data exposing through the web API return void
<route url="/V1/inventory/source-items" method="POST">
<service class="Magento\InventoryApi\Api\SourceItemsSaveInterface" method="execute"/>
<resources>
<resource ref="Magento_InventoryApi::source"/>
</resources>
</route>declare(strict_types=1);
namespace Magento\InventoryApi\Api;
/**
* Service method for source items save multiple
* Performance efficient API
*
* Used fully qualified namespaces in annotations for proper work of WebApi request parser
*
* @api
*/
interface SourceItemsSaveInterface
{
/**
* Save Multiple Source item data
*
* @param \Magento\InventoryApi\Api\Data\SourceItemInterface[] $sourceItems
* @return void
* @throws \Magento\Framework\Exception\InputException
* @throws \Magento\Framework\Validation\ValidationException
* @throws \Magento\Framework\Exception\CouldNotSaveException
*/
public function execute(array $sourceItems): void;
}There was a problem hiding this comment.
@maghamed how would REST and SOAP responses look like in this case?
There was a problem hiding this comment.
@paliarush for example, like this https://devdocs.magento.com/guides/v2.3/rest/tutorials/inventory/create-sources.html
or this
https://devdocs.magento.com/guides/v2.3/rest/tutorials/inventory/reassign-products-to-another-source.html
so, just an empty array returned
in accordance with https://jsonapi.org/ standard :
A JSON object MUST be at the root of every JSON API response document.
So, that we have to return body
|
|
||
| 6.4.4.4. Service contracts should support batch data whenever possible. For example, entity persisting interface should accept an array of entities to persist instead of a single entity. Customizations via plugins must be adjusted respectively. Batch retrieval operations must accept `SearchCriteriaInterface` and return `SearchResultInterface` to support pagination. | ||
|
|
||
| 6.4.4.5. Command operations (the ones which modify the state) must support asynchronous execution. Void must be used as return type, the status of the operation may be checked by polling of the respective query operation. Create operations must support UUID generated on the client side. |
There was a problem hiding this comment.
Command operations (the ones which modify the state)
If the description in brackets applies for all Command operations it's not accurate.
As, for example, our Repository::getList methods are also command operations, but not Query, because of getList works with the latest data via command models under the hood.
I propose to change the statement on "All services which modify the state must support asynchronous execution."
|
|
||
| 6.4.4.5. Command operations (the ones which modify the state) must support asynchronous execution. Void must be used as return type, the status of the operation may be checked by polling of the respective query operation. Create operations must support UUID generated on the client side. | ||
|
|
||
| 6.4.4.6. Command operations (the ones which modify the state) must support asynchronous execution. Void must be used as return type, the status of the operation may be checked by polling of the respective query operation. Create operations must support UUID generated on the client side. |
There was a problem hiding this comment.
looks like 6.4.4.6 fully duplicates 6.4.4.5
|
Please use all capital letters for words "must", "should", etc. |
|
|
||
| 6.4.1.3. All other APIs, including explicit extension points like Chain or Composite implementations must be placed under `MyModuleApi/Model`. | ||
|
|
||
| 6.4.1.4. Arbitrary directory nesting can be used for APIs to improve code structure. For example, `MyCompany\MyModuleApi\Api\Service\Relations\DeleteInterface`. |
There was a problem hiding this comment.
Suggest to call it RemoverInterface, name should be noun.
There was a problem hiding this comment.
We have examples in MSI which use verbs for service names and the only method execute.
There is work going on on the service contract layer for Review module and there we also agreed on the same naming.
To me it sounds more natural to have a verb, even if it is not expected for the interface.
There was a problem hiding this comment.
name should be a noun
Actually not. This rule is applicable for Classes which represent "things/entities", therefore you want to name them with nouns, and methods inside these classes represent actions over this entity so that methods usually represented by verbs.
But this approach does not work with Functors (Function objects) - https://en.wikipedia.org/wiki/Function_object
And our new services are very close to Functors, because:
- they only consist of a single method
- the method name does not introduce own semantic, its name is always "execute"
In the beginning, we even wanted to use __invoke instead of execute to prevent redundancy.
https://twitter.com/iminyaylo/status/938122403443552256
But people voted to use execute.
So, if we deal with Functors it's much more natural to them to be represented by verbs, as functors represent "action" by their nature.
You can read more about naming of factors here - https://stackoverflow.com/questions/10811169/do-you-use-nouns-for-classnames-that-represent-callable-objects
|
|
||
| 6.4.2.1. If service data interface should be extensible, it must extend from `Magento\Framework\Api\ExtensibleDataInterface`. | ||
|
|
||
| 6.4.2.1. Extensible data interfaces must not form hierarchies. Extension attributes do not work for child extensible interfaces. |
There was a problem hiding this comment.
I think it's covered in main document (point about super types). But if you want to keep it, I suggest to elaborate more, might not be obvious what is the issue.
|
|
||
| 6.4.3.3. Service contracts should not be used for read scenarios on the storefront, GraphQL should be used for storefront scenarios instead. Check out [web API technical vision]({{ page.baseurl }}/coding-standards/technical-vision/webapi.html) for more details. | ||
|
|
||
| 6.4.3.4. Each service interface should declare a single public method. Interface name should reflect task/action to be performed. Example `Magento\InventoryApi\Api\StockSourceLinksDeleteInterface::execute(array $links)`. The only exception is Repository API which may be added for convenience and must be limited to singular CRUD operations and `getList($searchCriteria)`. |
There was a problem hiding this comment.
There is a best practice to make name a noun, I would suggest to follow it. In this example we would have StockSourceLinksRemoverInterface.
There was a problem hiding this comment.
|
|
||
| 6.4.4.3. Strict typing is enforced for Service and Data interfaces located under `MyCompany/MyModuleApi/Api`. Only the following types are allowed: | ||
|
|
||
| * Scalar types: `string` (including Date and DateTime); `int`; `float`; `boolean` |
There was a problem hiding this comment.
I would suggest to create and use our own types for date types. We could have API similar to what Java has, Date, DateTime, LocalDate, LocalDateTime. I can look into this.
There was a problem hiding this comment.
this sounds like a more sophisticated topic, as you are proposing to introduce Value-Objects into Magento.
What distinguishes Value Object from Entity:
- the absence of ID or UUID
- the absence of setter methods (Value Objects are immutable only)
Even if we would introduce such entities, not sure that we should distinguish Value Objects from Data Interfaces (entities) in this document.
There was a problem hiding this comment.
I think this can be clarified separately in the future in scope of independent PR.
|
|
||
| 6.4.1.1. Service contract interfaces should be placed in separate API modules. All other modules will depend on the API module, while implementations could be easily swapped via `di.xml`. For example, if module is named `MyModule`, its APIs should be declared in the module named `MyModuleApi`. | ||
|
|
||
| 6.4.1.2. Service interfaces which should be exposed as web APIs must be placed under `MyModuleApi/Api/Service` directory. Service data interfaces must be placed under `MyModuleApi/Api/Data`. |
There was a problem hiding this comment.
Does API folder still makes sense for api modules? Maybe we should have something like below?
MyModuleApi\Data
MyModuleApi\Service
MyModuleApi\Model
There was a problem hiding this comment.
I don't see strong arguments for either of the approaches. Considering existence of Api directory in all modules currently, I'm inclined towards this option. It will make transition to the new style easier.
BTW, API modules might have more directories in the root, please take a look at https://github.com/magento-engcom/msi/tree/2.3-develop/app/code/Magento/InventoryApi as an example
| @@ -532,7 +532,75 @@ class View extends Template | |||
|
|
|||
| ### 6.4. Service Contracts (Application) layer | |||
|
|
|||
| We are reviewing this section and will publish it soon. | |||
| 6.4.1. Location | |||
There was a problem hiding this comment.
Maybe call it Location of API interfaces?
|
|
||
| 6.4.5.2. Replace strategy should be used to persist main entity fields/attributes, child entities and relation links. | ||
|
|
||
| 6.4.5.3. During update operations, Web APIs with `PATCH` HTTP method and all action controllers that accept entities should pre-load them first, merge request data on top of that and provide full data to service. |
There was a problem hiding this comment.
Should this logic be part of the service?
There was a problem hiding this comment.
it should NOT be, the same as deciding how to guarantee atomicity of these changes and prevent data inconsistency (i.e. wrapping all the process into the transaction).
This is the responsibility of the Infrastructure layer, but not the Domain one to which service belong to
There was a problem hiding this comment.
I think yes. This item was migrated from previous discussions, @buskamuza @maghamed do you know what is the history behind this item?
There was a problem hiding this comment.
We definitely don't have such ability on web API framework level at the moment and if agree to have this statement in technical vision, will have to start supporting PATCH with pre-load.
|
|
||
| 6.4.4.5. Command operations (the ones which modify the state) must support asynchronous execution. Void must be used as return type, the status of the operation may be checked by polling of the respective query operation. Create operations must support UUID generated on the client side. | ||
|
|
||
| 6.4.4.6. Command operations (the ones which modify the state) must support asynchronous execution. Void must be used as return type, the status of the operation may be checked by polling of the respective query operation. Create operations must support UUID generated on the client side. |
There was a problem hiding this comment.
How do you check status of operation if you receive void (sorry if it was discussed and I missed)?
There was a problem hiding this comment.
if you use UUID as an identity of the operation, you can check the status based on that UUID which is known in advance, so that no return value needed to resolve the current status.
So, there is no necessity to return "promise" or similar object
There was a problem hiding this comment.
By polling like in the statement. Is it not clear enough? Not sure if have examples right now, might have something in MSI. @maghamed do you know of any?
|
|
||
| 6.4.4. Service Method Signature | ||
|
|
||
| 6.4.4.1. Service contracts should not apply presentation layer formatting to the returned data. |
There was a problem hiding this comment.
Consider moving this section to Service implementation.
There was a problem hiding this comment.
Both sections have a number of items, and combining them would reduce readability.
There was a problem hiding this comment.
Some of the items not related to method signature.
There was a problem hiding this comment.
Please propose a list of exact items to be moved.
|
|
||
| 6.4.1.4. Arbitrary directory nesting can be used for APIs to improve code structure. For example, `MyCompany\MyModuleApi\Api\Service\Relations\DeleteInterface`. | ||
|
|
||
| 6.4.2. Types Hierarchy |
There was a problem hiding this comment.
Consider moving this section to Service implementation.
|
Guys, can someone help me out and remove me from these notifications? I don’t know why I suddenly start getting them. This is dev stuff, right? Sorry I’m not THAT technical.
Thanks teammates!
Luis Gomez
Marketing Lead, Wagento Creative LLC
P +1-612-594-7699
M (+52) 552-270-8322
E [email protected]
W https://www.wagento.com/
Supplier of Quality Magento Super Powers
…On Dec 10, 2018, 10:50 AM -0600, Alex Paliarush ***@***.***>, wrote:
@paliarush commented on this pull request.
In guides/v2.2/coding-standards/technical-guidelines.md:
> +
+6.4.3.3. Service contracts should not be used for read scenarios on the storefront, GraphQL should be used for storefront scenarios instead. Check out [web API technical vision]({{ page.baseurl }}/coding-standards/technical-vision/webapi.html) for more details.
+
+6.4.3.4. Each service interface should declare a single public method. Interface name should reflect task/action to be performed. Example `Magento\InventoryApi\Api\StockSourceLinksDeleteInterface::execute(array $links)`. The only exception is Repository API which may be added for convenience and must be limited to singular CRUD operations and `getList($searchCriteria)`.
+
+6.4.4. Service Method Signature
+
+6.4.4.1. Service contracts should not apply presentation layer formatting to the returned data.
+
+6.4.4.2. Service data interfaces represent domain entities and can reference related data interfaces.
+
+6.4.4.3. Service data interfaces must be implemented as DTOs with a set of symmetric getters and setters.
+
+6.4.4.3. Strict typing is enforced for Service and Data interfaces located under `MyCompany/MyModuleApi/Api`. Only the following types are allowed:
+
+* Scalar types: `string` (including Date and DateTime); `int`; `float`; `boolean`
I think this can be clarified separately in the future in scope of independent PR.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
@brentwpeterson , you can go to #3421 and select Unwatch at the top of the page. |
|
Thanks Kevin!
Fun fact is that I don’t know why I’m recognized as Brent too. I’ll work out something with him to fix this.
Cheers,
Luis Gomez
Marketing Lead, Wagento Creative LLC
P +1-612-594-7699
M (+52) 552-270-8322
E [email protected]
W https://www.wagento.com/
Supplier of Quality Magento Super Powers
…On Dec 10, 2018, 10:58 AM -0600, Kevin Harper ***@***.***>, wrote:
@brentwpeterson , you can go to #3421 and select Unwatch at the top of the page.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
| We are reviewing this section and will publish it soon. | ||
| 6.4.1. Location | ||
|
|
||
| 6.4.1.1. Service contract interfaces should be placed in separate API modules. All other modules will depend on the API module, while implementations could be easily swapped via `di.xml`. For example, if module is named `MyModule`, its APIs should be declared in the module named `MyModuleApi`. |
There was a problem hiding this comment.
Please add explicitly the "Api" part requirement for the name (not just in an example).
|
|
||
| 6.4.3.1. Methods named similarly must serve similar purpose across different services, but still might have different signatures. | ||
|
|
||
| 6.4.3.2. Services must only operate on scalar types or service data interfaces. |
There was a problem hiding this comment.
What about arrays (as lists)?
There was a problem hiding this comment.
Removed since it is very close to 6.4.3.2. Strict typing is enforced for Service and Data interfaces
|
|
||
| 6.4.3.3. Service contracts should not be used for read scenarios on the storefront, GraphQL should be used for storefront scenarios instead. Check out [web API technical vision]({{ page.baseurl }}/coding-standards/technical-vision/webapi.html) for more details. | ||
|
|
||
| 6.4.3.4. Each service interface should declare a single public method. Interface name should reflect task/action to be performed. Example `Magento\InventoryApi\Api\StockSourceLinksDeleteInterface::execute(array $links)`. The only exception is Repository API which may be added for convenience and must be limited to singular CRUD operations and `getList($searchCriteria)`. |
There was a problem hiding this comment.
Should we consider having a single-method interfaces and allow a facade with combination of such interfaces for convenience? Repository can be one of examples. Maybe there can be more.
There was a problem hiding this comment.
Single method interfaces should operate on arrays of entities. Repositories operate on individual entities. We do not prohibit creation of facades anyway.
|
|
||
| 6.4.4.3. Service data interfaces must be implemented as DTOs with a set of symmetric getters and setters. | ||
|
|
||
| 6.4.4.3. Strict typing is enforced for Service and Data interfaces located under `MyCompany/MyModuleApi/Api`. Only the following types are allowed: |
There was a problem hiding this comment.
This item is similar to 6.4.3.2. (just about output). Why that one is in the Structure section and this one is in the Signature section? Probably, both should be in the Signature section, and ideally be declared together.
|
|
||
| * Optional scalars or data interfaces: `string|null`. It is prohibited to use just `null` | ||
|
|
||
| 6.4.4.4. Service contracts should support batch data whenever possible. For example, entity persisting interface should accept an array of entities to persist instead of a single entity. Customizations via plugins must be adjusted respectively. Batch retrieval operations must accept `SearchCriteriaInterface` and return `SearchResultInterface` to support pagination. |
There was a problem hiding this comment.
I'd recommend to avoid phrases like "whenever possible". This meaning is included in the "should". On the contrary, "must" means that there are no exceptions.
|
|
||
| 6.4.5.3. During update operations, Web APIs with `PATCH` HTTP method and all action controllers that accept entities should pre-load them first, merge request data on top of that and provide full data to service. | ||
|
|
||
| 6.4.5.4. If service method needs to return a modified version of the argument, the original argument object must not be modified and its copy should be modified and returned instead. |
There was a problem hiding this comment.
What about a case when the argument should be modified, but not returned? Arguments should not be modified in any case.
|
|
||
| 6.4.6. Exception Handling | ||
|
|
||
| 6.4.6.1. Service methods must only throw exceptions defined as part of service contract layer. Exceptions thrown from lower layers should not be exposed outside and must be wrapped with appropriate service contract layer exception. |
There was a problem hiding this comment.
It's not specific to Service Layer and covered by
5.11. A separate exceptions hierarchy SHOULD be defined on each application layer. It is allowed to throw exceptions that are only defined on the same layer.
|
|
||
| 6.4.4. Service Implementation | ||
|
|
||
| 6.4.4.1. Service data interfaces MUST extend from `Magento\Framework\Api\ExtensibleDataInterface`. The only exception is when extensibility is not desired, like in case of value-objects. |
There was a problem hiding this comment.
If there is an exception, let's change MUST to SHOULD.
…on' into kh_tech-vision # Conflicts: # guides/v2.2/coding-standards/technical-guidelines.md
|
|
||
| 6.4.2. Service Interface Structure | ||
|
|
||
| 6.4.2.1. Methods that have similar MUST serve similar purposes across different services, but they still MAY have different signatures. |
There was a problem hiding this comment.
missing a word after first "similar"?
|
|
||
| 6.4.4.3. Service implementations and plugins MUST NOT rely on storage-specific integrity features, such as foreign key constraints. | ||
|
|
||
| 6.4.4.4. Replace strategy SHOULD be used to persist main entity fields/attributes, child entities, and relation links. |
There was a problem hiding this comment.
Replace strategy should be "replacement strategy", unless "replace strategy" is a technical term
|
|
||
| 6.4.4.7. Services SHOULD NOT apply ACL rules to methods or returned data. | ||
|
|
||
| 6.4.4.8. If multiple data scopes available, within one service call one entity SHOULD be persisted only for one specific data scope. |
There was a problem hiding this comment.
rewrite line 6.4.4.8 as follows: If multiple data scopes are available, at least one service call must contain a persisted entity for a specific data scope.
or, if this more accurate:
... each service call must contain a minimum of one persisted entity for a specified data scope.
|
running tests |
This PR is a:
Summary
When this pull request is merged, it will...
Additional information
List all affected URLs