Refactor REST APIs to handle AWS Cloud Instance entities

@yas

I have created a patch. Could you please check it?

note: Issue #3228270 and #3228276 are included in this patch.
If this patch is merged, will those two issues be closed as "won't fix"?

@yas

I have recreated a patch.
I created Ec2OperationsService and modified the source code to use its methods in ApiController and InstanceXxxForm classes.
Could you please check it?

@xiaohua-guan @yas

I updated the source code based on your comment. Please check it.

@yas I tried to load Ec2OperationsServiceInterface into InstanceStartForm by DI.

When I put it as the second argument, the value assigned to it during the DI was of EntityRepository type.

  public function __construct(Ec2ServiceInterface $ec2_service,
                              Ec2OperationsServiceInterface $ec2_operations_service,
                              EntityRepositoryInterface $entity_repository,
                              EntityTypeBundleInfoInterface $entity_type_bundle_info,
                              TimeInterface $time,
                              Messenger $messenger,
                              EntityTypeManager $entity_type_manager,
                              CacheBackendInterface $cacheRender,
                              CachedDiscoveryClearerInterface $plugin_cache_clearer,
                              EntityLinkRendererInterface $entity_link_renderer,
                              CloudConfigPluginManagerInterface $cloud_config_plugin_manager) {

TypeError: Drupal\aws_cloud\Form\Ec2\InstanceStartForm->__construct() 内 Argument 2 passed to Drupal\aws_cloud\Form\Ec2\InstanceStartForm::__construct() must be an instance of Drupal\aws_cloud\Service\Ec2\Ec2OperationsServiceInterface, instance of Drupal\Core\Entity\EntityRepository given, called in /var/www/html/web/modules/contrib/cloud/modules/cloud_service_providers/aws_cloud/src/Form/Ec2/AwsDeleteForm.php on line 136 (/var/www/html/web/modules/contrib/cloud/modules/cloud_service_providers/aws_cloud/src/Form/Ec2/InstanceStartForm.php の 59 行目)

I guessed that the order of the arguments was important, and when I added it to the last argument, I got an error like "Wrong number of arguments".

  public function __construct(Ec2ServiceInterface $ec2_service,
                              EntityRepositoryInterface $entity_repository,
                              EntityTypeBundleInfoInterface $entity_type_bundle_info,
                              TimeInterface $time,
                              Messenger $messenger,
                              EntityTypeManager $entity_type_manager,
                              CacheBackendInterface $cacheRender,
                              CachedDiscoveryClearerInterface $plugin_cache_clearer,
                              EntityLinkRendererInterface $entity_link_renderer,
                              CloudConfigPluginManagerInterface $cloud_config_plugin_manager,
                              Ec2OperationsServiceInterface $ec2_operations_service) {

ArgumentCountError: Drupal\aws_cloud\Form\Ec2\InstanceStartForm->__construct() 内 Too few arguments to function Drupal\aws_cloud\Form\Ec2\InstanceStartForm::__construct(), 10 passed in /var/www/html/web/modules/contrib/cloud/modules/cloud_service_providers/aws_cloud/src/Form/Ec2/AwsDeleteForm.php on line 136 and exactly 11 expected (/var/www/html/web/modules/contrib/cloud/modules/cloud_service_providers/aws_cloud/src/Form/Ec2/InstanceStartForm.php の 59 行目)

From this, it is guessed that it is not possible to call Ec2OperationsServiceInterface with DI.

@yas

As you pointed out, I created InstanceStartForm::create() for overriding AwsDeleteForm::create().

  public function __construct(Ec2ServiceInterface $ec2_service,
                              Ec2OperationsServiceInterface $ec2_operations_service,
                              EntityRepositoryInterface $entity_repository,
                              EntityTypeBundleInfoInterface $entity_type_bundle_info,
                              TimeInterface $time,
                              Messenger $messenger,
                              EntityTypeManager $entity_type_manager,
                              CacheBackendInterface $cacheRender,
                              CachedDiscoveryClearerInterface $plugin_cache_clearer,
                              EntityLinkRendererInterface $entity_link_renderer,
                              CloudConfigPluginManagerInterface $cloud_config_plugin_manager) {
    parent::__construct($ec2_service,
      $entity_repository,
      $entity_type_bundle_info,
      $time,
      $messenger,
      $entity_type_manager,
      $cacheRender,
      $plugin_cache_clearer,
      $entity_link_renderer,
      $cloud_config_plugin_manager);
    $this->ec2OperationsService = $ec2_operations_service;
  }

  /**
   * {@inheritdoc}
   */
  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('aws_cloud.ec2'),
      $container->get('aws_cloud.ec2_operations'),
      $container->get('entity.repository'),
      $container->get('entity_type.bundle.info'),
      $container->get('datetime.time'),
      $container->get('messenger'),
      $container->get('entity_type.manager'),
      $container->get('cache.render'),
      $container->get('plugin.cache_clearer'),
      $container->get('entity.link_renderer'),
      $container->get('plugin.manager.cloud_config_plugin'),
    );
  }

