Add support for topic schema defined as ServiceName:methodName #23

anvasiliev · 2018-05-22T18:19:22Z

Topics which configured as schema="ServiceName:methodName" always are synchronous topic type (see \Magento\Framework\Communication\Config\ReflectionGenerator::generateTopicConfigForServiceMethod ) and is not possible to use with \Magento\AsynchronousOperations\Model\MassConsumer

PR added new optional attribute "is_synchronous" in to urn:magento:framework:Communication/etc/communication.xsd which identify is topic are sync or async.
If topic attribute not defined or attribute value "true": topic are sync.
In case when topic attribute value "false": topic are async type, even if schema attribute or request+response attributes defined.

Example config:
communication.xml

<?xml version="1.0"?>
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:Communication/etc/communication.xsd">
<!-- ASYNC topics -->
    <topic name="async.topic.example.1" schema="Magento\Example\Api\EntityRepositoryInterface::save" is_synchronous="false">
        <handler name="async" type="Magento\Example\Api\EntityRepositoryInterface" method="save"/>
    </topic>
   <topic name="async.topic.example.2" request="string" response="string" is_synchronous="false">
        <handler name="async" type="Magento\Example\Api\EntityRepositoryInterface" method="save"/>
    </topic>

<!-- SYNC topics -->
    <topic name="sync.topic.example.3" request="string" response="string" is_synchronous="true">
        <handler name="async" type="Magento\Example\Api\EntityRepositoryInterface" method="save"/>
    </topic>
<topic name="sync.topic.example.4" request="string" response="string">
        <handler name="async" type="Magento\Example\Api\EntityRepositoryInterface" method="save"/>
    </topic>
<topic name="sync.topic.example.5" schema="Magento\Example\Api\EntityRepositoryInterface">
        <handler name="async" type="Magento\Example\Api\EntityRepositoryInterface" method="save"/>
    </topic>
</config>

queue_topology.xml

<?xml version="1.0"?>
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework-message-queue:etc/topology.xsd">
    <exchange name="magento" type="topic" connection="amqp">
        <binding id="async.operations.all" topic="async.#" destinationType="queue" destination="async.operations.all"/>
    </exchange>
</config>

queue_consumer.xml

<?xml version="1.0"?>
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xsi:noNamespaceSchemaLocation="urn:magento:framework-message-queue:etc/consumer.xsd">
    <consumer name="async.operations.all" queue="async.operations.all" connection="amqp"
              consumerInstance="Magento\AsynchronousOperations\Model\MassConsumer"/>
</config>

magento-cicd2 · 2018-05-22T18:19:26Z

All committers have signed the CLA.

magento-cicd2 · 2018-05-22T18:19:26Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

Vasiliev.A seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.

paliarush

Please explain the purpose of the task, not clear from the PR description.

paliarush · 2018-05-23T15:18:02Z

app/code/Magento/WebapiAsync/Plugin/Communication/Config/CompositeReader.php

+use Magento\AsynchronousOperations\Model\ConfigInterface;
+use Magento\Framework\Communication\ConfigInterface as CommunicationConfig;
+
+class CompositeReader


Please add class documentation

paliarush · 2018-05-23T15:21:19Z

app/code/Magento/WebapiAsync/Plugin/Communication/Config/CompositeReader.php

