From 0de101acddd2c4c332d03b40aabe5fc7ec5611f5 Mon Sep 17 00:00:00 2001 From: Saurabh Bhagwan Sathe Date: Thu, 4 Jul 2024 15:52:01 +0530 Subject: [PATCH] INFRA-966 | Saurabh | Kept the existing hard delete flow but added entry in manifest_audit (#1049) * INFRA-966 | Saurabh | Kept the existing hard delete flow but added entry in manifest_audit * INFRA-966 | Saurabh | Added test for createAuditFunction * INFRA-966 | Saurabh | in log added user email * INFRA-966 | Saurabh | build issue --- .../infra/portal/domain/manifest/Manifest.java | 2 -- .../portal/repository/ManifestRepository.java | 13 +------------ .../service/manifest/ManifestService.java | 15 +++++++++++---- .../V1.86__Dropping_is_delete_column.sql | 14 ++++++++++++++ .../service/manifest/ManifestServiceTest.java | 17 ++++++++++++----- .../infra/portal/service/manifest/User.java | 4 ++++ 6 files changed, 42 insertions(+), 23 deletions(-) create mode 100644 src/main/resources/db/migration/V1.86__Dropping_is_delete_column.sql create mode 100644 src/test/java/com/navi/infra/portal/service/manifest/User.java diff --git a/src/main/java/com/navi/infra/portal/domain/manifest/Manifest.java b/src/main/java/com/navi/infra/portal/domain/manifest/Manifest.java index dfbdd8e1..6518476b 100644 --- a/src/main/java/com/navi/infra/portal/domain/manifest/Manifest.java +++ b/src/main/java/com/navi/infra/portal/domain/manifest/Manifest.java @@ -69,8 +69,6 @@ public class Manifest extends JsonEntity { @Column private String environment; - @Column(name = "is_deleted", nullable = false) - private Boolean isDeleted = false; @Type(type = "jsonb") @Column(columnDefinition = "jsonb") diff --git a/src/main/java/com/navi/infra/portal/repository/ManifestRepository.java b/src/main/java/com/navi/infra/portal/repository/ManifestRepository.java index f669fdca..b17c12ab 100644 --- a/src/main/java/com/navi/infra/portal/repository/ManifestRepository.java +++ b/src/main/java/com/navi/infra/portal/repository/ManifestRepository.java @@ -10,32 +10,21 @@ import org.springframework.stereotype.Repository; @Repository public interface ManifestRepository extends JpaRepository { - @Query("SELECT m FROM Manifest m WHERE m.name = :name AND m.isDeleted = false") List findByName(String name); - @Query("SELECT m FROM Manifest m WHERE m.id = :id AND m.isDeleted = false") - Optional findById(Long id); - - @Query("SELECT m FROM Manifest m WHERE m.name = :name " - + "AND m.environment = :environment AND m.isDeleted = false") Optional findByNameAndEnvironment(String name, String environment); - @Query("SELECT m FROM Manifest m WHERE m.environment = :environment AND m.isDeleted = false") List findIdAndNameAndDataByEnvironment(String environment); - @Query("SELECT m FROM Manifest m WHERE m.isDeleted = false") List findAll(); - @Query("SELECT m FROM Manifest m WHERE m.isDeleted = false") List findIdAndNameAndEnvironmentAndDataBy(); - @Query("SELECT m FROM Manifest m WHERE m.id = :id AND m.isDeleted = false") Optional findManifestNameById(Long id); - @Query(value = "select m.id from manifest m " + "join deployment d on m.id = d.manifest_id " + "join load_balancer lb on d.id = lb.deployment_id " - + "where lb.data ->> 'endpoint' = :endpoint and m.is_deleted = false", nativeQuery = true) + + "where lb.data ->> 'endpoint' = :endpoint ", nativeQuery = true) Optional findIdByEndpoint(String endpoint); } diff --git a/src/main/java/com/navi/infra/portal/service/manifest/ManifestService.java b/src/main/java/com/navi/infra/portal/service/manifest/ManifestService.java index fde4ec50..1354d639 100644 --- a/src/main/java/com/navi/infra/portal/service/manifest/ManifestService.java +++ b/src/main/java/com/navi/infra/portal/service/manifest/ManifestService.java @@ -509,6 +509,14 @@ public class ManifestService { } } + private void auditDeletedManifest(Manifest manifest) { + var lastManifestAudit = manifestAuditService.getLatestByManifestId(manifest.getId()) + .orElseThrow(() -> new IllegalStateException( + "No audit found for manifest: " + manifest.getId())); + manifestAuditService.createAudit(manifest, lastManifestAudit.getSecretVersion().toString(), + lastManifestAudit.getSuperSecretVersion().toString()); + } + public List delete(Long id, Boolean deleteManifest) { Manifest manifest = map(manifestRepository.findById(id)); @@ -530,10 +538,9 @@ public class ManifestService { } if (deleteManifest) { result.add(manifest.fullName()); - Manifest oldManifest = manifestDeepCopy(manifest); - manifest.setIsDeleted(true); - logManifestDifference(manifest, oldManifest); - auditAndSaveIfManifestChanged(manifest, oldManifest); + log.info("User {} Deleting manifest Object - {}",userService.getCurrentUsername(), manifest.fullName()); + auditDeletedManifest(manifest); + manifestRepository.deleteById(id); } else { saveManifestWithoutSecrets(manifest); } diff --git a/src/main/resources/db/migration/V1.86__Dropping_is_delete_column.sql b/src/main/resources/db/migration/V1.86__Dropping_is_delete_column.sql new file mode 100644 index 00000000..92d54fd9 --- /dev/null +++ b/src/main/resources/db/migration/V1.86__Dropping_is_delete_column.sql @@ -0,0 +1,14 @@ +ALTER TABLE manifest +DROP COLUMN is_deleted; + +-- Drop the existing foreign key constraint if it exists +ALTER TABLE manifest_audit +DROP CONSTRAINT IF EXISTS manifest_audit_manifest_id_fk; + +-- Add the new foreign key constraint with ON DELETE SET NULL +ALTER TABLE manifest_audit + ADD CONSTRAINT manifest_audit_manifest_id_fk FOREIGN KEY (manifest_id) + REFERENCES manifest(id) ON DELETE SET NULL; + + + diff --git a/src/test/java/com/navi/infra/portal/service/manifest/ManifestServiceTest.java b/src/test/java/com/navi/infra/portal/service/manifest/ManifestServiceTest.java index 12a575de..0cdf2aa0 100644 --- a/src/test/java/com/navi/infra/portal/service/manifest/ManifestServiceTest.java +++ b/src/test/java/com/navi/infra/portal/service/manifest/ManifestServiceTest.java @@ -60,6 +60,7 @@ import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; import org.mockito.ArgumentMatcher; import org.mockito.Mock; import org.mockito.junit.jupiter.MockitoExtension; @@ -192,7 +193,7 @@ class ManifestServiceTest { + "\"infraVertical\":\"lending\",\"type\":\"deployment\"}"; String manifestResponseString = "{\"version\":null,\"id\":null," + "\"name\":\"test\"," - + "\"environment\":\"dev\",\"isDeleted\":false,\"dynamicConfiguration\":null," + + "\"environment\":\"dev\",\"dynamicConfiguration\":null," + "\"metadata\":{\"repo\":\"test\",\"language\":\"Java\",\"dataSensitivity\":\"PII_SPI\"," + "\"logCriticality\":\"AccessLogs\",\"disasterRecovery\":\"True\"}," + "\"extraResources\":null,\"notification\":null," @@ -254,7 +255,7 @@ class ManifestServiceTest { + " \"cluster\": \"spike.np.navi-tech.in\"," + "\"infraVertical\": \"lending\"}"; String manifestResponseString = "{" + "\"version\":null," + "\"id\":null," + "\"name\":\"test\"," - + "\"environment\":\"dev\"," + "\"isDeleted\":false," + + "\"environment\":\"dev\"," + "\"dynamicConfiguration\":null," + "\"metadata\":{\"repo\":\"test\",\"language\":\"Java\",\"dataSensitivity\":\"PII_SPI\",\"logCriticality\":\"AccessLogs\",\"disasterRecovery\":\"True\"}," + "\"extraResources\":null," + "\"notification\":null,\"deployment\":" @@ -269,6 +270,7 @@ class ManifestServiceTest { + "\"type\":\"deployment\"}"; Manifest manifest = stringToManifest(manifestRequestString); + when(userService.getCurrentUsername()).thenReturn(currentUser); when(repo.save(any(Manifest.class))).thenReturn(manifest); final var request = FindGroupNameRequest.builder() @@ -302,7 +304,7 @@ class ManifestServiceTest { String testManifest = "{\n" + " \"version\": 1,\n" + " \"id\": 2,\n" + " \"name\": \"sa-infra-dev-db\",\n" - + " \"environment\": \"dev\",\n" + "\"isDeleted\":false," + + " \"environment\": \"dev\",\n" + " \"extraResources\": {\n" + " \"version\": 1,\n" + " \"id\": 2\n" + " },\n" + " \"infraVertical\": \"sa\",\n" + " \"environmentVariables\": [\n" + " {\n" @@ -331,8 +333,12 @@ class ManifestServiceTest { List result = new ArrayList(); result.add(customManifest.fullName()); assertEquals(result, service.delete(2L, true)); + ArgumentCaptor manifestCaptor = ArgumentCaptor.forClass(Manifest.class); + verify(manifestAuditService).createAudit(manifestCaptor.capture(), eq("1"), eq("1")); + assertEquals(customManifest.getId(), manifestCaptor.getValue().getId()); } + @Test @DisplayName("should not update manifest if no changes") void shouldNotUpdateManifestIfNoChanges() throws JsonProcessingException { @@ -342,7 +348,7 @@ class ManifestServiceTest { mapDiffUtil, changeRequestUtils, authorizationFilter, null, null); String manifestRequestString = "{\"id\":1,\"name\":\"test\",\"environment\":\"dev\",\"metadata\":{\"repo\":\"test\",\"language\":\"Java\",\"dataSensitivity\":\"PII_SPI\",\"logCriticality\":\"AccessLogs\",\"disasterRecovery\":\"True\"},\"team\":{\"name\":\"Infra\"},\"infraVertical\":\"lending\",\"cluster\":\"spike.np.navi-tech.in\",\"type\":\"deployment\"}"; - String manifestResponseString = "{\"version\":null,\"id\":1,\"name\":\"test\",\"environment\":\"dev\",\"isDeleted\":false,\"dynamicConfiguration\":null,\"metadata\":{\"repo\":\"test\",\"language\":\"Java\",\"dataSensitivity\":\"PII_SPI\",\"logCriticality\":\"AccessLogs\",\"disasterRecovery\":\"True\"},\"extraResources\":null,\"notification\":null,\"deployment\":null,\"infraVertical\":\"lending\",\"environmentVariables\":[],\"cluster\":\"spike.np.navi-tech.in\",\"team\":{\"name\":\"Infra\"},\"type\":\"deployment\"}"; + String manifestResponseString = "{\"version\":null,\"id\":1,\"name\":\"test\",\"environment\":\"dev\",\"dynamicConfiguration\":null,\"metadata\":{\"repo\":\"test\",\"language\":\"Java\",\"dataSensitivity\":\"PII_SPI\",\"logCriticality\":\"AccessLogs\",\"disasterRecovery\":\"True\"},\"extraResources\":null,\"notification\":null,\"deployment\":null,\"infraVertical\":\"lending\",\"environmentVariables\":[],\"cluster\":\"spike.np.navi-tech.in\",\"team\":{\"name\":\"Infra\"},\"type\":\"deployment\"}"; Manifest manifest = stringToManifest(manifestRequestString); @@ -356,6 +362,7 @@ class ManifestServiceTest { assertEquals(manifestResponseString, manifestResponse.getManifest().convertToString()); } + @Test @DisplayName("should update manifest") void shouldUpdateManifest() throws JsonProcessingException { @@ -365,7 +372,7 @@ class ManifestServiceTest { mapDiffUtil, changeRequestUtils, authorizationFilter, null, null); String manifestRequestString = "{\"id\":1,\"name\":\"test\",\"environment\":\"dev\",\"metadata\":{\"repo\":\"test\",\"language\":\"Java\",\"dataSensitivity\":\"PII_SPI\",\"logCriticality\":\"AccessLogs\",\"disasterRecovery\":\"True\"},\"team\":{\"name\":\"Infra\"},\"infraVertical\":\"lending\",\"cluster\":\"spike.np.navi-tech.in\",\"type\":\"deployment\"}"; - String manifestResponseString = "{\"version\":null,\"id\":1,\"name\":\"test\",\"environment\":\"dev\",\"isDeleted\":false,\"dynamicConfiguration\":null,\"metadata\":{\"repo\":\"test\",\"language\":\"Java\",\"dataSensitivity\":\"PII_SPI\",\"logCriticality\":\"AccessLogs\",\"disasterRecovery\":\"True\"},\"extraResources\":null,\"notification\":null,\"deployment\":null,\"infraVertical\":\"lending\",\"environmentVariables\":[],\"cluster\":\"spike.np.navi-tech.in\",\"team\":{\"name\":\"Infra\"},\"type\":\"deployment\"}"; + String manifestResponseString = "{\"version\":null,\"id\":1,\"name\":\"test\",\"environment\":\"dev\",\"dynamicConfiguration\":null,\"metadata\":{\"repo\":\"test\",\"language\":\"Java\",\"dataSensitivity\":\"PII_SPI\",\"logCriticality\":\"AccessLogs\",\"disasterRecovery\":\"True\"},\"extraResources\":null,\"notification\":null,\"deployment\":null,\"infraVertical\":\"lending\",\"environmentVariables\":[],\"cluster\":\"spike.np.navi-tech.in\",\"team\":{\"name\":\"Infra\"},\"type\":\"deployment\"}"; String currentManifestString = "{\"id\":1,\"name\":\"test\",\"environment\":\"dev\",\"infraVertical\":\"lending\",\"team\":{\"name\":\"CBP\"}}"; Manifest manifest = stringToManifest(manifestRequestString); Manifest currentManifest = stringToManifest(currentManifestString); diff --git a/src/test/java/com/navi/infra/portal/service/manifest/User.java b/src/test/java/com/navi/infra/portal/service/manifest/User.java new file mode 100644 index 00000000..6ab5d5ee --- /dev/null +++ b/src/test/java/com/navi/infra/portal/service/manifest/User.java @@ -0,0 +1,4 @@ +package com.navi.infra.portal.service.manifest; + +public class User { +}