But now I get the following error. How else can I modify the code?

Sorry, I forgot to add the use statement. I'll check the behavior as soon as I add it.

Fatal error: Could not check compatibility between Drupal\aws_cloud\Form\Ec2\InstanceStartForm::create(Drupal\aws_cloud\Form\Ec2\ContainerInterface $container) and Drupal\aws_cloud\Form\Ec2\AwsDeleteForm::create(Symfony\Component\DependencyInjection\ContainerInterface $container), because class Drupal\aws_cloud\Form\Ec2\ContainerInterface is not available in /var/www/html/web/modules/contrib/cloud/modules/cloud_service_providers/aws_cloud/src/Form/Ec2/InstanceStartForm.php on line 86

@yas

Since the OpenStackInstanceDeleteForm inherits from InstanceDeleteForm, when we extended the constructor of InstanceDeleteForm, we found that OpenStackInstanceDeleteForm was also affected.

If you call the constructor of AwsDeleteUpdateEntityForm in the constructor of OpenStackInstanceDeleteForm, there is no need to increase the arguments of the constructor of OpenStackInstanceDeleteForm. However, in that case, OpenStackInstanceTest will give an error.

There was 1 error:

1) Drupal\Tests\openstack\Functional\OpenStack\OpenStackInstanceTest::testInstance
Exception: Error: Call to a member function deleteInstance() on null
Drupal\aws_cloud\Form\Ec2\InstanceDeleteForm->submitForm()() (Line: 132)

Therefore, we increased the arguments in the constructor of OpenStackInstanceDeleteForm to make it pass all the tests. Please check it.

@yas

I reformatted the code as instructed.

However, for the logger method of Ec2OperationsService, I changed return $this->getLogger($channel); to return CloudContentEntityTrait::logger($channel);. The following error occurred when I changed it.

Error: Drupal\cloud\Traits\CloudContentEntityTrait::logger() 内 Using $this when not in object context (/var/www/html/web/modules/contrib/cloud/src/Traits/CloudContentEntityTrait.php の 518 行目)
#0 /var/www/html/web/modules/contrib/cloud/modules/cloud_service_providers/aws_cloud/src/Service/Ec2/Ec2OperationsService.php(185): Drupal\cloud\Traits\CloudContentEntityTrait::logger('aws_cloud')

The following error also occurred when I changed return $this->getLogger($channel); to return CloudContentEntityTrait::getLogger($channel);.

Error: Drupal\aws_cloud\Service\Ec2\Ec2OperationsService->logger() 内 Call to protected method Drupal\cloud\Traits\CloudContentEntityTrait::getLogger() from context 'Drupal\aws_cloud\Service\Ec2\Ec2OperationsService' (/var/www/html/web/modules/contrib/cloud/modules/cloud_service_providers/aws_cloud/src/Service/Ec2/Ec2OperationsService.php の 185 行目)
#0 /var/www/html/web/modules/contrib/cloud/src/Traits/CloudContentEntityTrait.php(375): Drupal\aws_cloud\Service\Ec2\Ec2OperationsService->logger('aws_cloud')

For this reason, I''ll leave return $this->getLogger($channel); here.

@yas

I reformatted the source code and fix source code comments. Please check it.

@yas

> We don't have to add HTTP response code 200 here like 500 below?

• In the comment of your proposed correction, it says // Add 200 explicitly here?
• The default status code in JsonResponse is HTTP 200 OK.

Based on these two points, I guessed that you wanted me to omit the ", 200" and made the correction.
However, if it is more appropriate not to omit it, I will revise the relevant part.

> Where did you try to use this Ec2OperationsService::logger() method? I couldn't find any place.

I checked the source code again. Since Ec2OperationsService contains use CloudContentEntityTrait, we can already use

