Chnages Per code Review

microsoft · Jan 31, 2019 · 6256ec2 · 6256ec2
1 parent b295bc8
commit 6256ec2
Show file tree

Hide file tree

Showing 10 changed files with 41 additions and 43 deletions.
diff --git a/src/Microsoft.Health.ControlPlane.Core.UnitTests/Features/Rbac/RbacServiceTests.cs b/src/Microsoft.Health.ControlPlane.Core.UnitTests/Features/Rbac/RbacServiceTests.cs
@@ -36,7 +36,7 @@ public RbacServiceTests()
             _role = GetRole("clinician", resourcePermissions);
             _controlPlaneDataStore.GetRoleAsync(_role.Name, Arg.Any<CancellationToken>()).Returns(_role);
             _controlPlaneDataStore.UpsertRoleAsync(_role, Arg.Any<string>(), Arg.Any<CancellationToken>()).Returns(new UpsertResponse<Role>(_role, UpsertOutcome.Updated, "testEtag"));
-            _controlPlaneDataStore.DeleteRoleAsync(_role.Name, Arg.Any<CancellationToken>());
+            _controlPlaneDataStore.DeleteRoleAsync(_role.Name, Arg.Any<string>(), Arg.Any<CancellationToken>());
 
             _rbacService = new RbacService(_controlPlaneDataStore);
         }

diff --git a/src/Microsoft.Health.ControlPlane.Core/Features/Persistence/IControlPlaneDataStore.cs b/src/Microsoft.Health.ControlPlane.Core/Features/Persistence/IControlPlaneDataStore.cs
@@ -24,8 +24,8 @@ public interface IControlPlaneDataStore
 
         Task<UpsertResponse<Role>> UpsertRoleAsync(Role role, string eTag, CancellationToken cancellationToken);
 
-        Task<IEnumerable<Role>> GetRoleAllAsync(CancellationToken cancellationToken);
+        Task<IEnumerable<Role>> GetAllRoleAsync(CancellationToken cancellationToken);
 
-        Task DeleteRoleAsync(string name, CancellationToken cancellationToken);
+        Task DeleteRoleAsync(string name, string eTag, CancellationToken cancellationToken);
     }
 }
diff --git a/src/Microsoft.Health.ControlPlane.Core/Features/Rbac/IRbacService.cs b/src/Microsoft.Health.ControlPlane.Core/Features/Rbac/IRbacService.cs
@@ -26,6 +26,6 @@ public interface IRbacService
 
         Task<IEnumerable<Role>> GetRoleForAllAsync(CancellationToken cancellationToken);
 
-        Task DeleteRoleAsync(string name, CancellationToken cancellationToken);
+        Task DeleteRoleAsync(string name, string eTag, CancellationToken cancellationToken);
     }
 }
diff --git a/src/Microsoft.Health.ControlPlane.Core/Features/Rbac/RbacService.cs b/src/Microsoft.Health.ControlPlane.Core/Features/Rbac/RbacService.cs
@@ -26,7 +26,8 @@ public RbacService(IControlPlaneDataStore controlPlaneDataStore)
 
         public async Task DeleteIdentityProviderAsync(string name, string eTag, CancellationToken cancellationToken)
         {
-             await _controlPlaneDataStore.GetIdentityProviderAsync(name, cancellationToken);
+            EnsureArg.IsNotNullOrWhiteSpace(name, nameof(name));
+            await _controlPlaneDataStore.DeleteIdentityProviderAsync(name, eTag, cancellationToken);
         }
 
         public async Task<Role> GetRoleAsync(string name, CancellationToken cancellationToken)
