SALTO-6310: Add configurable validation for deploy requests [WIP]

packages/adapter-components/src/client/polling.ts

@@ -25,7 +25,7 @@ export const executeWithPolling = async <T>(
       const response = await singleClientCall(args)
       return polling.checkStatus(response) ? response : undefined
     } catch (e) {
-      if (e instanceof HTTPError && polling.retryOnStatus?.includes(e.response?.status)) {
+      if (polling.retryOnStatus?.includes(e.response?.status)) {
         return undefined
       }
       throw e

packages/adapter-components/src/definitions/system/deploy/deploy.ts

@@ -10,6 +10,7 @@ import { ArgsWithCustomizer, DefaultWithCustomizations, TransformDefinition } fr
 import { DeployRequestDefinition } from './request'
 import { ChangeIdFunction } from '../../../deployment/grouping'
 import { ChangeAndContext, ChangeAndExtendedContext } from './types'
+import { Response, ResponseValue } from '../../../client'
 
 export type ValueReferenceResolver = (args: { value: Values }) => Values
 
@@ -25,12 +26,22 @@ export type DeployRequestCondition = ArgsWithCustomizer<
   ChangeAndExtendedContext
 >
 
+export type DeployResponseValidator = ArgsWithCustomizer<
+  boolean,
+  {
+    allowedStatusCodes?: number[]
+  },
+  ChangeAndExtendedContext & { response: Response<ResponseValue | ResponseValue[]> }
+>
+
 export type DeployableRequestDefinition<ClientOptions extends string> = {
   // when provided, only changes matching the condition will be used in this request
   condition?: DeployRequestCondition
 
   request: DeployRequestDefinition<ClientOptions>
 
+  validate?: DeployResponseValidator
+
   // define what (if any) part of the response should be copied back to the workspace (via the original change), or be available for subsequent calls within the operation.
   // by default, only values of fields marked as service id are copied
   copyFromResponse?: {

packages/adapter-components/src/deployment/deploy/requester.ts

@@ -16,7 +16,7 @@ import {
   isModificationChange,
   isReferenceExpression,
 } from '@salto-io/adapter-api'
-import { elementExpressionStringifyReplacer, safeJsonStringify } from '@salto-io/adapter-utils'
+import { elementExpressionStringifyReplacer, inspectValue, safeJsonStringify } from '@salto-io/adapter-utils'
 import { logger } from '@salto-io/logging'
 import { collections, promises, values as lowerdashValues } from '@salto-io/lowerdash'
 import { ResponseValue, Response, ClientDataParams, executeWithPolling } from '../../client'
@@ -30,7 +30,11 @@ import {
 import { createValueTransformer } from '../../fetch/utils'
 import { replaceAllArgs } from '../../fetch/request/utils'
 import { TransformDefinition } from '../../definitions/system/shared'
-import { DeployRequestCondition, DeployableRequestDefinition } from '../../definitions/system/deploy/deploy'
+import {
+  DeployRequestCondition,
+  DeployableRequestDefinition,
+  DeployResponseValidator,
+} from '../../definitions/system/deploy/deploy'
 import { ChangeAndExtendedContext, DeployChangeInput } from '../../definitions/system/deploy/types'
 import { ChangeElementResolver } from '../../resolve_utils'
 import { ResolveAdditionalActionType, ResolveClientOptionsType } from '../../definitions/system/api'
@@ -90,6 +94,43 @@ const createCheck = (conditionDef?: DeployRequestCondition): ((args: ChangeAndEx
   }
 }
 
+export type ResponseValidationError = Error & { response: Response<ResponseValue | ResponseValue[]> }
+
+const createResponseValidationError = (
+  message: string,
+  response: Response<ResponseValue | ResponseValue[]>,
+): ResponseValidationError => ({
+  ...new Error(message),
+  response,
+})
+
+const createValidateFunc = ({
+  validate,
+  additionalValidStatuses,
+}: {
+  validate?: DeployResponseValidator
+  // Baseline endpoint status codes that are considered valid, to be used as a fallback when no `validate` is provided.
+  additionalValidStatuses: number[]
+}): ((
+  args: ChangeAndExtendedContext & { response: Response<ResponseValue | ResponseValue[]> },
+) => Promise<boolean>) => {
+  const { custom, allowedStatusCodes } = validate ?? {}
+  if (custom !== undefined) {
+    return async args => custom({ allowedStatusCodes })(args)
+  }
+  return async args => {
+    log.trace('validating response %o', args.response)
+    const {
+      response: { status },
+    } = args
+    if (allowedStatusCodes !== undefined) {
+      log.trace('validating response status %o against allowed status codes %o', status, allowedStatusCodes)
+      return allowedStatusCodes.includes(status)
+    }
+    return (status >= 200 && status < 300) || additionalValidStatuses?.includes(status)
+  }
+}
+
 const extractDataToApply = async ({
   definition,
   changeAndContext,
@@ -236,10 +277,12 @@ export const getRequester = <TOptions extends APIDefinitionsOptions>({
 
   const singleRequest = async ({
     requestDef,
+    validate,
     change,
     ...changeContext
   }: ChangeAndExtendedContext & {
     requestDef: DeployRequestEndpointDefinition<ResolveClientOptionsType<TOptions>>
+    validate?: DeployResponseValidator
   }): Promise<Response<ResponseValue | ResponseValue[]>> => {
     const { merged: mergedRequestDef, clientName } = getMergedRequestDefinition(requestDef)
     const mergedEndpointDef = mergedRequestDef.endpoint
@@ -298,26 +341,28 @@ export const getRequester = <TOptions extends APIDefinitionsOptions>({
       throwOnUnresolvedArgs: true,
     })
     const client = clientDefs[clientName].httpClient
-    const additionalValidStatuses = mergedEndpointDef.additionalValidStatuses ?? []
     const { polling } = mergedEndpointDef
 
+    const additionalValidStatuses = mergedEndpointDef.additionalValidStatuses ?? []
+    const validateFunc = createValidateFunc({ validate, additionalValidStatuses })
+
     const singleClientCall = async (args: ClientDataParams): Promise<Response<ResponseValue | ResponseValue[]>> => {
+      let response: Response<ResponseValue | ResponseValue[]>
       try {
-        return await client[finalEndpointIdentifier.method ?? 'get'](args)
+        response = await client[finalEndpointIdentifier.method ?? 'get'](args)
       } catch (e) {
+        // We leave response status validation to validateFunc
         const status = e.response?.status
-        if (additionalValidStatuses.includes(status)) {
-          log.debug(
-            'Suppressing %d error %o, for path %s in method %s',
-            status,
-            e,
-            finalEndpointIdentifier.path,
-            finalEndpointIdentifier.method,
-          )
-          return { data: {}, status }
-        }
-        throw e
+        response = { data: {}, status }
+      }
+
+      if (!(await validateFunc({ change, ...changeContext, response }))) {
+        throw createResponseValidationError(
+          `Response validation failed for action ${change.action} ${elemID.getFullName()}: ${inspectValue(response)}`,
+          response,
+        )
       }
+      return response
     }
 
     const updatedArgs: ClientDataParams = {
@@ -349,7 +394,7 @@ export const getRequester = <TOptions extends APIDefinitionsOptions>({
     }
 
     await awu(collections.array.makeArray(requests)).some(async def => {
-      const { request, condition } = def
+      const { request, condition, validate } = def
       if (request.earlySuccess === undefined && request.endpoint === undefined) {
         // should not happen
         throw new Error(`Invalid request for change ${elemID.getFullName()} action ${action}`)
@@ -375,7 +420,8 @@ export const getRequester = <TOptions extends APIDefinitionsOptions>({
         return true
       }
 
-      const res = await singleRequest({ ...args, requestDef: request })
+      const res = await singleRequest({ ...args, requestDef: request, validate })
+      log.trace(`received response for change ${elemID.getFullName()} action ${action}: %o`, res)
       try {
         const dataToApply = await extractResponseDataToApply({ ...args, requestDef: def, response: res })
         if (dataToApply !== undefined) {

packages/adapter-components/test/deployment/deploy/requester.test.ts

@@ -723,7 +723,9 @@ describe('DeployRequester', () => {
         sharedContext: {},
         errors: {},
       })
-    }).rejects.toThrow('Something went wrong')
+    }).rejects.toThrow(
+      'Response validation failed for action remove adapter.test.instance.instance: { data: {}, status: 400 }',
+    )
   })
 
   it('should call the client few times when polling', async () => {

packages/okta-adapter/src/adapter.ts

@@ -61,7 +61,6 @@ import userFilter from './filters/user'
 import serviceUrlFilter from './filters/service_url'
 import schemaDeploymentFilter from './filters/schema_deployment'
 import appLogoFilter from './filters/app_logo'
-import brandThemeRemovalFilter from './filters/brand_theme_removal'
 import brandThemeFilesFilter from './filters/brand_theme_files'
 import groupMembersFilter from './filters/group_members'
 import unorderedListsFilter from './filters/unordered_lists'
@@ -114,7 +113,6 @@ const DEFAULT_FILTERS = [
   appUserSchemaAdditionAndRemovalFilter,
   schemaDeploymentFilter,
   appLogoFilter,
-  brandThemeRemovalFilter,
   brandThemeFilesFilter,
   fieldReferencesFilter,
   // should run after fieldReferencesFilter

packages/okta-adapter/src/definitions/deploy/deploy.ts

@@ -116,13 +116,20 @@ const createCustomizations = (): Record<string, InstanceDeployApiDefinitions> =>
             },
           ],
           remove: [
-            // BrandThemes are removed automatically by Okta when the Brand is removed.
-            // We use an empty request list here to mark this action as supported in case a user removed the theme
-            // alongside its Brand.
-            // A separate Change Validator ensures that themes aren't removed by themselves.
             {
+              // BrandThemes are removed automatically by Okta when a Brand is removed.
+              // We send a GET query to verify that the theme is actually removed.
               request: {
-                earlySuccess: true,
+                endpoint: {
+                  path: '/api/v1/brands/{brandId}/themes',
+                  method: 'get',
+                },
+                context: {
+                  brandId: '{_parent.0.id}',
+                },
+              },
+              validate: {
+                allowedStatusCodes: [404],
               },
             },
           ],

packages/okta-adapter/test/adapter.test.ts

@@ -2511,8 +2511,12 @@ describe('adapter', () => {
           progressReporter: nullProgressReporter,
         })
         expect(result.errors).toHaveLength(1)
-        expect(result.errors[0].message).toEqual('Expected BrandTheme to be deleted')
-        expect(result.errors[0].detailedMessage).toEqual('Expected BrandTheme to be deleted')
+        expect(result.errors[0].message).toEqual(
+          'Error: Failed to validate response for change okta.BrandTheme.instance.brandTheme',
+        )
+        expect(result.errors[0].detailedMessage).toEqual(
+          'Error: Failed to validate response for change okta.BrandTheme.instance.brandTheme',
+        )
         expect(result.appliedChanges).toHaveLength(0)
         expect(nock.pendingMocks()).toHaveLength(0)
       })