Ec2OperationsService::logger</code.

Also, <code>Ec2OperationsService

inherits from CloudServiceBase, which contains use CloudContentEntityTrait. Therefore, it is possible to remove use CloudContentEntityTrait; from Ec2OperationsService.

Note: Ec2OperationsService::logger was created because EntityDeleteFormTrait required the implementation of protected function logger($channel). For the above reasons, we found that omitting it works fine.

==========

The code with the above two fixes passed CI/CD (commit hash: 50b4a76b).

However, when I applied the latest changes to origin/4.x (commit hash: b899f653) and rebased it, it did not pass CI/CD (commit hash: 495931d1).

The erro occurs in OpenStackInstanceTest, and the following error message is output.

There was 1 error:

1) Drupal\Tests\openstack\Functional\OpenStack\OpenStackInstanceTest::testInstance
Behat\Mink\Exception\ResponseTextException: The text "The Instance instance-entity #1 - 2021/08/25 18:45:32 - kfZyXBUx has been deleted." was not found anywhere in the text of the current page.

/var/www/html/vendor/behat/mink/src/WebAssert.php:785
/var/www/html/vendor/behat/mink/src/WebAssert.php:262
/var/www/html/core/tests/Drupal/Tests/WebAssert.php:898
/var/www/html/modules/contrib/cloud/modules/cloud_service_providers/openstack/tests/src/Functional/OpenStack/OpenStackInstanceTest.php:254
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:722

ERRORS!
Tests: 5, Assertions: 480, Errors: 1.

@baldwinlouie

/* Ec2OperationsService#startInstance() */

$result = $this->ec2Service->startInstances([
  'InstanceIds' => [
    $entity->getInstanceId(),
  ],
]);

if (empty($result)) {
  // Should we add a $this->processOperationErrorStatus()?
  return FALSE;
}

Regarding this part, the code before the fix (InstanceStartForm#submitForm() in 4.x branch) also did not include $this->processOperationErrorStatus().

/* InstanceStartForm#submitForm() */

$params = [
  'InstanceIds' => [
    $entity->getInstanceId(),
  ],
];

$this->ec2Service->setCloudContext($entity->getCloudContext());
$result = $this->ec2Service->startInstances($params);

if (empty($result)) {
  // Here
  $form_state->setRedirect("view.{$entity->getEntityTypeId()}.list", ['cloud_context' => $entity->getCloudContext()]);
  return;
}

Therefore, the code in the Issue (Ec2OperationsService#startInstance() in the Issue branch) also did not include $this->processOperationErrorStatus().

@xiaohua-guan

Thank you for your advice. I have removed the unnecessary commas from the source code.

I think I may have found the cause of the error.

In OpenStackInstanceDeleteForm#submitForm(), there is a process such as "Overwrite $this->ec2Service, and if its type is OpenStackEc2Service, then execute parent::submitForm() (InstanceDeleteForm::submitForm()) and exit.

  public function submitForm(array &$form, FormStateInterface $form_state): void {

    $entity = $this->entity;
    // Switch OpenStack EC2 or Rest service based on $entity->getCloudContext().
    $this->ec2Service = $this->openStackServiceFactory->get($entity->getCloudContext());

    if ($this->ec2Service instanceof OpenStackEc2Service) {
      parent::submitForm($form, $form_state);    // Here
      return;
    }

In this case, in the original InstanceDeleteForm, the specification was to use the overwritten $this->ec2Service as is. However, in the InstanceDeleteForm in this issue, the $this->ec2Service held by Ec2OperationsService will be used to delete the instance.

Therefore, it is necessary to prepare a class (e.g. OpenStackOperationsService) that inherits Ec2OperationsServiceInterface as well as Ec2OperationsService, and DI it to OpenStackInstanceDeleteForm.

Note: This problem with parent::submitForm() also exists in OpenStackInstanceStartForm, OpenStackInstanceStopForm, and OpenStackInstanceRebootForm.

@yas

I have created Ec2OperationsService for OpenStack, which can be called by DI from OpenStackInstanceXxxForm.
This allows us to pass all CI/CD test. Please check this commit.

@yas

By inheriting from Ec2OperationsService, the amount of source code has been reduced.
Please check this commit.

@yas @baldwinlouie

Thank you for pointing out by your comments. I fixed the relevant part of the source code. Please check it.

@yas

I rebased from the latest 4.x branch and also checked that it passed CI/CD. Please check it.