@@ -64,7 +65,7 @@ public async Task<UpsertResponse<IdentityProvider>> UpsertIdentityProviderAsync(
 
         public async Task<IEnumerable<Role>> GetRoleForAllAsync(CancellationToken cancellationToken)
         {
-            return await _controlPlaneDataStore.GetRoleAllAsync(cancellationToken);
+            return await _controlPlaneDataStore.GetAllRoleAsync(cancellationToken);
         }
 
         public async Task<UpsertResponse<Role>> UpsertRoleAsync(Role role, string eTag, CancellationToken cancellationToken)
@@ -76,17 +77,17 @@ public async Task<UpsertResponse<Role>> UpsertRoleAsync(Role role, string eTag,
             if (!Validator.TryValidateObject(role, new ValidationContext(role), validationResults))
             {
                 throw new InvalidDefinitionException(
-                    Resources.IdentityProviderDefinitionIsInvalid,
+                    Resources.RoleDefinitionIsInvalid,
                     validationResults);
             }
 
             return await _controlPlaneDataStore.UpsertRoleAsync(role, eTag, cancellationToken);
         }
 
-        public async System.Threading.Tasks.Task DeleteRoleAsync(string name, CancellationToken cancellationToken)
+        public async System.Threading.Tasks.Task DeleteRoleAsync(string name, string eTag, CancellationToken cancellationToken)
         {
             EnsureArg.IsNotNullOrWhiteSpace(name, nameof(name));
-            await _controlPlaneDataStore.DeleteRoleAsync(name, cancellationToken);
+            await _controlPlaneDataStore.DeleteRoleAsync(name, eTag, cancellationToken);
         }
     }
 }
diff --git a/src/Microsoft.Health.ControlPlane.Core/Features/Rbac/Role.cs b/src/Microsoft.Health.ControlPlane.Core/Features/Rbac/Role.cs
@@ -41,7 +41,7 @@ internal Role(string name, IList<ResourcePermission> resourcePermissions, string
         [JsonProperty("etag")]
         public virtual string VersionTag { get;  set; }
 
-        [JsonProperty("resourcepermissions")]
+        [JsonProperty("resourcePermissions")]
         public virtual IList<ResourcePermission> ResourcePermissions { get;  set; } = new List<ResourcePermission>();
 
         public virtual IEnumerable<ValidationResult> Validate(ValidationContext validationContext)

diff --git a/src/Microsoft.Health.ControlPlane.Core/Resources.Designer.cs b/src/Microsoft.Health.ControlPlane.Core/Resources.Designer.cs
diff --git a/src/Microsoft.Health.ControlPlane.Core/Resources.resx b/src/Microsoft.Health.ControlPlane.Core/Resources.resx
@@ -145,7 +145,7 @@
     <value>Role must have one or more ResourcePermissions.</value>
   </data>
   <data name="RoleNameEmpty" xml:space="preserve">
-    <value>Role Name '{0}' cannot be empty</value>
+    <value>Role Name is null or empty</value>
   </data>
   <data name="RoleNotFound" xml:space="preserve">
     <value>Role '{0}' was not found.</value>
@@ -156,4 +156,7 @@
   <data name="InvalidDocumentTypeForDelete" xml:space="preserve">
     <value>Document type '{0}' for delete is invalid.</value>
   </data>
-</root>
+  <data name="RoleDefinitionIsInvalid" xml:space="preserve">
+    <value>Role definition Is invalid.</value>
+  </data>
+</root>
diff --git a/src/Microsoft.Health.ControlPlane.CosmosDb/Features/Storage/ControlPlaneDataStore.cs b/src/Microsoft.Health.ControlPlane.CosmosDb/Features/Storage/ControlPlaneDataStore.cs
@@ -104,41 +104,18 @@ public async Task<Role> GetRoleAsync(string name, CancellationToken cancellation
             return role.ToRole();
         }
 
-        public async Task DeleteRoleAsync(string name, CancellationToken cancellationToken)
+        public async Task DeleteRoleAsync(string name, string eTag, CancellationToken cancellationToken)
         {
             EnsureArg.IsNotNull(name, nameof(name));
-
-            try
-            {
-                StoredProcedureResponse<IList<string>> response = await _retryExceptionPolicyFactory.CreateRetryPolicy().ExecuteAsync(
-                    async () => await _hardDeleteRole.Execute(
-                        _documentClient,
-                        _collectionUri,
-                        name),
-                    cancellationToken);
-
-                _logger.LogDebug($"Hard-deleted {response.Response.Count} documents, which consumed {response.RequestCharge} RUs. The list of hard-deleted documents: {string.Join(", ", response.Response)}.");
-            }
-            catch (DocumentClientException dce)
-            {
-                if (dce.Error?.Message?.Contains(GetValue(HttpStatusCode.RequestEntityTooLarge), StringComparison.Ordinal) == true)
-                {
-                    // TODO: Eventually, we might want to have our own RequestTooLargeException?
-                    throw new Exception();
-                }
-
-                _logger.LogError(dce, "Unhandled Document Client Exception");
-
-                throw;
-            }
+            await DeleteSystemDocumentsByIdAsync<Role>(name, CosmosRole.RolePartition, eTag, cancellationToken);
         }
 
         private static string GetValue(HttpStatusCode type)
         {
             return ((int)type).ToString();
         }
 
-        public async Task<IEnumerable<Role>> GetRoleAllAsync(CancellationToken cancellationToken)
+        public async Task<IEnumerable<Role>> GetAllRoleAsync(CancellationToken cancellationToken)
         {
             var role = await GetSystemDocumentsAsync<CosmosRole>(null, CosmosRole.RolePartition, cancellationToken);
             return role.Select(cr => cr.ToRole());
@@ -322,6 +299,15 @@ private async Task DeleteSystemDocumentsByIdAsync<T>(string id, string partition
                                             eTag),
                                         cancellationToken);
                         break;
+                    case "Role":
+                        response = await _retryExceptionPolicyFactory.CreateRetryPolicy().ExecuteAsync(
+                                  async () => await _hardDeleteRole.Execute(
+                                   _documentClient.Value,
+                                   _collectionUri,
+                                   id,
+                                   eTag),
+                                  cancellationToken);
+                        break;
                     default:
                         throw new InvalidControlPlaneTypeForDeleteException(typeName);
                 }

