Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SALTO-7409: Handle "INSUFFICIENT_ACCESS: insufficient access rights on entity" errors in retrieve #7232

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 40 additions & 3 deletions packages/salesforce-adapter/src/fetch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
import _ from 'lodash'
import JSZip from 'jszip'
import { inspectValue, safeJsonStringify } from '@salto-io/adapter-utils'
import { FileProperties, MetadataInfo, MetadataObject } from '@salto-io/jsforce-types'
import { FileProperties, MetadataInfo, MetadataObject, RetrieveRequest, RetrieveResult } from '@salto-io/jsforce-types'
import { InstanceElement, ObjectType, TypeElement } from '@salto-io/adapter-api'
import { collections, objects, values as lowerDashValues } from '@salto-io/lowerdash'
import { logger } from '@salto-io/logging'
Expand Down Expand Up @@ -458,7 +458,37 @@ export const retrieveMetadataInstances = async ({
const typesToRetrieve = [...new Set(filesToRetrieve.map(prop => prop.type))].join(',')
log.debug('retrieving types %s', typesToRetrieve)
const request = toRetrieveRequest(filesToRetrieve)
const result = await client.retrieve(request)

const typesWithInsufficientAccess = new Set<string>()
const retrieveWithRetry = async (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a great name because the real functionality this function provides is not the retry but rather the handling of types with insufficient access.
Maybe call it retrieveAccessibleEntities or something like that.

currentRequest: RetrieveRequest,
CurrentFilesToRetrieve: FileProperties[],
): Promise<RetrieveResult> => {
try {
return await client.retrieve(currentRequest)
} catch (error) {
const errorPattern = /INSUFFICIENT_ACCESS: insufficient access rights on entity: (\w+)/g
const matches = [...error.message.matchAll(errorPattern)]
if (matches.length > 0) {
matches.forEach(match => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can there be multiple matches?

const failedEntity = match[1]
log.debug(`Failed to retrieve ${failedEntity} due to insufficient access rights`)
typesWithInsufficientAccess.add(failedEntity)
})
const updatedFilesToRetrieve = CurrentFilesToRetrieve.filter(
fileProp => ![...matches.map(match => match[1])].includes(fileProp.type),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will also be simpler once you assume there's only 1 match.

)
if (updatedFilesToRetrieve.length === 0) {
throw new Error('No files left to retrieve after filtering out failed entities')
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this an error? Just return...

}
const updatedRequest = toRetrieveRequest(updatedFilesToRetrieve)
return retrieveWithRetry(updatedRequest, updatedFilesToRetrieve)
}
throw error
}
}

const result = await retrieveWithRetry(request, filesToRetrieve)

log.debug('retrieve result for types %s: %o', typesToRetrieve, _.omit(result, ['zipFile', 'fileProperties']))

Expand Down Expand Up @@ -505,7 +535,14 @@ export const retrieveMetadataInstances = async ({
)
).flat()
}

