[Linkedin Conversions] Add multistatus in linkedin conversions' stream conversion event#3745
[Linkedin Conversions] Add multistatus in linkedin conversions' stream conversion event#3745harsh-joshi99 wants to merge 1 commit intomainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds spec-v2 MultiStatus support to the LinkedIn Conversions streamConversion batched pathway so invalid events can be failed individually while valid events in the same batch are still sent.
Changes:
- Update
LinkedInConversions.batchConversionAddto build and return aMultiStatusResponse, filtering invalid payloads and mapping responses back to original batch indices. - Remove legacy try/catch wrapping in the action’s
performBatchnow that the API client returns a MultiStatus response. - Add unit tests covering mixed-validity batches, all-invalid batches (no outbound call), index mapping, and 401 handling.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/destination-actions/src/destinations/linkedin-conversions/streamConversion/index.ts | Switch batched execution to directly return the API client’s MultiStatusResponse. |
| packages/destination-actions/src/destinations/linkedin-conversions/api/index.ts | Implement MultiStatus construction, validation filtering, and per-index success/error response mapping. |
| packages/destination-actions/src/destinations/linkedin-conversions/streamConversion/tests/index.test.ts | Add coverage for multistatus behavior across validation errors, no-op batches, 401 throws, and index mapping. |
| } catch (error) { | ||
| if (error instanceof HTTPError) { | ||
| const status = error.response.status | ||
|
|
||
| // 401 means the OAuth token is expired — re-throw so the platform triggers a token refresh | ||
| if (status === 401) { | ||
| throw new InvalidAuthenticationError( | ||
| 'LinkedIn OAuth token is expired or invalid. Please re-authenticate.', | ||
| ErrorCodes.INVALID_AUTHENTICATION | ||
| ) | ||
| } | ||
|
|
||
| // For all other API errors, mark every valid event as failed with the actual status | ||
| validResponseIndices.forEach((originalIndex, filteredIndex) => { | ||
| multiStatusResponse.setErrorResponseAtIndex(originalIndex, { | ||
| status, | ||
| errormessage: error.message, | ||
| sent: validElements[filteredIndex] as unknown as JSONLikeObject | ||
| }) | ||
| }) |
There was a problem hiding this comment.
When the LinkedIn batch API call fails with retryable statuses (e.g., 408/423/429 and most 5xx), returning a MultiStatusResponse marks events as permanently failed and bypasses the retry behavior previously implemented via handleRequestError (which classified these as retryable). Consider throwing a RetryableError (or otherwise surfacing a retryable failure) for these whole-batch transport/API failures so events are retried instead of dropped.
There was a problem hiding this comment.
@copilot based on this feedback, prior to this change, was the handleRequestError being called for failures coming from the request, seeing as we do not await.
There was a problem hiding this comment.
Correct — handleRequestError was not being called for HTTP request failures in the original code. Because batchConversionAdd was returned without await, the surrounding try/catch only captured synchronous errors. Any async errors from the HTTP request (5xx, 429, etc.) would propagate as unhandled promise rejections and bypass handleRequestError entirely.
So the retryable-error concern raised by the reviewer is a net-new consideration rather than a regression from the original behavior. The original code was already broken for those cases.
| // For all other API errors, mark every valid event as failed with the actual status | ||
| validResponseIndices.forEach((originalIndex, filteredIndex) => { | ||
| multiStatusResponse.setErrorResponseAtIndex(originalIndex, { | ||
| status, | ||
| errormessage: error.message, | ||
| sent: validElements[filteredIndex] as unknown as JSONLikeObject | ||
| }) |
There was a problem hiding this comment.
HTTPError.message is derived from response.statusText (often generic/empty) and won't include LinkedIn's error payload. To make multistatus error details actionable, prefer populating errormessage (and optionally body) from error.response.data / error.response.content when available.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3745 +/- ##
==========================================
- Coverage 81.07% 80.92% -0.15%
==========================================
Files 1655 1347 -308
Lines 32088 25067 -7021
Branches 7090 5202 -1888
==========================================
- Hits 26014 20285 -5729
+ Misses 5101 3835 -1266
+ Partials 973 947 -26 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Testing
Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.
Security Review
Please ensure sensitive data is properly protected in your integration.
type: 'password'New Destination Checklist
verioning-info.tsfile. example