-
Notifications
You must be signed in to change notification settings - Fork 15
feat(PLATENG-800): Replace @lifeomic/alpha with @jupiterone/platform-sdk-fetch #1188
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
base: main
Are you sure you want to change the base?
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
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.
Pull request overview
This PR replaces the deprecated @lifeomic/alpha HTTP client with @jupiterone/platform-sdk-fetch canary release to modernize the API client implementation.
Changes:
- Updated dependency from
@lifeomic/alphato@jupiterone/platform-sdk-fetch - Replaced
Alphatype withRequestClientthroughout the codebase - Removed proxy configuration support and deprecated
alphaOptions/proxyUrlparameters
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/integration-sdk-runtime/package.json | Updated dependency to platform-sdk-fetch canary |
| packages/integration-sdk-runtime/src/api/index.ts | Replaced Alpha with RequestClient, removed proxy support |
| packages/integration-sdk-runtime/src/synchronization/index.ts | Updated type imports and error handling |
| packages/integration-sdk-runtime/src/synchronization/events.ts | Updated config type imports |
| packages/integration-sdk-runtime/src/synchronization/error.ts | Replaced AxiosError with custom RequestClientError interface |
| packages/integration-sdk-runtime/tsconfig.dist.json | Added skipLibCheck to handle external type issues |
| packages/cli/src/import/importAssetsFromCsv.ts | Updated type imports |
| packages/integration-sdk-runtime/src/api/tests/index.test.ts | Updated mocks for RequestClient |
| packages/cli/src/tests/cli-import.test.ts | Updated test mocks |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const compressRequest: RequestInterceptor = function (config) { | ||
| if ( | ||
| config.method === 'post' && | ||
| config.method === 'POST' && |
Copilot
AI
Jan 23, 2026
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.
The compressRequest interceptor now only sets the 'Content-Encoding' header but does not actually compress the data. The comment on lines 126-128 notes this issue but doesn't implement actual compression. This means data won't be compressed despite the header claiming it is, which could cause server-side decompression failures.
| @@ -23,7 +35,7 @@ export function synchronizationApiError( | |||
| return new IntegrationError({ | |||
| code: 'UNEXPECTED_SYNCRONIZATION_ERROR', | |||
Copilot
AI
Jan 23, 2026
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.
Corrected spelling of 'SYNCRONIZATION' to 'SYNCHRONIZATION'.
| code: 'UNEXPECTED_SYNCRONIZATION_ERROR', | |
| code: 'UNEXPECTED_SYNCHRONIZATION_ERROR', |
| 'Content-Encoding': 'gzip', | ||
| }; | ||
| } | ||
| config.data = await gzipData(config.data); |
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.
Does config.data need to be compressed here?
|
/canary-release |
| export const getAccountFromEnvironment = () => | ||
| getFromEnv('JUPITERONE_ACCOUNT', IntegrationAccountRequiredError); | ||
|
|
||
| function parseProxyUrl(proxyUrl: string) { |
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.
Does the proxy support need to be maintained?
|
/canary-release |
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.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const compressRequest: RequestInterceptor = function (config) { | ||
| if ( | ||
| config.method === 'post' && | ||
| config.method === 'POST' && | ||
| config.url && | ||
| /\/persister\/synchronization\/jobs\/[0-9a-fA-F-]+\/(entities|relationships)/.test( | ||
| config.url, | ||
| ) | ||
| ) { | ||
| if (config.headers) { | ||
| config.headers['Content-Encoding'] = 'gzip'; | ||
| } else { | ||
| config.headers = { | ||
| // Note: Compression is handled differently in RequestClient | ||
| // The data compression would need to be applied at the request level | ||
| // For now, we mark the headers - actual compression may need additional handling | ||
| return { | ||
| ...config, | ||
| headers: { | ||
| ...config.headers, | ||
| 'Content-Encoding': 'gzip', | ||
| }; | ||
| } | ||
| config.data = await gzipData(config.data); | ||
| }, | ||
| }; | ||
| } | ||
| return config; |
Copilot
AI
Jan 23, 2026
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.
The compressRequest interceptor sets the 'Content-Encoding: gzip' header but does not actually compress the data. The previous implementation called gzipData(config.data) but this logic has been removed. This will cause the server to expect compressed data but receive uncompressed data, resulting in decompression failures. Either implement the actual data compression using gzipData or remove this interceptor entirely if compression is handled elsewhere.
| code: 'UNEXPECTED_SYNCRONIZATION_ERROR', | ||
| message: errorMessage, | ||
| cause: err, | ||
| cause: err instanceof Error ? err : undefined, |
Copilot
AI
Jan 23, 2026
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.
Setting cause to undefined when the error is not an instance of Error loses error information. The original implementation passed err directly to preserve all error details. Consider using cause: err as Error or structuring the error differently to retain the original error object for debugging purposes.
| cause: err instanceof Error ? err : undefined, | |
| cause: err as Error, |
| }); | ||
|
|
||
| describe('getApiKeyFromEnvironment', () => { | ||
| describe('getAccountFromEnvironment', () => { |
Copilot
AI
Jan 23, 2026
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.
The describe block name 'getAccountFromEnvironment' is a duplicate of line 55's describe block name which should be 'getApiKeyFromEnvironment'. This appears to be a copy-paste error from the original code that should be corrected for clarity.
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.
Pull request overview
Copilot reviewed 10 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** | ||
| * Request interceptor that compresses upload data for synchronization endpoints | ||
| */ | ||
| export const compressRequest: RequestInterceptor = function (config) { | ||
| if ( | ||
| config.method === 'post' && | ||
| config.method === 'POST' && | ||
| config.url && | ||
| /\/persister\/synchronization\/jobs\/[0-9a-fA-F-]+\/(entities|relationships)/.test( | ||
| config.url, | ||
| ) | ||
| ) { | ||
| if (config.headers) { | ||
| config.headers['Content-Encoding'] = 'gzip'; | ||
| } else { | ||
| config.headers = { | ||
| // Note: Compression is handled differently in RequestClient | ||
| // The data compression would need to be applied at the request level | ||
| // For now, we mark the headers - actual compression may need additional handling | ||
| return { | ||
| ...config, | ||
| headers: { | ||
| ...config.headers, | ||
| 'Content-Encoding': 'gzip', | ||
| }; | ||
| } | ||
| config.data = await gzipData(config.data); | ||
| }, | ||
| }; | ||
| } | ||
| return config; | ||
| }; |
Copilot
AI
Jan 23, 2026
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.
The compressRequest interceptor sets the 'Content-Encoding: gzip' header but doesn't actually compress the data. The original implementation called gzipData(config.data) to compress the request body. Without actual compression, servers expecting gzipped data will fail to process the request.
| accessToken, | ||
| retryOptions, | ||
| compressUploads, | ||
| alphaOptions, | ||
| proxyUrl, | ||
| }: CreateApiClientInput): ApiClient { |
Copilot
AI
Jan 23, 2026
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.
The deprecated parameters alphaOptions and proxyUrl are destructured but never used in the function body. Either remove them from the destructuring or explicitly acknowledge their deprecation with a warning log.
| code: 'UNEXPECTED_SYNCRONIZATION_ERROR', | ||
| message: errorMessage, | ||
| cause: err, | ||
| cause: err instanceof Error ? err : undefined, |
Copilot
AI
Jan 23, 2026
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.
When err is not an Error instance, setting cause to undefined loses error context. Consider converting unknown errors to Error instances or using the original value to preserve debugging information.
| cause: err instanceof Error ? err : undefined, | |
| cause: err, |
|
/canary-release |
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.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| }, | ||
| ); | ||
| rawBody: compressedData, | ||
| } as any); |
Copilot
AI
Jan 23, 2026
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.
Using as any type assertion bypasses type safety. Consider defining a proper type for the options parameter that includes the rawBody property, or add a TODO comment explaining this is temporary until the upstream types are updated.
|
🚀 Canary release workflow has been triggered. You can follow the progress here. |
|
❌ Canary release failed. Check the workflow run for details. |
|
/canary-release |
|
🚀 Canary release workflow has been triggered. You can follow the progress here. |
|
❌ Canary release failed. Check the workflow run for details. |
|
/canary-release |
|
🚀 Canary release workflow has been triggered. You can follow the progress here. |
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.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| client.interceptors.response.use( | ||
| (response) => response, | ||
| (error: any) => { | ||
| async (error: any) => { |
Copilot
AI
Jan 23, 2026
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.
The error parameter is typed as any, which reduces type safety. Consider defining a proper error type that captures the expected error structure from RequestClient.
|
|
||
| export function synchronizationApiError( | ||
| err: AxiosError<SynchronizationApiErrorResponse>, | ||
| err: RequestClientError | Error | unknown, |
Copilot
AI
Jan 23, 2026
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.
The parameter accepts unknown in addition to specific error types, but then performs type assertions without proper type guards. Consider adding a type guard function to safely narrow the type before accessing properties.
|
❌ Canary release failed. Check the workflow run for details. |
|
/canary-release |
- Fix headers/config types in createMockResponse helper - Fix managedQuestionFileValidator tests to use proper response format
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.
Pull request overview
Copilot reviewed 23 out of 24 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ault - Add npm overrides to pin @sinclair/[email protected] to match main branch lockfile versions and avoid type incompatibilities - Add dotenv and dotenv-expand dependencies to integration-sdk-cli package to ensure correct v5 API is used (root node_modules has v10 for nx) - Update test expectations for compression default change (now enabled by default) - Disable compression in batching/upload tests to maintain test behavior
a484569 to
642fe86
Compare
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.
Pull request overview
Copilot reviewed 26 out of 27 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
/canary-release |
|
🚀 Canary release workflow has been triggered. You can follow the progress here. |
|
✅ Canary release published successfully! Published packages:
Install with: npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected] |
…ublish - Change preid counter from github.run_attempt to github.run_id to ensure unique versions across different workflow runs - Add git commit step before lerna publish from-package to ensure lerna detects the version changes as publishable
|
/canary-release |
|
🚀 Canary release workflow has been triggered. You can follow the progress here. |
|
✅ Canary release published successfully! Published packages:
Install with: npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected] |
|
/canary-release |
|
🚀 Canary release workflow has been triggered. You can follow the progress here. |
|
✅ Canary release published successfully! Published packages:
Install with: npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/integration-sdk-entity-validator@17.2.2-canary-1188-21458536266.0
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected]
npm install @jupiterone/[email protected] |
| const mockedGlobby = jest.mocked(globby); | ||
|
|
||
| // Mock RequestClient | ||
| const mockPost = jest.fn(); |
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.
Would recommend moving these to integration-sdk-testing since they're used and redeclared across multiple test files
| const mockedAxios = jest.mocked<AxiosInstance>(axios); | ||
| const mockedFileSystem = jest.mocked(fileSystem); | ||
|
|
||
| const mockGet = jest.fn(); |
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.
Would recommend moving these mocks to integration-sdk-testing since they're used and redeclared across multiple test files
| beforeEach(() => { | ||
| mockedRuntime.createApiClient.mockReturnValue(axios); | ||
| mockGet.mockReset(); | ||
| mockedRuntime.createApiClient.mockReturnValue(mockApiClient as any); |
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.
This is a violation of our no any policy - I would refactor this to support using a strongly typed interface. I would recommend using the node-typescript quality plugin moving forward that will continually enforce and handle auto fixing issues like this (immediately after code generation) in the future: https://github.com/JupiterOne/claude-toolbox/pull/39
| response: { use: jest.fn(), eject: jest.fn() }, | ||
| }; | ||
|
|
||
| jest.mock('@jupiterone/platform-sdk-fetch', () => ({ |
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.
Let's also move this to integration-sdk-testing
| "compilerOptions": { | ||
| "sourceMap": true | ||
| "sourceMap": true, | ||
| "skipLibCheck": true |
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.
Revert
| level: 'trace', | ||
| type: 'raw', | ||
| stream: ringbuffer, | ||
| stream: ringbuffer as any, |
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.
as any violation - revert/fix
| "strictFunctionTypes": true, | ||
| "allowJs": false | ||
| "allowJs": false, | ||
| "skipLibCheck": true |
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.
| "skipLibCheck": true | |
| "skipLibCheck": false |
| [type]: batch, | ||
| }, | ||
| { | ||
| const url = `/persister/synchronization/jobs/${jobId}/${type as string}`; |
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.
All of this should be a handled internal implementation detail of platform-sdk-fetch requestClient. For any consumer service today that uses: @lifeomic/alpha, we want to ensure that they get a drop in compatible replacement. If this code is required in the consuming service additional work needs to be done in platform-sdk-fetch to ensure we can minimize required code changes across all affected services so we can quickly migrate off @lifeomic/alpha by utilizing a deterministic codemod/code transform.
| } | ||
|
|
||
| function cleanAxiosError(err: AxiosError) { | ||
| function cleanRequestError(err: any) { |
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.
This should likely be replaced with a similar signature such as:
err: HttpRequestError that would be exported by platform-sdk-fetch as a type.
The type could have the same properties, etc. as AxiosError for drop in consumer compatibility.
Summary
Replace
@lifeomic/alphawith@jupiterone/platform-sdk-fetchcanary release (6.0.3-canary-487-1.0).Changes
packages/integration-sdk-runtime/package.jsonpackages/integration-sdk-runtime/src/api/index.tspackages/integration-sdk-runtime/src/synchronization/*.tspackages/cli/src/import/importAssetsFromCsv.tstsconfig.dist.jsonBreaking Changes
ApiClienttype is nowRequestClientinstead ofAlphaalphaOptionsandproxyUrl(not supported by RequestClient)Test Plan
Related