more PR feedback

elastic · Sep 26, 2024 · 1e5fda8 · 1e5fda8
1 parent 59c211c
commit 1e5fda8
Show file tree

Hide file tree

Showing 12 changed files with 69 additions and 64 deletions.
diff --git a/x-pack/plugins/observability_solution/slo/server/services/create_slo.test.ts b/x-pack/plugins/observability_solution/slo/server/services/create_slo.test.ts
@@ -65,7 +65,7 @@ describe('CreateSLO', () => {
 
       const response = await createSLO.execute(sloParams);
 
-      expect(mockRepository.save).toHaveBeenCalledWith(
+      expect(mockRepository.create).toHaveBeenCalledWith(
         expect.objectContaining({
           ...sloParams,
           id: 'unique-id',
@@ -80,8 +80,7 @@ describe('CreateSLO', () => {
           version: 2,
           createdAt: expect.any(Date),
           updatedAt: expect.any(Date),
-        }),
-        { existingSavedObjectId: undefined }
+        })
       );
 
       expect(mockTransformManager.install).toHaveBeenCalled();
@@ -106,7 +105,7 @@ describe('CreateSLO', () => {
 
       await createSLO.execute(sloParams);
 
-      expect(mockRepository.save).toHaveBeenCalledWith(
+      expect(mockRepository.create).toHaveBeenCalledWith(
         expect.objectContaining({
           ...sloParams,
           id: expect.any(String),
@@ -120,8 +119,7 @@ describe('CreateSLO', () => {
           enabled: true,
           createdAt: expect.any(Date),
           updatedAt: expect.any(Date),
-        }),
-        { existingSavedObjectId: undefined }
+        })
       );
     });
 
@@ -139,7 +137,7 @@ describe('CreateSLO', () => {
 
       await createSLO.execute(sloParams);
 
-      expect(mockRepository.save).toHaveBeenCalledWith(
+      expect(mockRepository.create).toHaveBeenCalledWith(
         expect.objectContaining({
           ...sloParams,
           id: expect.any(String),
@@ -153,8 +151,7 @@ describe('CreateSLO', () => {
           enabled: true,
           createdAt: expect.any(Date),
           updatedAt: expect.any(Date),
-        }),
-        { existingSavedObjectId: undefined }
+        })
       );
     });
   });

diff --git a/x-pack/plugins/observability_solution/slo/server/services/create_slo.ts b/x-pack/plugins/observability_solution/slo/server/services/create_slo.ts
@@ -23,7 +23,7 @@ import { getSLOPipelineTemplate } from '../assets/ingest_templates/slo_pipeline_
 import { getSLOSummaryPipelineTemplate } from '../assets/ingest_templates/slo_summary_pipeline_template';
 import { Duration, DurationUnit, SLODefinition } from '../domain/models';
 import { validateSLO } from '../domain/services';
-import { SecurityException } from '../errors';
+import { SecurityException, SLOIdConflict } from '../errors';
 import { retryTransientEsErrors } from '../utils/retry';
 import { SLORepository } from './slo_repository';
 import { createTempSummaryDocument } from './summary_transform_generator/helpers/create_temp_summary';
@@ -48,12 +48,13 @@ export class CreateSLO {
 
     const rollbackOperations = [];
 
-    const existingSavedObjectId = await this.repository.getExistingSavedObjectId(slo, {
-      throwOnConflict: true,
-    });
+    const sloAlreadyExists = await this.repository.checkIfSLOExists(slo);
 
-    // can be done async as we are not using the result
-    void this.repository.save(slo, { existingSavedObjectId });
+    if (sloAlreadyExists) {
+      throw new SLOIdConflict(`SLO [${slo.id}] already exists`);
+    }
+
+    const createPromise = this.repository.create(slo);
 
     rollbackOperations.push(() => this.repository.deleteById(slo.id, true));
 
@@ -84,6 +85,7 @@ export class CreateSLO {
       rollbackOperations.push(() => this.deleteTempSummaryDocument(slo));
 
       await Promise.all([
+        createPromise,
         sloPipelinePromise,
         rollupTransformPromise,
         summaryPipelinePromise,

diff --git a/x-pack/plugins/observability_solution/slo/server/services/manage_slo.test.ts b/x-pack/plugins/observability_solution/slo/server/services/manage_slo.test.ts
@@ -38,7 +38,7 @@ describe('ManageSLO', () => {
 
       expect(mockTransformManager.start).not.toHaveBeenCalled();
       expect(mockSummaryTransformManager.start).not.toHaveBeenCalled();
-      expect(mockRepository.save).not.toHaveBeenCalled();
+      expect(mockRepository.create).not.toHaveBeenCalled();
     });
 
     it('enables the slo when disabled', async () => {
@@ -49,7 +49,9 @@ describe('ManageSLO', () => {
 
       expect(mockTransformManager.start).toMatchSnapshot();
       expect(mockSummaryTransformManager.start).toMatchSnapshot();
-      expect(mockRepository.save).toHaveBeenCalledWith(expect.objectContaining({ enabled: true }));
+      expect(mockRepository.update).toHaveBeenCalledWith(
+        expect.objectContaining({ enabled: true })
+      );
     });
   });
 
@@ -62,7 +64,7 @@ describe('ManageSLO', () => {
 
       expect(mockTransformManager.stop).not.toHaveBeenCalled();
       expect(mockSummaryTransformManager.stop).not.toHaveBeenCalled();
-      expect(mockRepository.save).not.toHaveBeenCalled();
+      expect(mockRepository.update).not.toHaveBeenCalled();
     });
 
     it('disables the slo when enabled', async () => {
@@ -73,7 +75,9 @@ describe('ManageSLO', () => {
 
       expect(mockTransformManager.stop).toMatchSnapshot();
       expect(mockSummaryTransformManager.stop).toMatchSnapshot();
-      expect(mockRepository.save).toHaveBeenCalledWith(expect.objectContaining({ enabled: false }));
+      expect(mockRepository.update).toHaveBeenCalledWith(
+        expect.objectContaining({ enabled: false })
+      );
     });
   });
 });
diff --git a/x-pack/plugins/observability_solution/slo/server/services/manage_slo.ts b/x-pack/plugins/observability_solution/slo/server/services/manage_slo.ts
@@ -26,7 +26,7 @@ export class ManageSLO {
     await this.transformManager.start(getSLOTransformId(slo.id, slo.revision));
     slo.enabled = true;
     slo.updatedAt = new Date();
-    await this.repository.save(slo);
+    await this.repository.update(slo);
   }
 
   async disable(sloId: string) {
@@ -39,6 +39,6 @@ export class ManageSLO {
     await this.transformManager.stop(getSLOTransformId(slo.id, slo.revision));
     slo.enabled = false;
     slo.updatedAt = new Date();
-    await this.repository.save(slo);
+    await this.repository.update(slo);
   }
 }
diff --git a/x-pack/plugins/observability_solution/slo/server/services/mocks/index.ts b/x-pack/plugins/observability_solution/slo/server/services/mocks/index.ts
@@ -42,12 +42,13 @@ const createSummaryTransformManagerMock = (): jest.Mocked<TransformManager> => {
 
 const createSLORepositoryMock = (): jest.Mocked<SLORepository> => {
   return {
-    save: jest.fn(),
+    create: jest.fn(),
+    update: jest.fn(),
     findById: jest.fn(),
     findAllByIds: jest.fn(),
     deleteById: jest.fn(),
     search: jest.fn(),
-    getExistingSavedObjectId: jest.fn(),
+    checkIfSLOExists: jest.fn(),
   };
 };
 

diff --git a/x-pack/plugins/observability_solution/slo/server/services/reset_slo.test.ts b/x-pack/plugins/observability_solution/slo/server/services/reset_slo.test.ts
@@ -63,7 +63,7 @@ describe('ResetSLO', () => {
   it('resets all associated resources', async () => {
     const slo = createSLO({ id: 'irrelevant', version: 1 });
     mockRepository.findById.mockResolvedValueOnce(slo);
-    mockRepository.save.mockImplementation((v) => Promise.resolve(v));
+    mockRepository.update.mockImplementation((v) => Promise.resolve(v));
 
     await resetSLO.execute(slo.id);
 
@@ -87,7 +87,7 @@ describe('ResetSLO', () => {
 
     expect(mockEsClient.index).toMatchSnapshot();
 
-    expect(mockRepository.save).toHaveBeenCalledWith({
+    expect(mockRepository.update).toHaveBeenCalledWith({
       ...slo,
       version: SLO_MODEL_VERSION,
       updatedAt: expect.anything(),

diff --git a/x-pack/plugins/observability_solution/slo/server/services/reset_slo.ts b/x-pack/plugins/observability_solution/slo/server/services/reset_slo.ts
@@ -104,7 +104,7 @@ export class ResetSLO {
       throw err;
     }
 
-    const updatedSlo = await this.repository.save({
+    const updatedSlo = await this.repository.update({
       ...slo,
       version: SLO_MODEL_VERSION,
       updatedAt: new Date(),

diff --git a/x-pack/plugins/observability_solution/slo/server/services/slo_repository.test.ts b/x-pack/plugins/observability_solution/slo/server/services/slo_repository.test.ts
@@ -11,7 +11,7 @@ import { MockedLogger } from '@kbn/logging-mocks';
 import { sloDefinitionSchema } from '@kbn/slo-schema';
 import { SLO_MODEL_VERSION } from '../../common/constants';
 import { SLODefinition, StoredSLODefinition } from '../domain/models';
-import { SLOIdConflict, SLONotFound } from '../errors';
+import { SLONotFound } from '../errors';
 import { SO_SLO_TYPE } from '../saved_objects';
 import { aStoredSLO, createAPMTransactionDurationIndicator, createSLO } from './fixtures/slo';
 import { KibanaSavedObjectsSLORepository } from './slo_repository';
@@ -82,13 +82,13 @@ describe('KibanaSavedObjectsSLORepository', () => {
   });
 
   describe('saving an SLO', () => {
-    it('finding existing id for slo', async () => {
+    it('checking existing id for slo', async () => {
       const slo = createSLO({ id: 'my-id' });
       soClientMock.find.mockResolvedValueOnce(soFindResponse([]));
       soClientMock.create.mockResolvedValueOnce(aStoredSLO(slo));
       const repository = new KibanaSavedObjectsSLORepository(soClientMock, loggerMock);
 
-      await repository.getExistingSavedObjectId(slo);
+      await repository.checkIfSLOExists(slo);
 
       expect(soClientMock.find).toHaveBeenCalledWith({
         type: SO_SLO_TYPE,
@@ -103,27 +103,21 @@ describe('KibanaSavedObjectsSLORepository', () => {
       soClientMock.create.mockResolvedValueOnce(aStoredSLO(slo));
       const repository = new KibanaSavedObjectsSLORepository(soClientMock, loggerMock);
 
-      const savedSLO = await repository.save(slo);
+      const savedSLO = await repository.create(slo);
 
       expect(savedSLO).toEqual(slo);
       expect(soClientMock.create).toHaveBeenCalledWith(
         SO_SLO_TYPE,
-        sloDefinitionSchema.encode(slo),
-        {
-          id: undefined,
-          overwrite: true,
-        }
+        sloDefinitionSchema.encode(slo)
       );
     });
 
-    it('throws when the SLO id already exists and "throwOnConflict" is true', async () => {
+    it('checks when the SLO id already exists', async () => {
       const slo = createSLO({ id: 'my-id' });
       soClientMock.find.mockResolvedValueOnce(soFindResponse([slo]));
       const repository = new KibanaSavedObjectsSLORepository(soClientMock, loggerMock);
 
-      await expect(
-        repository.getExistingSavedObjectId(slo, { throwOnConflict: true })
-      ).rejects.toThrowError(new SLOIdConflict(`SLO [my-id] already exists`));
+      await expect(await repository.checkIfSLOExists(slo)).toEqual(true);
       expect(soClientMock.find).toHaveBeenCalledWith({
         type: SO_SLO_TYPE,
         perPage: 0,
@@ -137,7 +131,7 @@ describe('KibanaSavedObjectsSLORepository', () => {
       soClientMock.create.mockResolvedValueOnce(aStoredSLO(slo));
       const repository = new KibanaSavedObjectsSLORepository(soClientMock, loggerMock);
 
-      const savedSLO = await repository.save(slo, { existingSavedObjectId: 'my-id' });
+      const savedSLO = await repository.update(slo);
 
       expect(savedSLO).toEqual(slo);
 

diff --git a/x-pack/plugins/observability_solution/slo/server/services/slo_repository.ts b/x-pack/plugins/observability_solution/slo/server/services/slo_repository.ts
@@ -11,15 +11,13 @@ import { ALL_VALUE, Paginated, Pagination, sloDefinitionSchema } from '@kbn/slo-
 import { isLeft } from 'fp-ts/lib/Either';
 import { SLO_MODEL_VERSION } from '../../common/constants';
 import { SLODefinition, StoredSLODefinition } from '../domain/models';
-import { SLOIdConflict, SLONotFound } from '../errors';
+import { SLONotFound } from '../errors';
 import { SO_SLO_TYPE } from '../saved_objects';
 
 export interface SLORepository {
-  getExistingSavedObjectId(
-    slo: SLODefinition,
-    options?: { throwOnConflict: boolean }
-  ): Promise<string | undefined>;
-  save(slo: SLODefinition, options?: { existingSavedObjectId?: string }): Promise<SLODefinition>;
+  checkIfSLOExists(slo: SLODefinition): Promise<boolean>;
+  create(slo: SLODefinition): Promise<SLODefinition>;
+  update(slo: SLODefinition, options?: { existingSavedObjectId?: string }): Promise<SLODefinition>;
   findAllByIds(ids: string[]): Promise<SLODefinition[]>;
   findById(id: string): Promise<SLODefinition>;
   deleteById(id: string, ignoreNotFound?: boolean): Promise<void>;
@@ -33,26 +31,35 @@ export interface SLORepository {
 export class KibanaSavedObjectsSLORepository implements SLORepository {
   constructor(private soClient: SavedObjectsClientContract, private logger: Logger) {}
 
-  async getExistingSavedObjectId(slo: SLODefinition, options = { throwOnConflict: false }) {
+  async checkIfSLOExists(slo: SLODefinition) {
     const findResponse = await this.soClient.find<StoredSLODefinition>({
       type: SO_SLO_TYPE,
       perPage: 0,
       filter: `slo.attributes.id:(${slo.id})`,
     });
+
+    return findResponse.total > 0;
+  }
+
+  async create(slo: SLODefinition): Promise<SLODefinition> {
+    await this.soClient.create<StoredSLODefinition>(SO_SLO_TYPE, toStoredSLO(slo));
+    return slo;
+  }
+
+  async update(slo: SLODefinition): Promise<SLODefinition> {
+    let existingSavedObjectId: string | undefined;
+
+    const findResponse = await this.soClient.find<StoredSLODefinition>({
+      type: SO_SLO_TYPE,
+      perPage: 1,
+      filter: `slo.attributes.id:(${slo.id})`,
+    });
     if (findResponse.total === 1) {
-      if (options.throwOnConflict) {
-        throw new SLOIdConflict(`SLO [${slo.id}] already exists`);
-      }
-      return findResponse.saved_objects[0].id;
+      existingSavedObjectId = findResponse.saved_objects[0].id;
     }
-  }
 
-  async save(
-    slo: SLODefinition,
-    options?: { existingSavedObjectId: string }
-  ): Promise<SLODefinition> {
     await this.soClient.create<StoredSLODefinition>(SO_SLO_TYPE, toStoredSLO(slo), {
-      id: options?.existingSavedObjectId,
+      id: existingSavedObjectId,
       overwrite: true,
     });
 

diff --git a/x-pack/plugins/observability_solution/slo/server/services/update_slo.test.ts b/x-pack/plugins/observability_solution/slo/server/services/update_slo.test.ts
@@ -204,7 +204,7 @@ describe('UpdateSLO', () => {
       await updateSLO.execute(slo.id, { settings: newSettings });
 
       expectDeletionOfOriginalSLOResources(slo);
-      expect(mockRepository.save).toHaveBeenCalledWith(
+      expect(mockRepository.update).toHaveBeenCalledWith(
         expect.objectContaining({
           ...slo,
           settings: newSettings,
@@ -316,7 +316,7 @@ describe('UpdateSLO', () => {
         updateSLO.execute(originalSlo.id, { indicator: newIndicator })
       ).rejects.toThrowError('Transform install error');
 
-      expect(mockRepository.save).toHaveBeenCalledWith(originalSlo);
+      expect(mockRepository.update).toHaveBeenCalledWith(originalSlo);
       expect(
         mockScopedClusterClient.asSecondaryAuthUser.ingest.deletePipeline
       ).toHaveBeenCalledTimes(1); // for the sli only
@@ -343,7 +343,7 @@ describe('UpdateSLO', () => {
         updateSLO.execute(originalSlo.id, { indicator: newIndicator })
       ).rejects.toThrowError('summary transform start error');
 
-      expect(mockRepository.save).toHaveBeenCalledWith(originalSlo);
+      expect(mockRepository.update).toHaveBeenCalledWith(originalSlo);
       expect(mockSummaryTransformManager.uninstall).toHaveBeenCalled();
       expect(mockScopedClusterClient.asSecondaryAuthUser.ingest.deletePipeline).toHaveBeenCalled();
       expect(mockTransformManager.stop).toHaveBeenCalled();

diff --git a/x-pack/plugins/observability_solution/slo/server/services/update_slo.ts b/x-pack/plugins/observability_solution/slo/server/services/update_slo.ts
@@ -70,8 +70,8 @@ export class UpdateSLO {
 
     const rollbackOperations = [];
 
-    await this.repository.save(updatedSlo);
-    rollbackOperations.push(() => this.repository.save(originalSlo));
+    await this.repository.update(updatedSlo);
+    rollbackOperations.push(() => this.repository.update(originalSlo));
 
     if (!requireRevisionBump) {
       // At this point, we still need to update the sli and summary pipeline to include the changes (id and revision in the rollup index) and (name, desc, tags, ...) in the summary index

diff --git a/x-pack/test/api_integration/apis/slos/update_slo.ts b/x-pack/test/api_integration/apis/slos/update_slo.ts
@@ -14,7 +14,7 @@ import { loadTestData } from './helper/load_test_data';
 import { sloData } from './fixtures/create_slo';
 
 export default function ({ getService }: FtrProviderContext) {
-  describe('Update SLOs', function () {
+  describe('UpdateSLOs', function () {
     this.tags('skipCloud');
 
     const supertestAPI = getService('supertest');
@@ -36,7 +36,7 @@ export default function ({ getService }: FtrProviderContext) {
     });
 
     afterEach(async () => {
-      await slo.deleteAllSLOs();
+      // await slo.deleteAllSLOs();
     });
 
     after(async () => {