Add retry to StandardValueSet query to fix network errors #652
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,6 +6,7 @@ | |
| */ | ||
|
|
||
| import { Connection, Logger } from '@salesforce/core'; | ||
| import { retry, NotRetryableError, RetryError } from 'ts-retry-promise'; | ||
| import { RegistryAccess, registry as defaultRegistry, MetadataType } from '../registry'; | ||
| import { standardValueSet } from '../registry/standardvalueset'; | ||
| import { FileProperties, StdValueSetRecord, ListMetadataQuery } from '../client/types'; | ||
|
|
@@ -108,10 +109,28 @@ export class ConnectionResolver { | |
| if (query.type === defaultRegistry.types.standardvalueset.name && members.length === 0) { | ||
| const standardValueSetPromises = standardValueSet.fullnames.map(async (standardValueSetFullName) => { | ||
| try { | ||
| // The 'singleRecordQuery' method was having connection errors, using `retry` resolves this | ||
| // Note that this type of connection retry logic may someday be added to jsforce v2 | ||
| // Once that happens this logic could be reverted | ||
| const standardValueSetRecord: StdValueSetRecord = await retry(async () => { | ||
peternhale marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| try { | ||
| return await this.connection.singleRecordQuery( | ||
| `SELECT Id, MasterLabel, Metadata FROM StandardValueSet WHERE MasterLabel = '${standardValueSetFullName}'`, | ||
| { tooling: true } | ||
| ); | ||
| } catch (err) { | ||
| // We exit the retry loop with `NotRetryableError` if we get an (expected) unsupported metadata type error | ||
| const error = err as Error; | ||
| if (error.message.includes('either inaccessible or not supported in Metadata API')) { | ||
| this.logger.debug('Expected error:', error.message); | ||
| throw new NotRetryableError(error.message); | ||
| } | ||
|
|
||
| // Otherwise throw the err so we can retry again | ||
| throw err; | ||
| } | ||
| }); | ||
|
|
||
| return ( | ||
| standardValueSetRecord.Metadata.standardValue.length && { | ||
| fullName: standardValueSetRecord.MasterLabel, | ||
|
|
@@ -126,8 +145,15 @@ export class ConnectionResolver { | |
| lastModifiedDate: '', | ||
| } | ||
| ); | ||
| } catch (err) { | ||
| // error.message here will be overwritten by 'ts-retry-promise' | ||
| // Example error.message from the library: "All retries failed" or "Met not retryable error" | ||
| // 'ts-retry-promise' exposes the actual error on `error.lastError` | ||
| const error = err as RetryError; | ||
|
|
||
| if (error.lastError && error.lastError.message) { | ||
| this.logger.debug(error.lastError.message); | ||
| } | ||
|
There was a problem hiding this comment. Should we add a few unit tests to cover the conditions added here?
It would make the unit testing a bit cleaner if you refactored the There was a problem hiding this comment. Added some tests @gbockus-sf 👍 |
||
| } | ||
| }); | ||
| for await (const standardValueSetResult of standardValueSetPromises) { | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a common thing to do for circular deps?
Does this not mean that for the tests source-tracking would be using the current version of SDR, but after deploy it'd be using whatever version it specifies in it's package.json?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
they're not circular, just transitive. plugin-source depends on SDR and STL and STL depends on SDR.
When we build the CLI, we bump both SDR and STL via yarn resolutions to their latest version.
This tests the changes on this branch everywhere that SDR is used. Imagine a bug in SDR that causes problems for STL, but you don't catch it because the nuts are using an STL with a different (bug-free) SDR. Wouldn't want to miss that!