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

feat!: convert stream error responses #1663

Merged
merged 1 commit into from
Nov 6, 2024
Merged

Conversation

ricellis
Copy link
Member

@ricellis ricellis commented Nov 4, 2024

PR summary

Add interceptor for converting streamed error responses to error objects.

Fixes: s1022

Note: An existing issue is required before opening a PR.

PR Checklist

Please make sure that your PR fulfills the following requirements:

  • The commit message follows the
    Angular Commit Message Guidelines.
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

  • Bugfix
  • Feature
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • New tests
  • Build/CI related changes
  • Documentation content changes
  • Other (please describe)

What is the current behavior?

For *AsStrream endpoint successful responses are returned as Readable streams. In the case of an error the error response body is also provided as a Readable by the SDK core. This deviates from the behaviour of errors from all other endpoints and prevents the SDK core from, for example, formatting the error response nicely. It also prevents the new improved error responses from #1646 from functioning for *AsStream calls.

What is the new behavior?

For unsuccessful responses the error response body is automatically converted from the Readable into a JSON object. This means error handling behaviour can be the same for all endpoints and the enhancements from the error response interceptor and error formatting code paths can be applied to stream error cases as well.

Does this PR introduce a breaking change?

  • Yes
  • No

No change for success responses.
For error responses the error's result is converted from a stream to an object.
User code that catches the rejection and tries to process the result as a stream will need modification.

Other information

Modified existing tests error_no_augment_stream becomes error_augment_stream,
Removed obsolete stream handling code in tests.
Modified the interceptor assertion code to account for the presence of two interceptors and validate the ordering.

@ricellis ricellis self-assigned this Nov 4, 2024
Copy link
Member

@eiri eiri left a comment

Choose a reason for hiding this comment

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

A couple of nits, but overall looks fine to me

test/unit/errorResponseInterceptor.test.js Outdated Show resolved Hide resolved
test/unit/errorResponseInterceptor.test.js Outdated Show resolved Hide resolved
test/unit/errorResponseInterceptor.test.js Outdated Show resolved Hide resolved
lib/errorResponseInterceptor.ts Outdated Show resolved Hide resolved
@ricellis ricellis force-pushed the s1022-stream-error-converter branch from e2f1e37 to 089b942 Compare November 5, 2024 16:13
Copy link
Member

@eiri eiri left a comment

Choose a reason for hiding this comment

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

+1

Reject with a parsed JSON object for streaming response cases.

BREAKING CHANGE: Error responses from *AsStream endpoints now
return a JSON object on the rejected promise instead of the raw
stream. Successful responses continue to provide a stream result.
@ricellis ricellis force-pushed the s1022-stream-error-converter branch from 17cb22e to f3cca19 Compare November 6, 2024 13:39
@ricellis ricellis merged commit 32e0c25 into main Nov 6, 2024
8 checks passed
@ricellis ricellis deleted the s1022-stream-error-converter branch November 6, 2024 13:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants