Skip to content
This repository was archived by the owner on Nov 19, 2024. It is now read-only.

Added service contract technical guidelines #3421

Merged
merged 15 commits into from
Dec 19, 2018
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions _data/toc/coding-standards.yml
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@ pages:
url: /coding-standards/technical-guidelines.html

- label: Technical vision
url: /coding-standards/technical-vision/index.html
children:

- label: Web Api
Expand Down
7 changes: 7 additions & 0 deletions guides/v2.0/coding-standards/technical-vision/index.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
group: coding-standards
title: Magento technical vision
---

A collection of component technical vision documents describes the desired state of Magento product.
Each individual technical vision relates to a set of modules logically grouped together. Technical vision typically describes component's place in the system, its architecture, extension scenarios and a list of invariants which must be preserved at all times during component refactoring or extension to ensure consistency.
7 changes: 7 additions & 0 deletions guides/v2.1/coding-standards/technical-vision/index.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
group: coding-standards
title: Magento technical vision
---

A collection of component technical vision documents describes the desired state of Magento product.
Each individual technical vision relates to a set of modules logically grouped together. Technical vision typically describes component's place in the system, its architecture, extension scenarios and a list of invariants which must be preserved at all times during component refactoring or extension to ensure consistency.
70 changes: 69 additions & 1 deletion guides/v2.2/coding-standards/technical-guidelines.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe call it Location of API interfaces?


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`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add explicitly the "Api" part requirement for the name (not just in an example).


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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does API folder still makes sense for api modules? Maybe we should have something like below?
MyModuleApi\Data
MyModuleApi\Service
MyModuleApi\Model

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggest to call it RemoverInterface, name should be noun.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@maghamed maghamed Dec 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@melnikovi

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. Types Hierarchy
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider moving this section to Service implementation.


6.4.2.1. If service data interface should be extensible, it must extend from `Magento\Framework\Api\ExtensibleDataInterface`.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.2.1. Extensible data interfaces must not form hierarchies. Extension attributes do not work for child extensible interfaces.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. Service Interface Structure

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about arrays (as lists)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)`.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a best practice to make name a noun, I would suggest to follow it. In this example we would have StockSourceLinksRemoverInterface.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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. Service Method Signature

6.4.4.1. Service contracts should not apply presentation layer formatting to the returned data.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider moving this section to Service implementation.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both sections have a number of items, and combining them would reduce readability.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some of the items not related to method signature.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please propose a list of exact items to be moved.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6.4.3.1 and 6.4.3.3.


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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".


6.4.4.3. Strict typing is enforced for Service and Data interfaces located under `MyCompany/MyModuleApi/Api`. Only the following types are allowed:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed 6.4.3.2


* Scalar types: `string` (including Date and DateTime); `int`; `float`; `boolean`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this can be clarified separately in the future in scope of independent PR.


* Data interfaces

* 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`

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you should also mention void as an allowed type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might not be supported by the web APIs, but I think this is the desired state.

Copy link

@maghamed maghamed Dec 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maghamed how would REST and SOAP responses look like in this case?

Copy link

@maghamed maghamed Dec 12, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks like 6.4.4.6 fully duplicates 6.4.4.5

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you check status of operation if you receive void (sorry if it was discussed and I missed)?

Copy link

@maghamed maghamed Dec 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.7. Data objects returned by service contracts should be fully loaded to ensure consistency.

6.4.5. Service Implementation

6.4.5.1. Service implementations and plugins must not rely on storage-specific integrity features, such as foreign key constraints.

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.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this logic be part of the service?

Copy link

@maghamed maghamed Dec 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think yes. This item was migrated from previous discussions, @buskamuza @maghamed do you know what is the history behind this item?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about a case when the argument should be modified, but not returned? Arguments should not be modified in any case.


6.4.5.5. Services should not apply ACL rules to methods or returned data.

6.4.5.6. If multiple data scopes available, within one service call entity should be persisted only for one specific data scope.

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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


## 7. Configuration

Expand Down
1 change: 1 addition & 0 deletions guides/v2.2/coding-standards/technical-vision/index.md
1 change: 1 addition & 0 deletions guides/v2.3/coding-standards/technical-vision/index.md