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 index c75bdc2cabac3..7c89379816c9d 100644 --- 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 index f24feda8d2916..d85d97c87613e 100644 --- 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 index bace47b69ff4b..d0fd587f6ed0b 100644 --- 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 index 84e8e3798598b..65f59832b57a6 100644 --- 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 index fad5f3adb5090..dc458fcdb813e 100644 --- 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 => { const createSLORepositoryMock = (): jest.Mocked => { 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 index efbf3eedb52e1..4e66d992b46cd 100644 --- 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 index f69651ff2ad8a..634f02c8f6f90 100644 --- 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 index 753d17a683c99..243b2b5e9958b 100644 --- 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 index e2b39c2001820..74e1cea36611e 100644 --- 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; - save(slo: SLODefinition, options?: { existingSavedObjectId?: string }): Promise; + checkIfSLOExists(slo: SLODefinition): Promise; + create(slo: SLODefinition): Promise; + update(slo: SLODefinition, options?: { existingSavedObjectId?: string }): Promise; findAllByIds(ids: string[]): Promise; findById(id: string): Promise; deleteById(id: string, ignoreNotFound?: boolean): Promise; @@ -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({ type: SO_SLO_TYPE, perPage: 0, filter: `slo.attributes.id:(${slo.id})`, }); + + return findResponse.total > 0; + } + + async create(slo: SLODefinition): Promise { + await this.soClient.create(SO_SLO_TYPE, toStoredSLO(slo)); + return slo; + } + + async update(slo: SLODefinition): Promise { + let existingSavedObjectId: string | undefined; + + const findResponse = await this.soClient.find({ + 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 { await this.soClient.create(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 index 37855bd4d8fa4..dccfe5f97d633 100644 --- 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 index 0f1967800d1df..9418bfb1ea91a 100644 --- 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 index 7bf6967bd26a3..e3c428e89db51 100644 --- 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 () => {