+class CompositeReader
+{
+    /**
+     * Topics with type schema is always are sync


Description does not sound right, please correct

vrann · 2018-06-27T21:03:51Z

@anvasiliev the problem described in bug was more related to the fact that in \Magento\Framework\MessageQueue\Consumer the topic type definition CommunicationConfig::TOPIC_REQUEST_TYPE_METHOD is supported: https://github.com/magento/bulk-api-ce/blob/2.3-develop/lib/internal/Magento/Framework/MessageQueue/Consumer.php#L135

While it is not supported here https://github.com/magento/bulk-api-ce/blob/2.3-develop/app/code/Magento/AsynchronousOperations/Model/MassConsumer.php#L109

Also, I don't think we need a plugin to convert topic to asynchronous, because all topics generated with async prefix will be defined using request type TOPIC_REQUEST_TYPE_CLASS.

But in case if somebody uses MassConsumer to process some other queue defined manually throughthe communication.xml, the support of TOPIC_REQUEST_TYPE_METHOD will be important

…E_METHOD, added new attribute is_synchronous for topic to communication.xsd schema

latest 2.1 develop

paliarush · 2018-09-04T14:45:42Z

lib/internal/Magento/Framework/Communication/Config/Reader/XmlReader/Converter.php

@@ -143,7 +144,7 @@ protected function extractTopics($config)
            } elseif ($requestSchema) {
                $output[$topicName] = [
                    Config::TOPIC_NAME => $topicName,
-                    Config::TOPIC_IS_SYNCHRONOUS => false,
+                    Config::TOPIC_IS_SYNCHRONOUS => $isSynchronous ?? false,


How can this request be synchronous? Please provide an example.

Request will be synchronous in next etc/communication.xml configs:

Topic attributes "request" and "response" are both defined, attribute "is_synchronous" not defined or "true".
<topic name="example.sync.topic" request="Magento\Example\Api\Data\EntityInterface" response="Magento\Example\Api\Data\EntityInterface"> <handler name="handler" type="Magento\Example\Api\EntityRepositoryInterface" method="save"/> </topic>

Topic attribute "schema" defined and attribute "is_synchronous" not defined or "true"
<topic name="example.sync.topic" schema="Magento\Example\Api\EntityRepositoryInterface::save"> <handler name="handler" type="Magento\Example\Api\EntityRepositoryInterface" method="save"/> </topic>

@anvasiliev, seems there is a bug in the implementation. If is_synchronous is missed then extractTopicIsSynchronous return null which is treated as false. To avoid such issue better always return boolean from extractTopicIsSynchronous.

Also, I suppose, @paliarush question was about what is the reason to support asynchrony for topics that only send data and don't expect any response. Looks like for the last elseif ($requestSchema) we should always have Config::TOPIC_IS_SYNCHRONOUS => false regardless attribute value.

paliarush · 2018-09-04T14:55:14Z

lib/internal/Magento/Framework/Communication/Config/ReflectionGenerator.php

            Config::TOPIC_REQUEST => $methodMetadata[Config::SCHEMA_METHOD_PARAMS],
            Config::TOPIC_REQUEST_TYPE => Config::TOPIC_REQUEST_TYPE_METHOD,
            Config::TOPIC_RESPONSE => $returnType,
            Config::TOPIC_HANDLERS => $handlers
-                ?: [self::DEFAULT_HANDLER => $methodMetadata[Config::SCHEMA_METHOD_HANDLER]]
+                ? : [self::DEFAULT_HANDLER => $methodMetadata[Config::SCHEMA_METHOD_HANDLER]]


I believe this is an accidental change.

paliarush · 2018-09-04T14:57:18Z

lib/internal/Magento/Framework/Communication/Config/ReflectionGenerator.php

        ];
    }

    /**
     * Generate topic name based on service type and method name.
-     *
     * Perform the following conversion:


There should be an empty line between short and long descriptions.

paliarush · 2018-09-04T15:06:07Z

lib/internal/Magento/Framework/Communication/Config/ReflectionGenerator.php

+        $serviceType,
+        $serviceMethod,
+        $handlers = [],
+        $isSynchronous = null


Is this already used anywhere?

was not used anywhere. Fixed in last commit

paliarush

Please address comments above and cover with tests.

vkublytskyi

Please review comments and failed integration tests

vkublytskyi · 2018-09-17T10:59:57Z

lib/internal/Magento/Framework/Communication/Config/Reader/XmlReader/Converter.php

+     * @param \DOMNode $topicNode
+     * @return boolean|null
+     */
+    protected function extractTopicIsSynchronous($topicNode)


As this is new method it should be private

vkublytskyi · 2018-09-17T11:02:27Z

lib/internal/Magento/Framework/Communication/Config/Reader/XmlReader/Converter.php

+        $attributeName = Config::TOPIC_IS_SYNCHRONOUS;
+        $topicAttributes = $topicNode->attributes;
+        if (!$topicAttributes->getNamedItem($attributeName)) {
+            return null;


Please avoid methods with multiple return types. If some value should be applied by default this is the best place to define it. With this PHP return type declaration should be used for new code

private function extractTopicIsSynchronous($topicNode): bool

vkublytskyi · 2018-09-17T11:06:51Z

lib/internal/Magento/Framework/Communication/Config/Reader/XmlReader/Converter.php

+            return null;
+        }
+        $value = $topicAttributes->getNamedItem($attributeName)->nodeValue;
+        $isSynchronous = filter_var($value, FILTER_VALIDATE_BOOLEAN, FILTER_NULL_ON_FAILURE);


If XML file contains inappropriate value it should lead to an exception. Ideally, such case should be avoided by providing corresponding restrictions in XSD file.

vkublytskyi · 2018-09-17T11:14:12Z

lib/internal/Magento/Framework/Communication/Config/Reader/XmlReader/Converter.php

@@ -143,7 +144,7 @@ protected function extractTopics($config)
            } elseif ($requestSchema) {
                $output[$topicName] = [
                    Config::TOPIC_NAME => $topicName,
-                    Config::TOPIC_IS_SYNCHRONOUS => false,
+                    Config::TOPIC_IS_SYNCHRONOUS => $isSynchronous ?? false,


@anvasiliev, seems there is a bug in the implementation. If is_synchronous is missed then extractTopicIsSynchronous return null which is treated as false. To avoid such issue better always return boolean from extractTopicIsSynchronous.

Also, I suppose, @paliarush question was about what is the reason to support asynchrony for topics that only send data and don't expect any response. Looks like for the last elseif ($requestSchema) we should always have Config::TOPIC_IS_SYNCHRONOUS => false regardless attribute value.

vkublytskyi · 2018-09-19T12:15:14Z

Code looks good. PR will be merged if manual QA doesn't discover bugs.

…odName #23

ghost · 2019-01-27T20:51:47Z

Hi @anvasiliev, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

Add support for topic schema defined as ServiceName:methodName

270817f

paliarush reviewed May 23, 2018

View reviewed changes

fix class and method descriptions

2f8cb97

magento-engcom-team added the Award: complex label Jun 27, 2018

merge upstream/2.3-develop into work branch

aa9f2af

Vasiliev.A added 3 commits July 25, 2018 01:12

fix for support schema request CommunicationConfig::TOPIC_REQUEST_TYP…

09b6e03

…E_METHOD, added new attribute is_synchronous for topic to communication.xsd schema

fix long string

08c35d4

Merge remote-tracking branch 'upstream/2.3-develop' into issues/13

703b2fb

magento-engcom-team pushed a commit that referenced this pull request Aug 1, 2018

Merge pull request #23 from magento/2.1-develop

64bad6b

latest 2.1 develop

nuzil requested a review from vkublytskyi August 24, 2018 15:29

paliarush reviewed Sep 4, 2018

View reviewed changes

paliarush suggested changes Sep 4, 2018

View reviewed changes

fix formatting, fix unused method param $isSynchronous

b7ea0d8

vkublytskyi suggested changes Sep 17, 2018

View reviewed changes

fix implmentation of isSynchronuos attribute processing

4de1d94

nuzil mentioned this pull request Oct 4, 2018

Add support for topic schema defined as ServiceName:methodName #13

Open

vkublytskyi self-assigned this Oct 10, 2018

paliarush approved these changes Oct 25, 2018

View reviewed changes

magento-engcom-team assigned nuzil Jan 8, 2019

magento-engcom-team added this to the Release: 2.3.1 milestone Jan 8, 2019

magento-engcom-team added Release Line: 2.3 Partner: Comwrap partners-contribution labels Jan 8, 2019

magento-engcom-team added the Component: Framework/Communication label Jan 8, 2019

vkublytskyi approved these changes Jan 15, 2019

View reviewed changes

nuzil self-requested a review January 16, 2019 08:46

nuzil approved these changes Jan 16, 2019

View reviewed changes

magento-engcom-team merged commit 4de1d94 into magento:2.3-develop Jan 27, 2019

magento-engcom-team pushed a commit that referenced this pull request Jan 27, 2019

ENGCOM-3805: Add support for topic schema defined as ServiceName:meth…

2952946

…odName #23

magento-engcom-team added Award: special achievement Award: category of expertise labels Jan 27, 2019

Add support for topic schema defined as ServiceName:methodName #23

Add support for topic schema defined as ServiceName:methodName #23

Uh oh!

Conversation

anvasiliev commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magento-cicd2 commented May 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

magento-cicd2 commented May 22, 2018

Uh oh!

paliarush left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vrann commented Jun 27, 2018

Uh oh!

paliarush Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paliarush Sep 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paliarush left a comment

Choose a reason for hiding this comment

Uh oh!

vkublytskyi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkublytskyi commented Sep 19, 2018

Uh oh!

ghost commented Jan 27, 2019

Uh oh!

Uh oh!

anvasiliev commented May 22, 2018 •

edited

Loading

magento-cicd2 commented May 22, 2018 •

edited

Loading

paliarush Sep 4, 2018 •

edited

Loading

paliarush Sep 4, 2018 •

edited

Loading