-
Notifications
You must be signed in to change notification settings - Fork 247
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Introduce Reservation Api #64
Conversation
- Add Reservation data interface and ReservationAppend service interface - Add Reservation model triad - Add Reservation save multiple resource model - Add Reservation builder - Update class preferences for ReservationAppendInterface, ReservationInterface and ReservationBuilderInterface - Update install schema script to create Reservation table
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if the ReservationBuilder/ ReservationAppend service would be covered with Unit tests
protected function _construct() | ||
{ | ||
$this->_init(ReservationResourceModel::class); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not mandatory to use the pseudo constructor.
Because we separated responsibilities of Model and Resource model.
According to separation of concerns (SoC) principle Model is no more responsible for self-persistence (with the help of Resource model used underneath).
Thus, we marked as @deprecated
save/delete/load methods on the level of AbstractModel
And pseudo constructor was usually used to make the connection with ResourceModel for a given Model. Which is not needed anymore.
Btw, from my point of view, we should mark _construct
method as @deprecated
as well to prevent such misunderstandings for future.
/**
* Model construct that should be used for object initialization
* @deprecated No need to use pseudo constructor anymore, please use real constructor for object initialization
* @return void
*/
protected function _construct()
{
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will create an internal ticket to get this method marked as deprecated before the 2.2.0 Magento release
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that (at least at the moment) we can get rid of Magento\Inventory\Model\ResourceModel\Reservation
class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I get the point, I can turn Reservation
into a framework-agnostic domain model with its own __constructor()
, without inheriting from AbstractModel
.
At this point calling the create()
method on the object manager gives an issue related on how the arguments array is passed.
In the ReservationBuilder
I create a Reservation
instance this way:
$data = [
ReservationInterface::RESERVATION_ID => $this->reservationId,
ReservationInterface::STOCK_ID => $this->stockId,
ReservationInterface::SKU => $this->sku,
ReservationInterface::QUANTITY => $this->quantity,
ReservationInterface::METADATA => $this->metadata,
];
$arguments = [
'data' => $data,
];
return $this->objectManager->create(ReservationInterface::class, $arguments);
For some reasons, if I don't inherit from AbstractModel
this call will fail because arguments cannot be resolved anymore in Magento\Framework\ObjectManager\Factory\AbstractFactory::resolveArgumentsInRuntime()
.
It only works if I change the code as shown below:
$arguments = [
'reservationId' => $this->reservationId,
'stockId' => $this->stockId,
'sku' => $this->sku,
'quantity' => $this->quantity,
'metadata' => $this->metadata,
];
return $this->objectManager->create(ReservationInterface::class, $arguments);
Maybe we can examine how to overcome this together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean that (at least at the moment) we can get rid of Magento\Inventory\Model\ResourceModel\Reservation class?
It dependes whether we want to use Reservation Collection or not.
At least for now we don't have such use-cases
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I get the point, I can turn Reservation into a framework-agnostic domain model with its own __constructor(), without inheriting from AbstractModel.
Yes, exactly. The intention is totally correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reasons, if I don't inherit from AbstractModel this call will fail because arguments cannot be resolved anymore in Magento\Framework\ObjectManager\Factory\AbstractFactory::resolveArgumentsInRuntime().
I see your problem. The reason of the issue is how ObjectManager implemented in Magento 2.
When you pass Entity and $arguments for this entity to ObjectManager create()
function, like this:
$this->objectManager->create(ReservationInterface::class, $arguments);
ObjectManager factory does next (simplified version):
public function create($requestedType, array $arguments = [])
{
// Get All the possible Arguments for Requested Type using Reflection
$args = $this->config->getArguments($requestedType);
// Make binding between Interface -> Class to implement for requested type
$type = $this->config->getInstanceType($requestedType);
//looping through the all possible arguments for Requested type and
//if the argument with the same Name provided for ObjectManager::create -
//pass one for object being instantiated
foreach ($args as $key => &$argument) {
if (isset($arguments[$key])) {
$argument = $arguments[$key];
}
}
What's important here, that argument resolver bind Entity's constructor parameters using the same names (keys of array) in the $arguments array (provided as the second parameter for ObjectManager::create function).
In your case, inheriting from the Magento\Framework\Model\AbstractModel
Your Reservation class inherits parent's constructor as well.
In this case, it looks like:
public function __construct(
\Magento\Framework\Model\Context $context,
\Magento\Framework\Registry $registry,
\Magento\Framework\Model\ResourceModel\AbstractResource $resource = null,
\Magento\Framework\Data\Collection\AbstractDb $resourceCollection = null,
array $data = []
) {
The most interesting in our case is $data array
.
Thus making a call:
$arguments = [
'data' => $data,
];
return $this->objectManager->create(ReservationInterface::class, $arguments);
ObjectManager makes a mapping between 'data' => $data,
and array $data
Thus at the time of Reservation instantiation ObjectManager will provide the whole array of data you prepared for the last constructor parameter ($data).
After that in the basic constructor these data would be assigned to data array
public function __construct(array $data = [])
{
$this->_data = $data;
}
Removing inheritance from the AbstractModel - you lose the basic constructor.
Thus there is no $data parameter anymore in Reservation constructor, so data you provide can't be binded.
What you need to do - is to introduce own constructor for reservation accepting each of the argument as an independent parameter. Like this:
class Reservation
{
public function __construct(
$stockId,
$sku,
$qty,
$metadata = null
) {
}
after that you can make initialization this way:
$arguments = [
ReservationInterface::STOCK_ID => $this->stockId,
ReservationInterface::SKU => $this->sku,
ReservationInterface::QUANTITY => $this->quantity,
ReservationInterface::METADATA => $this->metadata,
];
return $this->objectManager->create(ReservationInterface::class, $arguments);
each of the parameters would be bind independently in this case (not in the scope of $data array).
* | ||
* @codeCoverageIgnore | ||
*/ | ||
class Reservation extends AbstractModel implements ReservationInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's try to not use Inheritance at all.
as we are not going to expose Reservation object in API, we no need to inherit from the base Model object.
At least now, maybe in future we will see that there are some circumstances which force us to inherit from Magento\Framework\Model\AbstractExtensibleModel but at least for now, let's try to get rid of inheritance for Reservation entity
* | ||
* @api | ||
*/ | ||
interface ReservationAppendInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe make sense to rename service into ReservationsAppendInterface
I mean add plural form to Reservation, as we deal with an array of Reservations
* | ||
* @api | ||
*/ | ||
interface ReservationInterface extends ExtensibleDataInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
usually, we extend from ExtensibleDataInterface in case we want to provide an ability to add custom attributes to some particular entity using methods get/setExtensibleAttributes
you can read about this here - http://devdocs.magento.com/guides/v2.0/extension-dev-guide/extension_attributes/adding-attributes.html
We also use this functionality for entities being exposed outside, like web api.
don't think we need to extend from ExtensibleDataInterface in this particular case
)->addColumn( | ||
ReservationInterface::METADATA, | ||
Table::TYPE_TEXT, | ||
'64k', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure that 255 symbols would be enough for us.
But in current Magento implementation, framewrok detects by itself based on the provided length which MySQL data type should be used for storing data.
We definitely want that Magento created varchar data type, not the mediumtext
one.
if ($length <= 255) {
$cType = $ddlType == Table::TYPE_TEXT ? 'varchar' : 'varbinary';
$cType = sprintf('%s(%d)', $cType, $length);
} elseif ($length > 255 && $length <= 65536) {
$cType = $ddlType == Table::TYPE_TEXT ? 'text' : 'blob';
} elseif ($length > 65536 && $length <= 16777216) {
$cType = $ddlType == Table::TYPE_TEXT ? 'mediumtext' : 'mediumblob';
} else {
$cType = $ddlType == Table::TYPE_TEXT ? 'longtext' : 'longblob';
}
Looks like a good point for improvement.
I believe initial limitation in 255 symbols length
exists from the ancient times and need to be fixed.
Before Mysql version 5.0.3 Varchar datatype can store 255 character, but from 5.0.3 it can be store 65,535 characters. BUT it has a limitation of maximum row size of 65,535 bytes. It means including all columns it must not be more than 65,535 bytes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure that 255 symbols would be enough for us
I'm not sure I'm getting the point; I used a 64k
size, why do you refer to 255?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aleron75
First of all, I think 64k is too much space for this field.
Another thing is that currently, this length would result in creation MySQL data type text
because of
elseif ($length > 255 && $length <= 65536) {
$cType = $ddlType == Table::TYPE_TEXT ? 'text' : 'blob';
}
we definetly don't want to use text
on the level of MySQL. Because this is not efficient from the MySQL point of view.
The most appropriate MySQL data type for our use-case is VARCHAR (4096)
But currently, there is no possibility to create one, as we have limitations on the framework level. Magento creates varchars up to 255 symbols length only.
* | ||
* @var \Magento\Framework\ObjectManagerInterface | ||
*/ | ||
protected $_objectManager = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the property should not be protected.
Just private properties are allowed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also, it makes sense to follow the naming convention in M2.2. and call attribute as $objectManager
without underscore prefix
*/ | ||
public function build(): ReservationInterface | ||
{ | ||
$data = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to add validation here that all the properties (except metadata) are set before the build method called.
/** | ||
* Used to instantiate ReservationInterface objects | ||
* | ||
* @see ReservationInterface |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this interface should be marked as @api
because it's supposed to be called from the code of business logic
public function setQuantity(float $quantity): ReservationBuilder; | ||
|
||
/** | ||
* @param string|null $metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't think that we need to call setMetadata
passing null value
thus, the only possible input param would be string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't think that we need to call setMetadata passing null value
should this also be applied to setReservationId()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aleron75 good catch! you are right
'data' => $data, | ||
]; | ||
|
||
return $this->_objectManager->create(ReservationInterface::class, $arguments); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
after the object creation builder should clean its own state,
because we will use this builder to create several entities in a row (one by one).
So, after the first Reservation is created the state of builder should not be polluted, which will affect creation of second entity
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add a private reset()
method that cleans the builder state; I can call it after the creation of a Reservation instance before returning it. Does it sound good?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aleron75 yes, it sounds reasonable
- Transform Reservation into a pure domain model - Rename ReservationAppend service - Fix docblocks in ReservationBuilderInterface - Unmark @api in ReservationColection - Update size of Reservation metadata DB field - Remove inheritance from ExtensibleDataInterface in ReservationInterface - Add Validators for ReservationBuilder - Add String Service used to map DB fields into corresponding model fields
I've added validation for ReservationBuilder but I think there is a general issue. The This causes an issue in the |
Hi @aleron75 regarding this one:
you are totally correct. The ValidationException namespace Magento\Framework\Validation;
use Magento\Framework\Exception\AbstractAggregateException;
/**
* Add possibility to set several messages to exception
*
* @api
*/
class ValidationException extends AbstractAggregateException
{
/**
* @param ValidationResult $validationResult
* @param \Exception $cause
* @param int $code
*/
public function __construct(ValidationResult $validationResult, \Exception $cause = null, $code = 0)
{
foreach ($validationResult->getErrors() as $error) {
$this->addError($error);
}
parent::__construct($this->phrase, $cause, $code);
}
} violates 4 main rules which should be followed by Magento constructors:
So, the problem appeared as an outcome of item 4. |
@aleron75 I created a separate GitHub issue devoted to this bug - https://github.com/magento-engcom/magento2/issues/81 and added it to the project board. |
} | ||
|
||
$data = [ | ||
ReservationInterface::RESERVATION_ID => $this->reservationId, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to pass RESERVATION_ID
moreover not sure that Builder should provide a corresponding method setReservationId
if we rely on auto generated IDs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to pass RESERVATION_ID
if we don't allow the ReservationBuilder to be able to instantiate a Reservation object with an ID this means that the ReservationBuilder can't be used to create a representation of persisted entities.
I'm not saying this is wrong, just asking for a confirmation.
|
||
$value = $reservationBuilder->getStockId(); | ||
|
||
if (false === is_numeric($value)) { // TODO should also check if it is positive? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
potentially we could check whether provided StockId exists in the system.
But that will introduce performance degradation. Because we need to make additional service call for each builder::build
So, let's leave as-is now
|
||
$value = $reservationBuilder->getSku(); | ||
|
||
if ('' === trim($value)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
potentially we could check whether provided SKU exists in the system.
But that will introduce performance degradation. Because we need to make additional service call (ProductRepository::get(SKU)) for each builder::build
* @param int $reservationId | ||
* @return ReservationBuilder | ||
*/ | ||
public function setReservationId($reservationId): ReservationBuilder; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure that we need this method.
Because currently we rely on Database generated IDs for reservations
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As commented above, I introduced this thinking that the ReservationBuilder could be used to instantiate persisted entities but if this is not the case I can remove it.
- Remove Reservation ID from ReservationBuilderInterface and implementing class - Add ReservationBuilder unit test
…into msi-reservation
int $stockId, | ||
string $sku, | ||
float $quantity, | ||
$metadata |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
metadata should be nullable
$metadata = null
* | ||
* @var \Magento\Framework\ObjectManagerInterface | ||
*/ | ||
private $objectManager = null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to provide a default null value in variable declaration
/** | ||
* String Service instance | ||
* | ||
* @var \Magento\Inventory\Model\String\Service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I propose to call this service as
Magento\Inventory\Model\SnakeToCamelCaseConvertor
namespace Magento\Inventory\Model;
class SnakeToCamelCaseConvertor
{
public function convert(array $data);
}
its responsibility to convert not Array Keys, but array Elements.
and to introduce new dedicated service which will accept $data
, retrieves array_keys from that $data array and make a conversion using SnakeToCamelCaseConvertor.
Because currently our service \Magento\Inventory\Model\String\Service
is not re-usable, because it relies on specific array format and makes conversion of Array Keys.
It's better to have 2 dedicated services, each one responsible for own small task.
It's easier to test and re-use such code
/** | ||
* @return int|null | ||
*/ | ||
public function getStockId() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we no need getter methods in Builder like
getStockId/getSku/getQty
{ | ||
$this->validationResultFactory = $validationResultFactory; | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't forget to add PHP DocBlock for all methods
/** | ||
* Resource Collection of Reservation entities | ||
*/ | ||
class Collection extends AbstractCollection |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure we need Reservation Collection and ReservationResource model at all.
Because we no need to getList of reservations.
Thus, these two classes could be eliminated. At least for now
public function convertStringFromSnakeToCamelCase(string $string): string | ||
{ | ||
return lcfirst(str_replace(' ', '', ucwords(str_replace('_', ' ', $string)))); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I prefer to split this service onto two separate services.
* Copyright © Magento, Inc. All rights reserved. | ||
* See COPYING.txt for license details. | ||
*/ | ||
namespace Magento\Inventory\Model; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we need to move current Builder Interface to
Magento\InventoryApi\Api\
It's compatible with what we have for DataInterface factories, for example, auto-generated factory for ProductInterface would be ProductInterfaceFactory generated in the same namespace (API)
*/ | ||
namespace Magento\Inventory\Model\String; | ||
|
||
class Service |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW makes sense to cover this service with Unit Tests
$reservationBuilder->setMetadata($reservationData[ReservationInterface::METADATA]); | ||
|
||
$this->assertSame($this->reservation, $reservationBuilder->build()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add Test covering Negative cases, when validation not passed and we need to throw a ValidationException
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I may be wrong but in ReservationBuilder the throw new ValidationException($validationResult);
doesn't allow me to mock the ValidationException
object so I will incur in issue #81.
Let me know if I can use a workaround to avoid direct exception instantiation or we have to wait for issue #81 to be fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can use in your Unit Test
$this->expectException(ValidationException::class);
That will make sure that Exception has been thrown.
In addition to the expectException() method the expectExceptionCode(), expectExceptionMessage(), and expectExceptionMessageRegExp() methods exist to set up expectations for exceptions raised by the code under test.
Alternatively, you can use the @ExpectedException, @expectedExceptionCode, @expectedExceptionMessage, and @expectedExceptionMessageRegExp annotations to set up expectations for exceptions raised by the code under test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried but the ValidationException
is not thrown because when I instance it with throw new ValidationException($validationResult);
in ReservationBuilder::build()
I incur in issue #81 ang get the following error:
Magento\Inventory\Test\Unit\Model\ReservationBuilderTest::testThrowValidationException with data set #0 (array(1, 'somesku', 11, 'some meta data'), array(1, 'somesku', 11, 'some meta data'))
TypeError: Argument 1 passed to Magento\Framework\Exception\AbstractAggregateException::__construct() must be an instance of Magento\Framework\Phrase, null given, called in /app/magento2/lib/internal/Magento/Framework/Validation/ValidationException.php on line 27
/app/magento2/lib/internal/Magento/Framework/Exception/AbstractAggregateException.php:44
/app/magento2/lib/internal/Magento/Framework/Validation/ValidationException.php:27
/app/magento2/app/code/Magento/Inventory/Model/ReservationBuilder.php:113
/app/magento2/app/code/Magento/Inventory/Test/Unit/Model/ReservationBuilderTest.php:139
That's why I tried to mock the ValidationException
but creating it via Object Manager and then throwing it doesn't seem to work.
- Set nullable $metadata property in Reservation - Move ReservationBuilderInterface under Magento_InventoryApi module and update references in classes and di.xml - Remove nullable $objectManager in ReservationBuilder - Remove getters from ReservationBuilder - Add SnakeToCamelCaseConvertor service and test - Use SnakeToCamelCaseConvertor in ReservationBuilder - Add missing DocBlock in ReservationBuilder validators - Delete Reservation resource model and collection - Split String Service functionality between SnakeToCamelCaseConvertor and ReservationBuilder and remove String Service
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good for me.
-- refactoring, fixes
-- refactoring, fixes
-- refactoring, fixes
-- refactoring, fixes
[lynx] MC-38772 : Apply automation to fix non-MFTF tests for MDEs
Introduction
This PR is a replacement for PR#60.
Description
Note
Since the Install Schema has been updated, you need to re-install the Inventory module, wiping out existing data structure and content.
A more practical approach I followed:
Magento_Inventory
entry fromsetup_module
table but keep module's tables and dataMagento\Inventory\Setup\InstallSchema::install()
leaving only$this->createReservationTable->execute($setup);
bin/magento setup:upgrade
Magento\Inventory\Setup\InstallSchema::install()