typesWithInsufficientAccess.forEach(entity => {
configChanges.push(
createSkippedListConfigChange({
type: entity,
reason: `Insufficient access rights on entity: ${entity}`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be written in more Salto language by using "type" instead of "entity".

}),
)
})
const newConfigChanges = createRetrieveConfigChange(result).filter(change => !configChangeAlreadyExists(change))
configChanges.push(...newConfigChanges)
// if we get an error then result.zipFile will be a single 'nil' XML element, which will be parsed as an object by
Expand Down
81 changes: 78 additions & 3 deletions packages/salesforce-adapter/test/adapter.discover.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ import {
import { MetadataInfo } from '@salto-io/jsforce'
import { collections, values } from '@salto-io/lowerdash'
import { MockInterface } from '@salto-io/test-utils'
import { FileProperties } from '@salto-io/jsforce-types'
import { FileProperties, RetrieveRequest } from '@salto-io/jsforce-types'
import { buildElementsSourceFromElements } from '@salto-io/adapter-utils'
import SalesforceAdapter from '../src/adapter'
import Connection from '../src/client/jsforce'
Expand All @@ -47,7 +47,7 @@ import {
mockRetrieveLocator,
mockRetrieveResult,
} from './connection'
import { ConfigChangeSuggestion, FetchElements, MAX_ITEMS_IN_RETRIEVE_REQUEST } from '../src/types'
import { ConfigChangeSuggestion, FetchElements, FetchProfile, MAX_ITEMS_IN_RETRIEVE_REQUEST } from '../src/types'
import * as fetchModule from '../src/fetch'
import { fetchMetadataInstances, retrieveMetadataInstances } from '../src/fetch'
import * as xmlTransformerModule from '../src/transformers/xml_transformer'
Expand All @@ -68,7 +68,7 @@ import {
SOCKET_TIMEOUT,
} from '../src/constants'
import { apiNameSync, isInstanceOfType, isInstanceOfTypeSync } from '../src/filters/utils'
import { NON_TRANSIENT_SALESFORCE_ERRORS } from '../src/config_change'
import { createSkippedListConfigChange, NON_TRANSIENT_SALESFORCE_ERRORS } from '../src/config_change'
import SalesforceClient from '../src/client/client'
import createMockClient from './client'
import { mockInstances, mockTypes } from './mock_elements'
Expand Down Expand Up @@ -2751,6 +2751,81 @@ describe('Fetch via retrieve API', () => {
},
)

describe('when retrieve fails for INSUFFICIENT_ACCESS rights on entity', () => {
let fetchProfile: FetchProfile
let mockRetrieve: jest.Mock

beforeEach(async () => {
const instances = [
{
type: mockTypes.CustomObject,
instanceName: 'CustomObject1',
},
{
type: mockTypes.CustomObject,
instanceName: 'CustomObject2',
},
{
type: mockTypes.Flow,
instanceName: 'Flow1',
},
{
type: mockTypes.Flow,
instanceName: 'Flow2',
},
{
type: mockTypes.Opportunity,
instanceName: 'Opportunity1',
},
{
type: mockTypes.Opportunity,
instanceName: 'Opportunity2',
},
]
await setupMocks(instances)
fetchProfile = buildFetchProfile({
fetchParams: {},
})
mockRetrieve = jest.fn().mockImplementation((retrieveRequest: RetrieveRequest) => {
if (
retrieveRequest.unpackaged?.types.some(t => t.name === 'CustomObject') &&
retrieveRequest.unpackaged?.types.some(t => t.name === 'Flow')
) {
throw new Error(
`Retrieve request for ${retrieveRequest.unpackaged.types} failed. messages: INSUFFICIENT_ACCESS: insufficient access rights on entity: CustomObject INSUFFICIENT_ACCESS: insufficient access rights on entity: Flow`,
)
}
if (retrieveRequest.unpackaged?.types.some(t => t.name === 'CustomObject')) {
throw new Error(
`Retrieve request for ${retrieveRequest.unpackaged.types} failed. messages: INSUFFICIENT_ACCESS: insufficient access rights on entity: CustomObject`,
)
}
return connection.metadata.retrieve(retrieveRequest).complete()
})
})
it('should try again without the failed entity and create a config suggestion', async () => {
const expectedConfigChanges = [
createSkippedListConfigChange({
type: 'CustomObject',
reason: 'Insufficient access rights on entity: CustomObject',
}),
createSkippedListConfigChange({
type: 'Flow',
reason: 'Insufficient access rights on entity: Flow',
}),
]
jest.spyOn(client, 'retrieve').mockImplementation(mockRetrieve)
const { configChanges } = await retrieveMetadataInstances({
client,
types: [mockTypes.CustomObject, mockTypes.Flow, mockTypes.Opportunity],
fetchProfile,
})
// expect(elements).toHaveLength(2)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks left over.

expect(client.retrieve).toHaveBeenCalledTimes(2)
expect(configChanges).toIncludeSameMembers(expectedConfigChanges)
})
})

describe('Config changes', () => {
let configChanges: ConfigChangeSuggestion[]

Expand Down