Fix a bug in Ec2Service::updateInstanceEntities()

1. +++ b/modules/cloud_service_providers/aws_cloud/src/Controller/Ec2/ApiController.php
   @@ -163,7 +163,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +    if ($updated !== FALSE) {
   
   @@ -342,7 +330,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +    if ($updated !== FALSE) {
   
   @@ -392,7 +374,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +    if ($updated !== FALSE) {
   
   @@ -508,7 +473,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +    if ($updated !== FALSE) {
   
   @@ -558,7 +517,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +    if ($updated !== FALSE) {
   
   @@ -609,7 +562,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +    if ($updated !== FALSE) {
   
   +++ b/modules/cloud_service_providers/aws_cloud/src/Controller/Vpc/ApiController.php
   @@ -118,7 +115,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +    if ($updated !== FALSE) {
   
   @@ -169,7 +160,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +    if ($updated !== FALSE) {
   
   @@ -220,7 +205,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +    if ($updated !== FALSE) {
   
   +++ b/modules/cloud_service_providers/aws_cloud/src/Service/Ec2/Ec2Service.php
   @@ -3200,4 +3216,96 @@ class Ec2Service extends CloudServiceBase implements Ec2ServiceInterface {
   +      if ($updated !== FALSE) {
   ...
   +        if ($updated !== FALSE) {
   ...
   +      if ($updated !== FALSE) {
   

   Is there any reason why you didn't use $updated === TRUE here?

2. +++ b/modules/cloud_service_providers/aws_cloud/src/Controller/Ec2/ApiController.php
   @@ -177,9 +183,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
      public function updateAllInstanceList() {
   
   @@ -356,29 +350,17 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
      public function updateAllSecurityGroupList() {
   
   @@ -406,9 +394,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
      public function updateAllNetworkInterfaceList() {
   
   @@ -456,44 +448,17 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
      public function updateAllElasticIpList() {
   
   @@ -522,29 +493,17 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
      public function updateAllKeyPairList() {
   
   @@ -572,29 +537,17 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
      public function updateAllVolumeList() {
   
   @@ -623,9 +582,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
      public function updateAllSnapshotList() {
   
   +++ b/modules/cloud_service_providers/aws_cloud/src/Controller/Vpc/ApiController.php
   @@ -132,29 +135,17 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
      public function updateAllVpcList() {
   
   @@ -183,29 +180,17 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
      public function updateAllVpcPeeringConnectionList() {
   
   @@ -234,9 +225,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
      public function updateAllSubnetList() {
   

   Can you refactor to aggregate those updateAll*List() methods to e.g. updateAllResourceList()?

3. +++ b/modules/cloud_service_providers/aws_cloud/src/Service/Ec2/Ec2Service.php
   @@ -3200,4 +3216,96 @@ class Ec2Service extends CloudServiceBase implements Ec2ServiceInterface {
   +  public function updateEntityMessage($entity_type, $cloud_context, $cloud_config, $updated) {
   

   Can you refactor the logic in the method to even simpler? (Reducing or aggregating if-statement)

4. +++ b/src/Entity/CloudConfig.php
   @@ -340,6 +340,29 @@ class CloudConfig extends CloudContentEntityBase implements CloudConfigInterface
   +  public function getResourceLink($plural_label, $entity_type, $cloud_context) {
   

   Do we need the parameter $cloud_context? Because this is CloudConfig entity, so it should hold the $cloud_context inside the instance. (Can we use getCloudContext() in getResourceLink()?)

@jigishaddweb

Thank you for the update.

Can we refactor as follows?

FROM:

+++ b/modules/cloud_service_providers/aws_cloud/src/Service/Ec2/Ec2Service.php
@@ -3200,4 +3074,187 @@ class Ec2Service extends CloudServiceBase implements Ec2ServiceInterface {
+  private function updateEntityMessage($entity_type, $cloud_config, $updated) {
+    $labels = $this->getDisplayLabels($entity_type);
+    $resource_link = $cloud_config->getResourceLink($labels['plural'], $entity_type);
+    $cloud_config_link = $cloud_config->getCloudConfigLink();
+
+    if ($entity_type === 'aws_cloud_elastic_ip') {
+      $network_interface_label = $this->getDisplayLabels('aws_cloud_network_interface');
+      $network_interface_link = Link::fromTextAndUrl(
+        $network_interface_label['plural'],
+        Url::fromRoute(
+          "entity.aws_cloud_network_interface.collection",
+          ['cloud_context' => $cloud_config->getCloudContext()]
+        )
+      )->toString();
+
+      if ($updated === TRUE) {
+        // Also update Network Interfaces.
+        $updated = $this->updateAllNetworkInterfaces();
+        $success_message = $this->t('Updated %resources and %other_resource of %cloud_config cloud service provider.', [
+          '%resources' => $resource_link ?? $labels['plural'],
+          '%other_resource' => $network_interface_link ?? $network_interface_label['plural'],
+          '%cloud_config' => $cloud_config_link ?? $cloud_config->getName(),
+        ]);
+
+        $error_message = $this->t('Unable to update %other_resource while updating %resources of %cloud_config cloud service provider.', [
+          '%resources' => $resource_link ?? $labels['plural'],
+          '%other_resource' => $network_interface_link ?? $network_interface_label['plural'],
+          '%cloud_config' => $cloud_config_link ?? $cloud_config->getName(),
+        ]);
+
+        $updated === TRUE
+          ? $this->messageUser($success_message)
+          : $this->messageUser($error_message, 'error');
+
+        $this->clearCacheValue();
+      }
+      else {
+        $this->messageUser($this->t('Unable to update %resources and %other_resource of %cloud_config cloud service provider.', [
+          '%resources' => $resource_link ?? $labels['plural'],
+          '%other_resource' => $network_interface_link ?? $network_interface_label['plural'],
+          '%cloud_config' => $cloud_config_link ?? $cloud_config->getName(),
+        ]), 'error');
+      }
+    }
+    else {
+      $success_message = $this->t('Updated %resources of %cloud_config cloud service provider.', [
+        '%resources' => $resource_link ?? $labels['plural'],
+        '%cloud_config' => $cloud_config_link ?? $cloud_config->getName(),
+      ]);
+
+      $error_message = $this->t('Unable to update %resources of %cloud_config cloud service provider.', [
+        '%resources' => $resource_link ?? $labels['plural'],
+        '%cloud_config' => $cloud_config_link ?? $cloud_config->getName(),
+      ]);
+
+      $updated === TRUE
+        ? $this->messageUser($success_message)
+        : $this->messageUser($error_message, 'error');
+
+      $this->clearCacheValue();
+    }
+  }

TO:

  private function updateEntityMessage($entity_type, $cloud_config, $updated) {
    $labels = $this->getDisplayLabels($entity_type);
    $resource_link = $cloud_config->getResourceLink($labels['plural'], $entity_type);
    $cloud_config_link = $cloud_config->getCloudConfigLink();

    $success_message = $this->t('Updated %resources of %cloud_config cloud service provider.', [
      '%resources' => $resource_link ?? $labels['plural'],
      '%cloud_config' => $cloud_config_link ?? $cloud_config->getName(),
    ]);

    $error_message = $this->t('Unable to update %resources of %cloud_config cloud service provider.', [
      '%resources' => $resource_link ?? $labels['plural'],
      '%cloud_config' => $cloud_config_link ?? $cloud_config->getName(),
    ]);

    if ($entity_type === 'aws_cloud_elastic_ip') {
      $network_interface_label = $this->getDisplayLabels('aws_cloud_network_interface');
      $network_interface_link = Link::fromTextAndUrl(
        $network_interface_label['plural'],
        Url::fromRoute(
          "entity.aws_cloud_network_interface.collection",
          ['cloud_context' => $cloud_config->getCloudContext()]
        )
      )->toString();

      if ($updated === TRUE) {
        // Also update Network Interfaces.
        $updated = $this->updateAllNetworkInterfaces();
        $success_message = $this->t('Updated %resources and %other_resource of %cloud_config cloud service provider.', [
          '%resources' => $resource_link ?? $labels['plural'],
          '%other_resource' => $network_interface_link ?? $network_interface_label['plural'],
          '%cloud_config' => $cloud_config_link ?? $cloud_config->getName(),
        ]);

        $error_message = $this->t('Unable to update %other_resource while updating %resources of %cloud_config cloud service provider.', [
          '%resources' => $resource_link ?? $labels['plural'],
          '%other_resource' => $network_interface_link ?? $network_interface_label['plural'],
          '%cloud_config' => $cloud_config_link ?? $cloud_config->getName(),
        ]);
      }
      else {
        $error_message = $this->t('Unable to update %resources and %other_resource of %cloud_config cloud service provider.', [
          '%resources' => $resource_link ?? $labels['plural'],
          '%other_resource' => $network_interface_link ?? $network_interface_label['plural'],
          '%cloud_config' => $cloud_config_link ?? $cloud_config->getName(),
        ]);
      }
    }

    $updated === TRUE
    ? $this->messageUser($success_message)
    : $this->messageUser($error_message, 'error');

    $this->clearCacheValue();
  }

@jigishaddweb

Thank you for the update.

Let's reduce the if-statement nest and adding !empty():

FROM:

+++ b/modules/cloud_service_providers/aws_cloud/src/Service/Ec2/Ec2Service.php
@@ -3200,4 +3074,180 @@ class Ec2Service extends CloudServiceBase implements Ec2ServiceInterface {
+      if ($entity_type_name === 'aws_cloud_image') {
+        $cloud_config_entities = $this->entityTypeManager->getStorage('cloud_config')->loadByProperties(
+          ['cloud_context' => [$cloud_context]]
+        );
+
+        $cloud_config = $cloud_config_entities ? reset($cloud_config_entities) : [];
+        $account_id = $this->getAccountId($cloud_config_entities);
+
+        if ($account_id) {
+          $this->setCloudContext($cloud_context);
+          $updated = $this->updateAllImages([
+            'Owners' => [
+              $account_id,
+            ],
+          ], TRUE);
+
+          $this->updateEntityMessage('aws_cloud_image', $cloud_config, $updated);
+        }
+        else {
+          $message = $this->t('AWS User ID is not specified.');
+          $account = $this->currentUser();
+          $account->hasPermission('edit cloud service providers')
+            ? $message = Link::createFromRoute($message, 'entity.cloud_config.edit_form', ['cloud_config' => $cloud_config->id()])->toString()
+            : '';
+          $this->messageUser($message, 'error');
+        }
+      }
+      else {
+        $this->setCloudContext($cloud_context);
+        $updated = $this->$update_method_name();
+        $this->updateEntityMessage($entity_type_name, $cloud_config, $updated);
+      }

TO:

      if ($entity_type_name !== 'aws_cloud_image') {
        $this->setCloudContext($cloud_context);
        $updated = $this->$update_method_name();
        $this->updateEntityMessage($entity_type_name, $cloud_config, $updated);
        continue;
      }

      $cloud_config_entities = $this->entityTypeManager->getStorage('cloud_config')->loadByProperties([
        'cloud_context' => [$cloud_context],
      ]);

      $cloud_config = !empty($cloud_config_entities) ? reset($cloud_config_entities) : [];
      $account_id = $this->getAccountId($cloud_config_entities);

      if (!empty($account_id)) {
        $this->setCloudContext($cloud_context);
        $updated = $this->updateAllImages([
          'Owners' => [
            $account_id,
          ],
        ], TRUE);

        $this->updateEntityMessage('aws_cloud_image', $cloud_config, $updated);
      }
      else {
        $message = $this->t('AWS User ID is not specified.');
        $account = $this->currentUser();
        $account->hasPermission('edit cloud service providers')
          ? $message = Link::createFromRoute($message, 'entity.cloud_config.edit_form', ['cloud_config' => $cloud_config->id()])->toString()
          : '';
        $this->messageUser($message, 'error');
      }

@xiaohua-guan, @baldwinloue

What do you think in the other portion?

@jigishaddweb, Thank you for the updated patch. I have the following comments.

1. +++ b/modules/cloud_service_providers/aws_cloud/src/Controller/Ec2/ApiController.php
   @@ -342,7 +256,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +      Ec2Service::clearCacheValue();
   
   @@ -392,7 +289,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +      Ec2Service::clearCacheValue();
   
   @@ -508,7 +366,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +      Ec2Service::clearCacheValue();
   
   @@ -558,7 +402,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +      Ec2Service::clearCacheValue();
   
   @@ -609,7 +436,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +      Ec2Service::clearCacheValue();
   
   +++ b/modules/cloud_service_providers/aws_cloud/src/Controller/Vpc/ApiController.php
   @@ -118,7 +115,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +      Ec2Service::clearCacheValue();
   
   @@ -169,7 +149,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +      Ec2Service::clearCacheValue();
   
   @@ -220,7 +183,13 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   +      Ec2Service::clearCacheValue();
   
   +++ b/modules/cloud_service_providers/aws_cloud/src/Service/Ec2/Ec2Service.php
   @@ -3200,4 +3074,180 @@ class Ec2Service extends CloudServiceBase implements Ec2ServiceInterface {
   +    $this->clearCacheValue();
   

   clearCacheValue() can potentially be called twice per API operation. Once inside the update*List() and once inside the updateEntityMessage method.

   Please choose one place to call the cache clear.

   I think it should be in the APIController code and not inside updateEntityMessage. updateEntityMessage should only be for setting messages.

2. +++ b/modules/cloud_service_providers/aws_cloud/src/Controller/Ec2/ApiController.php
   @@ -451,47 +347,9 @@ class ApiController extends ControllerBase implements ApiControllerInterface {
   -      $updated = $this->ec2Service->updateAllNetworkInterfaces();
   

   When an Elastic Ip is updated, there is code that updates the NetworkInterfaces as well. This code needs to be moved to updateAllResourceList as well.

3. +++ b/modules/cloud_service_providers/aws_cloud/src/Service/Ec2/Ec2Service.php
   @@ -3200,4 +3074,180 @@ class Ec2Service extends CloudServiceBase implements Ec2ServiceInterface {
   +    return $account_id;
   

   Please initialize $account_id to something. There is a chance $account_id can be returned uninitialized.

4. +++ b/modules/cloud_service_providers/aws_cloud/src/Service/Ec2/Ec2Service.php
   @@ -3200,4 +3074,180 @@ class Ec2Service extends CloudServiceBase implements Ec2ServiceInterface {
   +  public function updateAllResourceList($entity_type_name) {
   

   If this is a public method, please put this in ApiControllerInterface. If it is only meant to be a helper method, please make this private.

   This method should also catch the following exceptions:

      \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException
   
   \Drupal\Component\Plugin\Exception\PluginNotFoundException
   

5. +++ b/modules/cloud_service_providers/aws_cloud/src/Service/Ec2/Ec2Service.php
   @@ -3200,4 +3074,180 @@ class Ec2Service extends CloudServiceBase implements Ec2ServiceInterface {
   +      if ($entity_type_name !== 'aws_cloud_image') {
   +        $this->setCloudContext($cloud_context);
   +        $updated = $this->$update_method_name();
   +        $this->updateEntityMessage($entity_type_name, $cloud_config, $updated);
   +        continue;
   +      }
   +
   +      $cloud_config_entities = $this->entityTypeManager->getStorage('cloud_config')->loadByProperties([
   +        'cloud_context' => [$cloud_context],
   +      ]);
   +
   +      $cloud_config = !empty($cloud_config_entities) ? reset($cloud_config_entities) : [];
   +      $account_id = $this->getAccountId($cloud_config_entities);
   +
   +      if (!empty($account_id)) {
   +        $this->setCloudContext($cloud_context);
   +        $updated = $this->updateAllImages([
   +          'Owners' => [
   +            $account_id,
   +          ],
   +        ], TRUE);
   +
   +        $this->updateEntityMessage('aws_cloud_image', $cloud_config, $updated);
   +      }
   +      else {
   +        $message = $this->t('AWS User ID is not specified.');
   +        $account = $this->currentUser();
   +        $account->hasPermission('edit cloud service providers')
   +          ? $message = Link::createFromRoute($message, 'entity.cloud_config.edit_form', ['cloud_config' => $cloud_config->id()])->toString()
   +          : '';
   +        $this->messageUser($message, 'error');
   +      }
   +    }
   

   Since we have to support a custom case for aws_elastic_ips, how about we use a "switch" statement, where the default case is the code inside if ($entity_type_name !== 'aws_cloud_image)?

   We can then have switch cases for 'aws_cloud_image' and 'aws_cloud_elastic_ips'..etc...

6. +++ b/src/Entity/CloudConfig.php
   @@ -340,6 +340,27 @@ class CloudConfig extends CloudContentEntityBase implements CloudConfigInterface
   +  public function getResourceLink($plural_label, $entity_type) {
   

   If this is a public method, please add a definition in CloudConfigInterface

@jigishaddweb, thank you for the updated patch. Please see my comments below.

1. +++ b/modules/cloud_service_providers/aws_cloud/src/Service/Ec2/Ec2Service.php
   @@ -3200,4 +3074,197 @@ class Ec2Service extends CloudServiceBase implements Ec2ServiceInterface {
   +   * @throws \Drupal\Component\Plugin\Exception\InvalidPluginDefinitionException
   +   * @throws \Drupal\Component\Plugin\Exception\PluginNotFoundException
   

   I think it is better to catch the exceptions in the method and display an error message.

   The methods that call updateallResourceList do not catch the exceptions. Thus, there is a chance we get a White Screen due to a 500 error.

2. +++ b/modules/cloud_service_providers/aws_cloud/src/Service/Ec2/Ec2Service.php
   @@ -3200,4 +3074,197 @@ class Ec2Service extends CloudServiceBase implements Ec2ServiceInterface {
   +            $account = $this->currentUser();
   +            $account->hasPermission('edit cloud service providers')
   +              ? $message = Link::createFromRoute($message, 'entity.cloud_config.edit_form', ['cloud_config' => $cloud_config->id()])->toString()
   +              : '';
   

   I think you can get rid of $account = $this->currentUser() and use

   $this->currentUser()->hasPermission()
   

   Since we are not using $account again.

@jigishaddweb, thank you for the patch. It looks good now!

@baldwinlouie

Thank you for your review. I'll merge the patch to 8.x-1.x, 8.x-2.x and 3.x and close this issue as Fixed.