From 9fd499fa98ca1c643b45d5a7d96aeb6fd09a999c Mon Sep 17 00:00:00 2001 From: Duarte Meneses Date: Fri, 13 Aug 2021 16:37:51 -0500 Subject: [PATCH] SONAR-15142 Logs for add/update/delete are inconsistent for webhooks and devops --- .../sonar/db/alm/setting/AlmSettingDao.java | 6 +-- .../sonar/db/alm/setting/AlmSettingDto.java | 3 +- .../db/alm/setting/ProjectAlmSettingDao.java | 18 ++------- .../model/DevOpsPlatformSettingNewValue.java | 7 ---- .../sonar/db/audit/model/WebhookNewValue.java | 13 +++---- .../java/org/sonar/db/webhook/WebhookDao.java | 3 +- .../AlmSettingDaoWithPersisterTest.java | 22 ++++++----- ...ProjectAlmSettingDaoWithPersisterTest.java | 16 +------- .../webhook/WebhookDaoWithPersisterTest.java | 37 ++++++++++++++----- 9 files changed, 55 insertions(+), 70 deletions(-) diff --git a/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/AlmSettingDao.java b/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/AlmSettingDao.java index b946a7c8797..35f8ae7fa2e 100644 --- a/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/AlmSettingDao.java +++ b/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/AlmSettingDao.java @@ -58,8 +58,7 @@ public class AlmSettingDao implements Dao { getMapper(dbSession).insert(almSettingDto); if (auditPersister != null) { - auditPersister.addDevOpsPlatformSetting(dbSession, new DevOpsPlatformSettingNewValue(almSettingDto.getUuid(), - almSettingDto.getKey())); + auditPersister.addDevOpsPlatformSetting(dbSession, new DevOpsPlatformSettingNewValue(almSettingDto)); } } @@ -83,8 +82,7 @@ public class AlmSettingDao implements Dao { getMapper(dbSession).deleteByKey(almSettingDto.getKey()); if (auditPersister != null) { - auditPersister.deleteDevOpsPlatformSetting(dbSession, new DevOpsPlatformSettingNewValue(almSettingDto.getUuid(), - almSettingDto.getKey())); + auditPersister.deleteDevOpsPlatformSetting(dbSession, new DevOpsPlatformSettingNewValue(almSettingDto.getUuid(), almSettingDto.getKey())); } } diff --git a/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/AlmSettingDto.java b/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/AlmSettingDto.java index 4f56d4b3989..94a74ff68ed 100644 --- a/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/AlmSettingDto.java +++ b/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/AlmSettingDto.java @@ -89,8 +89,9 @@ public class AlmSettingDto { return uuid; } - void setUuid(String uuid) { + AlmSettingDto setUuid(String uuid) { this.uuid = uuid; + return this; } public String getKey() { diff --git a/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/ProjectAlmSettingDao.java b/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/ProjectAlmSettingDao.java index caa11bea5b5..6333987cbbd 100644 --- a/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/ProjectAlmSettingDao.java +++ b/server/sonar-db-dao/src/main/java/org/sonar/db/alm/setting/ProjectAlmSettingDao.java @@ -48,8 +48,7 @@ public class ProjectAlmSettingDao implements Dao { this.auditPersister = auditPersister; } - public void insertOrUpdate(DbSession dbSession, ProjectAlmSettingDto projectAlmSettingDto, - String key, String projectName) { + public void insertOrUpdate(DbSession dbSession, ProjectAlmSettingDto projectAlmSettingDto, String key, String projectName) { String uuid = uuidFactory.create(); long now = system2.now(); ProjectAlmSettingMapper mapper = getMapper(dbSession); @@ -64,11 +63,11 @@ public class ProjectAlmSettingDao implements Dao { projectAlmSettingDto.setUpdatedAt(now); if (auditPersister != null) { + DevOpsPlatformSettingNewValue value = new DevOpsPlatformSettingNewValue(projectAlmSettingDto, key, projectName); if (isUpdate) { - auditPersister.updateDevOpsPlatformSetting(dbSession, new DevOpsPlatformSettingNewValue(projectAlmSettingDto, key, projectName)); + auditPersister.updateDevOpsPlatformSetting(dbSession, value); } else { - auditPersister.addDevOpsPlatformSetting(dbSession, new DevOpsPlatformSettingNewValue(projectAlmSettingDto.getAlmSettingUuid(), - projectAlmSettingDto.getProjectUuid(), key, projectName)); + auditPersister.addDevOpsPlatformSetting(dbSession, value); } } } @@ -82,16 +81,7 @@ public class ProjectAlmSettingDao implements Dao { } public void deleteByAlmSetting(DbSession dbSession, AlmSettingDto almSetting) { - deleteByAlmSetting(dbSession, almSetting, false); - } - - public void deleteByAlmSetting(DbSession dbSession, AlmSettingDto almSetting, boolean track) { getMapper(dbSession).deleteByAlmSettingUuid(almSetting.getUuid()); - - if (track && auditPersister != null) { - auditPersister.deleteDevOpsPlatformSetting(dbSession, new DevOpsPlatformSettingNewValue(almSetting.getUuid(), - almSetting.getKey())); - } } public int countByAlmSetting(DbSession dbSession, AlmSettingDto almSetting) { diff --git a/server/sonar-db-dao/src/main/java/org/sonar/db/audit/model/DevOpsPlatformSettingNewValue.java b/server/sonar-db-dao/src/main/java/org/sonar/db/audit/model/DevOpsPlatformSettingNewValue.java index bfef1eac78b..71876aa96c0 100644 --- a/server/sonar-db-dao/src/main/java/org/sonar/db/audit/model/DevOpsPlatformSettingNewValue.java +++ b/server/sonar-db-dao/src/main/java/org/sonar/db/audit/model/DevOpsPlatformSettingNewValue.java @@ -88,13 +88,6 @@ public class DevOpsPlatformSettingNewValue implements NewValue { this.isMonorepo = dto.getMonorepo(); } - public DevOpsPlatformSettingNewValue(String devOpsPlatformSettingUuid, String projectUuid, String key, String projectName) { - this.devOpsPlatformSettingUuid = devOpsPlatformSettingUuid; - this.key = key; - this.projectUuid = projectUuid; - this.projectName = projectName; - } - public DevOpsPlatformSettingNewValue(ProjectDto projectDto) { this.projectUuid = projectDto.getUuid(); this.projectName = projectDto.getName(); diff --git a/server/sonar-db-dao/src/main/java/org/sonar/db/audit/model/WebhookNewValue.java b/server/sonar-db-dao/src/main/java/org/sonar/db/audit/model/WebhookNewValue.java index cdf2be4463c..c5a0c49043d 100644 --- a/server/sonar-db-dao/src/main/java/org/sonar/db/audit/model/WebhookNewValue.java +++ b/server/sonar-db-dao/src/main/java/org/sonar/db/audit/model/WebhookNewValue.java @@ -41,20 +41,19 @@ public class WebhookNewValue implements NewValue { private String projectName; public WebhookNewValue(WebhookDto dto, @Nullable String projectName) { - this.webhookUuid = dto.getUuid(); - this.name = dto.getName(); - this.url = dto.getUrl(); - this.projectUuid = dto.getProjectUuid(); - this.projectName = projectName; + this(dto.getUuid(), dto.getName(), dto.getProjectUuid(), projectName, dto.getUrl()); } public WebhookNewValue(String webhookUuid, String webhookName) { + this.webhookUuid = webhookUuid; this.name = webhookName; } - public WebhookNewValue(String webhookUuid, String webhookName, @Nullable String projectUuid, @Nullable String projectName) { - this(webhookUuid, webhookName); + public WebhookNewValue(String webhookUuid, String webhookName, @Nullable String projectUuid, @Nullable String projectName, @Nullable String url) { + this.webhookUuid = webhookUuid; + this.name = webhookName; + this.url = url; this.projectUuid = projectUuid; this.projectName = projectName; } diff --git a/server/sonar-db-dao/src/main/java/org/sonar/db/webhook/WebhookDao.java b/server/sonar-db-dao/src/main/java/org/sonar/db/webhook/WebhookDao.java index 246c3e89f91..7d906aec2fa 100644 --- a/server/sonar-db-dao/src/main/java/org/sonar/db/webhook/WebhookDao.java +++ b/server/sonar-db-dao/src/main/java/org/sonar/db/webhook/WebhookDao.java @@ -60,8 +60,7 @@ public class WebhookDao implements Dao { mapper(dbSession).insert(dto.setCreatedAt(system2.now()).setUpdatedAt(system2.now())); if (auditPersister != null) { - auditPersister.addWebhook(dbSession, new WebhookNewValue(dto.getUuid(), dto.getName(), - dto.getProjectUuid(), projectName)); + auditPersister.addWebhook(dbSession, new WebhookNewValue(dto, projectName)); } } diff --git a/server/sonar-db-dao/src/test/java/org/sonar/db/alm/setting/AlmSettingDaoWithPersisterTest.java b/server/sonar-db-dao/src/test/java/org/sonar/db/alm/setting/AlmSettingDaoWithPersisterTest.java index fbde7b47b9b..9d734652e28 100644 --- a/server/sonar-db-dao/src/test/java/org/sonar/db/alm/setting/AlmSettingDaoWithPersisterTest.java +++ b/server/sonar-db-dao/src/test/java/org/sonar/db/alm/setting/AlmSettingDaoWithPersisterTest.java @@ -55,34 +55,36 @@ public class AlmSettingDaoWithPersisterTest { @Test public void insertAndUpdateArePersisted() { when(uuidFactory.create()).thenReturn(A_UUID); - AlmSettingDto almSettingDto = newGithubAlmSettingDto(); + AlmSettingDto almSettingDto = newGithubAlmSettingDto() + .setKey("key") + .setAppId("id1") + .setClientId("cid1") + .setUrl("url"); underTest.insert(dbSession, almSettingDto); verify(auditPersister).addDevOpsPlatformSetting(eq(dbSession), newValueCaptor.capture()); DevOpsPlatformSettingNewValue newValue = newValueCaptor.getValue(); assertThat(newValue) - .extracting(DevOpsPlatformSettingNewValue::getDevOpsPlatformSettingUuid, DevOpsPlatformSettingNewValue::getKey) + .extracting("devOpsPlatformSettingUuid", "key") .containsExactly(almSettingDto.getUuid(), almSettingDto.getKey()); - assertThat(newValue.toString()).doesNotContain("url"); + assertThat(newValue).hasToString("{\"devOpsPlatformSettingUuid\": \"1\", \"key\": \"key\", \"devOpsPlatformName\": \"id1\", \"url\": \"url\", \"appId\": \"id1\", \"clientId\": \"cid1\" }"); almSettingDto.setPrivateKey("updated private key"); almSettingDto.setAppId("updated app id"); almSettingDto.setUrl("updated url"); almSettingDto.setPersonalAccessToken("updated pat"); almSettingDto.setKey("updated key"); - system2.setNow(NOW + 1); underTest.update(dbSession, almSettingDto); verify(auditPersister).updateDevOpsPlatformSetting(eq(dbSession), newValueCaptor.capture()); newValue = newValueCaptor.getValue(); assertThat(newValue) - .extracting(DevOpsPlatformSettingNewValue::getDevOpsPlatformSettingUuid, DevOpsPlatformSettingNewValue::getKey, - DevOpsPlatformSettingNewValue::getAppId, DevOpsPlatformSettingNewValue::getDevOpsPlatformName, - DevOpsPlatformSettingNewValue::getUrl, DevOpsPlatformSettingNewValue::getClientId) - .containsExactly(almSettingDto.getUuid(), almSettingDto.getKey(), almSettingDto.getAppId(), almSettingDto.getAppId(), - almSettingDto.getUrl(), almSettingDto.getClientId()); + .extracting("devOpsPlatformSettingUuid", "key","appId", "devOpsPlatformName", "url", "clientId") + .containsExactly(almSettingDto.getUuid(), almSettingDto.getKey(), almSettingDto.getAppId(), almSettingDto.getAppId(), almSettingDto.getUrl(), almSettingDto.getClientId()); + assertThat(newValue).hasToString("{\"devOpsPlatformSettingUuid\": \"1\", \"key\": \"updated key\", \"devOpsPlatformName\": \"updated app id\", " + + "\"url\": \"updated url\", \"appId\": \"updated app id\", \"clientId\": \"cid1\" }"); } @Test @@ -96,7 +98,7 @@ public class AlmSettingDaoWithPersisterTest { verify(auditPersister).deleteDevOpsPlatformSetting(eq(dbSession), newValueCaptor.capture()); DevOpsPlatformSettingNewValue newValue = newValueCaptor.getValue(); assertThat(newValue) - .extracting(DevOpsPlatformSettingNewValue::getDevOpsPlatformSettingUuid, DevOpsPlatformSettingNewValue::getKey) + .extracting("devOpsPlatformSettingUuid", "key") .containsExactly(almSettingDto.getUuid(), almSettingDto.getKey()); assertThat(newValue.toString()).doesNotContain("url"); } diff --git a/server/sonar-db-dao/src/test/java/org/sonar/db/alm/setting/ProjectAlmSettingDaoWithPersisterTest.java b/server/sonar-db-dao/src/test/java/org/sonar/db/alm/setting/ProjectAlmSettingDaoWithPersisterTest.java index 25d3ce811ea..1d33930bd2e 100644 --- a/server/sonar-db-dao/src/test/java/org/sonar/db/alm/setting/ProjectAlmSettingDaoWithPersisterTest.java +++ b/server/sonar-db-dao/src/test/java/org/sonar/db/alm/setting/ProjectAlmSettingDaoWithPersisterTest.java @@ -109,24 +109,10 @@ public class ProjectAlmSettingDaoWithPersisterTest { } @Test - public void deleteByAlmSettingWhenNotTracked() { + public void deleteByAlmSettingNotTracked() { AlmSettingDto githubAlmSetting = db.almSettings().insertGitHubAlmSetting(); underTest.deleteByAlmSetting(dbSession, githubAlmSetting); verifyNoInteractions(auditPersister); } - - @Test - public void deleteByAlmSettingWhenTracked() { - AlmSettingDto githubAlmSetting = db.almSettings().insertGitHubAlmSetting(); - underTest.deleteByAlmSetting(dbSession, githubAlmSetting, true); - - verify(auditPersister).deleteDevOpsPlatformSetting(eq(dbSession), newValueCaptor.capture()); - DevOpsPlatformSettingNewValue newValue = newValueCaptor.getValue(); - assertThat(newValue) - .extracting(DevOpsPlatformSettingNewValue::getDevOpsPlatformSettingUuid, DevOpsPlatformSettingNewValue::getKey) - .containsExactly(githubAlmSetting.getUuid(), githubAlmSetting.getKey()); - assertThat(newValue.toString()).doesNotContain("url"); - } - } diff --git a/server/sonar-db-dao/src/test/java/org/sonar/db/webhook/WebhookDaoWithPersisterTest.java b/server/sonar-db-dao/src/test/java/org/sonar/db/webhook/WebhookDaoWithPersisterTest.java index edb6f407368..7dd4c49e674 100644 --- a/server/sonar-db-dao/src/test/java/org/sonar/db/webhook/WebhookDaoWithPersisterTest.java +++ b/server/sonar-db-dao/src/test/java/org/sonar/db/webhook/WebhookDaoWithPersisterTest.java @@ -37,7 +37,7 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.verify; public class WebhookDaoWithPersisterTest { - private AuditPersister auditPersister = mock(AuditPersister.class); + private final AuditPersister auditPersister = mock(AuditPersister.class); @Rule public final DbTester dbTester = DbTester.create(System2.INSTANCE, auditPersister); @@ -65,7 +65,7 @@ public class WebhookDaoWithPersisterTest { assertThat(newValue) .extracting(WebhookNewValue::getWebhookUuid, WebhookNewValue::getName) .containsExactly(dto.getUuid(), dto.getName()); - assertThat(newValue.toString()).doesNotContain("url"); + assertThat(newValue).hasToString("{\"webhookUuid\": \"UUID_1\", \"name\": \"NAME_1\", \"url\": \"URL_1\" }"); } @Test @@ -82,14 +82,13 @@ public class WebhookDaoWithPersisterTest { verify(auditPersister).addWebhook(eq(dbSession), newValueCaptor.capture()); WebhookNewValue newValue = newValueCaptor.getValue(); assertThat(newValue) - .extracting(WebhookNewValue::getWebhookUuid, WebhookNewValue::getName, WebhookNewValue::getProjectUuid, - WebhookNewValue::getProjectName) + .extracting(WebhookNewValue::getWebhookUuid, WebhookNewValue::getName, WebhookNewValue::getProjectUuid, WebhookNewValue::getProjectName) .containsExactly(dto.getUuid(), dto.getName(), dto.getProjectUuid(), "project_name"); - assertThat(newValue.toString()).doesNotContain("url"); + assertThat(newValue).hasToString("{\"webhookUuid\": \"UUID_1\", \"name\": \"NAME_1\", \"url\": \"URL_1\", \"projectUuid\": \"UUID_2\", \"projectName\": \"project_name\" }"); } @Test - public void updateWebhookIsPersisted() { + public void updateGlobalWebhookIsPersisted() { WebhookDto dto = webhookDbTester.insertGlobalWebhook(); dto = dto .setName("a-fancy-webhook") @@ -103,12 +102,30 @@ public class WebhookDaoWithPersisterTest { assertThat(newValue) .extracting(WebhookNewValue::getWebhookUuid, WebhookNewValue::getName, WebhookNewValue::getUrl) .containsExactly(dto.getUuid(), dto.getName(), dto.getUrl()); - assertThat(newValue.toString()).doesNotContain("projectUuid"); + assertThat(newValue).hasToString("{\"webhookUuid\": \"" + dto.getUuid() + "\", \"name\": \"a-fancy-webhook\", \"url\": \"http://www.fancy-webhook.io\" }"); + } + + @Test + public void updateProjectWebhookIsPersisted() { + WebhookDto dto = webhookDbTester.insertGlobalWebhook(); + dto = dto + .setName("a-fancy-webhook") + .setUrl("http://www.fancy-webhook.io") + .setSecret(null); + + underTest.update(dbSession, dto, "project"); + + verify(auditPersister).updateWebhook(eq(dbSession), newValueCaptor.capture()); + WebhookNewValue newValue = newValueCaptor.getValue(); + assertThat(newValue) + .extracting(WebhookNewValue::getWebhookUuid, WebhookNewValue::getName, WebhookNewValue::getUrl) + .containsExactly(dto.getUuid(), dto.getName(), dto.getUrl()); + assertThat(newValue).hasToString("{\"webhookUuid\": \"" + dto.getUuid() +"\", \"name\": \"a-fancy-webhook\", \"url\": \"http://www.fancy-webhook.io\", \"projectName\": \"project\" }"); } @Test public void deleteProjectWebhooksIsPersisted() { - ProjectDto projectDto = componentDbTester.insertPrivateProjectDto(); + ProjectDto projectDto = componentDbTester.insertPrivateProjectDto(p -> p.setUuid("puuid").setName("pname")); webhookDbTester.insertWebhook(projectDto); underTest.deleteByProject(dbSession, projectDto); @@ -118,7 +135,7 @@ public class WebhookDaoWithPersisterTest { assertThat(newValue) .extracting(WebhookNewValue::getProjectUuid, WebhookNewValue::getProjectName) .containsExactly(projectDto.getUuid(), projectDto.getName()); - assertThat(newValue.toString()).doesNotContain("webhookUuid"); + assertThat(newValue).hasToString("{\"projectUuid\": \"puuid\", \"projectName\": \"pname\" }"); } @Test @@ -132,6 +149,6 @@ public class WebhookDaoWithPersisterTest { assertThat(newValue) .extracting(WebhookNewValue::getWebhookUuid, WebhookNewValue::getName) .containsExactly(dto.getUuid(), dto.getName()); - assertThat(newValue.toString()).doesNotContain("url"); + assertThat(newValue).hasToString("{\"webhookUuid\": \"" + dto.getUuid() + "\", \"name\": \"" + dto.getName() + "\" }"); } } -- 2.39.5