diff --git a/src/Microsoft.Health.ControlPlane.CosmosDb/Features/Storage/Rbac/CosmosRoleExtensions.cs b/src/Microsoft.Health.ControlPlane.CosmosDb/Features/Storage/Rbac/CosmosRoleExtensions.cs
@@ -14,7 +14,7 @@ public static Role ToRole(this CosmosRole cosmosRole)
             return new Role(
                 cosmosRole.Name,
                 cosmosRole.ResourcePermissions,
-                cosmosRole.ETag.Trim('"'));
+                cosmosRole.ETag);
         }
     }
 }
diff --git a/...alth.ControlPlane.CosmosDb/Features/Storage/StoredProcedures/HardDelete/HardDeleteRole.cs b/...alth.ControlPlane.CosmosDb/Features/Storage/StoredProcedures/HardDelete/HardDeleteRole.cs
@@ -11,19 +11,18 @@
 using Microsoft.Azure.Documents.Client;
 using Microsoft.Health.ControlPlane.CosmosDb.Features.Storage.Rbac;
 using Microsoft.Health.CosmosDb.Features.Storage.StoredProcedures;
-using Microsoft.Health.Extensions.DependencyInjection;
 
 namespace Microsoft.Health.ControlPlane.CosmosDb.Features.Storage.StoredProcedures.HardDelete
 {
     internal class HardDeleteRole : StoredProcedureBase, IControlPlaneStoredProcedure
     {
-        public async Task<StoredProcedureResponse<IList<string>>> Execute(IScoped<IDocumentClient> client, Uri collection,  string id)
+        public async Task<StoredProcedureResponse<IList<string>>> Execute(IDocumentClient client, Uri collection,  string id, string eTag)
         {
             EnsureArg.IsNotNull(client, nameof(client));
             EnsureArg.IsNotNull(collection, nameof(collection));
             EnsureArg.IsNotNull(id, nameof(id));
 
-            return await ExecuteStoredProc<IList<string>>(client.Value, collection, CosmosRole.RolePartition, id);
+            return await ExecuteStoredProc<IList<string>>(client, collection, CosmosRole.RolePartition, id, eTag);
         }
     }
 }