feat(PLATENG-800): Replace @lifeomic/alpha with @jupiterone/platform-sdk-fetch #1188
Conversation
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
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' && |
There was a problem hiding this comment.
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.
| }); | ||
| } else { | ||
| return new IntegrationError({ | ||
| code: 'UNEXPECTED_SYNCRONIZATION_ERROR', |
There was a problem hiding this comment.
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.
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.
Does the proxy support need to be maintained?
|
/canary-release |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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', () => { |
There was a problem hiding this comment.
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.
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; | ||
| }; |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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.
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); |
There was a problem hiding this comment.
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.
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) => { |
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "dependencies": { | ||
| "@jupiterone/integration-sdk-core": "^17.2.1", | ||
| "@lifeomic/alpha": "^5.2.0", | ||
| "@jupiterone/platform-sdk-fetch": "6.0.5-canary-490-1.0", |
There was a problem hiding this comment.
The package.json references version 6.0.5-canary-490-1.0, but the PR description mentions 6.0.3-canary-487-1.0. Ensure the version number is correct and update the PR description if the version has changed during development.
| "@jupiterone/platform-sdk-fetch": "6.0.5-canary-490-1.0", | |
| "@jupiterone/platform-sdk-fetch": "6.0.3-canary-487-1.0", |
|
/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/cli@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-cli@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-core@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-dev-tools@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-entities@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-entity-validator@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-http-client@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-runtime@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-testing@17.2.2-canary-1188-1.0 |
|
/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/cli@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-cli@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-core@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-dev-tools@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-entities@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-entity-validator@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-http-client@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-runtime@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-testing@17.2.2-canary-1188-1.0 |
- Add deprecation warnings when alphaOptions or proxyUrl are provided - Improve JSDoc deprecation messages with clearer guidance - Replace `as any` with proper RequestConfigWithRawBody interface - Add TODO for removing interface once platform-sdk-fetch exports rawBody type
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interface RequestConfigWithRawBody extends RequestClientRequestConfig { | ||
| rawBody?: Buffer; | ||
| } |
There was a problem hiding this comment.
The TODO comment mentions removing this interface once platform-sdk-fetch exports rawBody in its types. Consider adding a tracking issue reference or version number to help track when this can be removed.
| # Get list of packages changed since main (compact JSON for single-line output) | ||
| CHANGED=$(npx lerna changed --json 2>/dev/null | jq -c '.' || echo "[]") | ||
| echo "Changed packages: $CHANGED" |
There was a problem hiding this comment.
The removed comment about 'single line (no heredoc needed)' was helpful context. Consider keeping documentation about why the output format was chosen, as it clarifies the implementation decision.
| # Get list of packages changed since main (compact JSON for single-line output) | |
| CHANGED=$(npx lerna changed --json 2>/dev/null | jq -c '.' || echo "[]") | |
| echo "Changed packages: $CHANGED" | |
| # Get list of packages changed since main, as compact single-line JSON | |
| CHANGED=$(npx lerna changed --json 2>/dev/null | jq -c '.' || echo "[]") | |
| echo "Changed packages: $CHANGED" | |
| # Keep output on a single line so we can write it directly to $GITHUB_OUTPUT without a heredoc |
|
/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/cli@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-cli@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-core@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-dev-tools@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-entities@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-entity-validator@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-http-client@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-runtime@17.2.2-canary-1188-1.0
npm install @jupiterone/integration-sdk-testing@17.2.2-canary-1188-1.0 |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Store compression flag on client for use by upload functions | ||
| // Default to true to match previous Alpha behavior where uploads were always compressed | ||
| const apiClient = client as ApiClient; | ||
| if (compressUploads !== false) { | ||
| apiClient._compressUploads = true; | ||
| } |
There was a problem hiding this comment.
The condition compressUploads !== false sets _compressUploads to true when compressUploads is undefined, but the comment states this matches previous behavior where uploads were 'always compressed'. This default could be made more explicit by checking for undefined separately, or the comment should be updated to clarify that compression is enabled by default only when not explicitly disabled.
| /** | ||
| * Extended request config that includes rawBody for sending pre-serialized/compressed data. | ||
| * This extends the standard RequestClientRequestConfig to support gzip-compressed uploads. | ||
| * | ||
| * TODO: Remove this interface once platform-sdk-fetch officially exports rawBody in its types. | ||
| */ | ||
| interface RequestConfigWithRawBody extends RequestClientRequestConfig { | ||
| rawBody?: Buffer; | ||
| } |
There was a problem hiding this comment.
The interface extends RequestClientRequestConfig to add rawBody, but casts it using 'as RequestConfigWithRawBody' at the call site (line 547). This type casting bypasses TypeScript's type checking. Consider documenting this workaround more prominently or tracking this TODO with a specific issue reference.
The @jupiterone/platform-sdk-logging@5.1.4 package was tagged but never published to npm (tarball returns 404). This adds an npm override to force version 5.1.1 which is the latest actually published version. This is a workaround until platform-sdk-logging is fixed to properly publish new versions (similar to JupiterOne/web-apps-core#150).
The actions/setup-node with registry-url creates .npmrc expecting NODE_AUTH_TOKEN, but the workflow was passing NPM_AUTH_TOKEN. This caused npm ci to fail with 404 errors when downloading private @JupiterOne packages because authentication wasn't properly configured. Also reverts the npm override workaround as it's no longer needed.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 19 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.
Updated test mocks to return complete RequestClientResponse objects instead of partial objects. Added createMockResponse helper function to create properly typed mock responses. Also removed AxiosError import since we no longer use axios.
- Fix headers/config types in createMockResponse helper - Fix managedQuestionFileValidator tests to use proper response format
There was a problem hiding this comment.
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/typebox@0.32.30 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
tokio-on-jupiter
force-pushed
the
feat/PLATENG-800-replace-lifeomic-alpha
branch
from
a484569 to
642fe86
Compare
There was a problem hiding this comment.
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 |
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