[core] Add support for SSE content type

[deyaaeldeen]

Packages impacted by this PR

@azure-rest/core-client, @azure/core-rest-pipeline, @azure/openai

Issues associated with this PR

#26961
#26953

Describe the problem that is addressed by this PR

When the response's content type is text/event-stream, the response shouldn't be deserialized as JSON.

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

N/A

Are there test cases added in this PR? (If not, why?)

Yes

Provide a list of related PRs (if any)

Command used to generate this PR:**(Applicable only to SDK release request PRs)

Checklists

• Added impacted package name to the issue description
• Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
• Added a changelog (if necessary)

[azure-sdk]

API change check

APIView has identified API level changes in this PR and created following API reviews.

azure-rest-core-client

[jeremymeng]

minor comments

[xirzec]

I'm still not quite feeling the changes to core-client-rest -- today we support retrieving a stream using the asNodeStream / asBrowserStream methods.

Couldn't the codegen use this existing pattern to retrieve the stream instead of us having to hack in a new way of getting the stream?

[xirzec]

To add on to my last remark, inside getOaiSSEs instead of await responseObj call await responseObj.asNodeStream() -- it doesn't seem to matter which stream type you use since your helper supports both

[deyaaeldeen]

today we support retrieving a stream using the asNodeStream / asBrowserStream methods. Couldn't the codegen use this existing pattern to retrieve the stream instead of us having to hack in a new way of getting the stream?

There are a few problems with this:

• The return types of asNodeStream() and asBrowserStream() are not valid input to isUnexpected. Even if we add them, what would be the type of the unexpected response?
• The return type of asNodeStream() and its sibling is conservative in that it marks body as optional because it is not so sure it can return a stream body. Casting to a type with body as required feels smelly.
• Handling both node and browser streams in the client is annoying and we could leave that to core.

Taking a step back, what do we expect the RLC customer to get in their response body in this case?

[xirzec]

Thanks for raising these issues! I figured there was more pain that I wasn't seeing in this PR.

• The return types of asNodeStream() and asBrowserStream() are not valid input to isUnexpected. Even if we add them, what would be the type of the unexpected response?

This depends, I think. Unexpected responses have more to do with the HTTP status code than the body, and streaming APIs can still have atomic errors. For example, some Storage APIs will throw an XML or JSON error if the call fails but will stream on success.

It feels like a gap that isUnexpected can't handle this.

• The return type of asNodeStream() and its sibling is conservative in that it marks body as optional because it is not so sure it can return a stream body. Casting to a type with body as required feels smelly.

Agreed this feels odd. I would rather expect it to always be present and we could throw if we couldn't return one.

• Handling both node and browser streams in the client is annoying and we could leave that to core.

Yeah, this is an unfortunate result of our evolution from only supporting streams in Node at first and those having an incompatible shape with the browser streams. If we think WebStreams are the way forward, perhaps we could add a new asStream() helper and do some coercion on the Node stream if it's not supported by the runtime yet?

[deyaaeldeen]

It feels like a gap that isUnexpected can't handle this.

Totally, I guess what I am trying to say is that it is a more involved situation. Imagine having two streaming APIs and (both of them return streams of Uint8Arrays) but their error models are different for whatever reason. How can we model these unexpected response models correctly in isUnexpected when the discriminator input types are identical?

btw this OpenAI API, similar to the Storage ones, returns a JSON response on failure and a stream on success. The challenge is how to teach isUnexpected about the JSON model to narrow to. Right now, it narrows to the first error model (which is bad but happens to be similar to the one desired because all error models are identical in this case) because the input body is typed as any (notice that I didn't initialize the type variable in StreamableMethod).

I opened Azure/autorest.typescript#2008 for tracking.

I think at the end of the day we need to be deliberate about what to return to the customer in the response's body field if they don't invoke the asStream methods. I think returning the stream makes the most sense as implemented here.

[joheredi]

@deyaaeldeen do you think it would be reasonable to do this instead:

• Don't change the isStream logic
• In sdk/core/core-client-rest/src/sendRequest.ts add a check to if (firstType === "text/plain") { so that it checks for the event stream so that it deserializes to string.

This way we keep the same pattern as we have today which is to deserialize the response if you await on the method or get the stream if you explicitly call any of the asStream methods. Which also aligns with the RLC philosophy

If I understand correctly there are other 2 concerns that will need to be addressed:

• Support streams on isUnexpected
• Make the body non-optional

For isUnexpected we may be able to prototype something specific for OpenAI in customization and port it back to the code-gen once we are happy with it.

And I think it is reasonable to make the body non-optional and throw if it is not there.

Sorry if I'm missing something, will be happy to chat.

[deyaaeldeen]

@joheredi and I synced offline and he is still looking into the change.

Also to understand more about the impact of this change, see a fix to the same issue in #27109 where core is not updated. The gist of it is that for an API that could return both a stream and JSON, you can instruct core to deal with one of the formats only if you don't know beforehand which one you're getting. You will need to handle the other format manually inside the SDK. Notice all the buffering and parsing logic added in the other PR to handle the JSON case across both node and the browser. This PR adds to core the ability to automatically handle either format before it gets to the client. This issue gets much worse if the API produces more content types.

So to summarize my argument:

• core needs to sensibly process the response based on the content type (text/event-stream in this case) so that we don't unnecessarily pass the pain to handle that to the client
• the streaming methods part of the StreamableMethod interface are there to give the consumer a choice on whether to stream a response irrespective of its content-type. This can be useful in cases where the payload could be pretty large (think of small file vs large file scenario)
• some content types have to be processed in a certain way only such as text/event-stream where a stream has to always be returned. It doesn't make sense to buffer it all
• there is a gap in the current experience where if the client asked for a stream by setting the stream status codes on the request and the customer doesn't invoke one of the streaming methods, what should they be getting?

[xirzec]

Hmm yeah I don't think we considered the "error is a JSON, success is a stream" very much when we designed the original feature.

[joheredi]

I'm a bit worried about handling "text/event-stream" directly because it only helps in very specific cases. The RLC is designed to be un-opinionated, and handling specific content-types like this goes against that principle. When we were working on the asStream APIs, we thought this was the best way to deal with requests since any request can be streamed or loaded into memory. I don't know of a complete list of all the content-types that should always return a stream. If we had that, I'd be more okay with the change. But without it, making this change will lead to inconsistency when dealing with streams.

I think a better approach would be to create a helper function in RLC that can manage the stream as SSE, instead of deciding to return a stream based on the content-type. On the other hand, in DPG, we should do what you're suggesting, either by customizing it or by coding it into the codegen.

[deyaaeldeen]

Closing this for now.

[github-actions]

Hi @deyaaeldeen. Thank you for your interest in helping to improve the Azure SDK experience and for your contribution. We've noticed that there hasn't been recent engagement on this pull request. If this is still an active work stream, please let us know by pushing some changes or leaving a comment. Otherwise, we'll close this out in 7 days.

[github-actions]

Hi @deyaaeldeen. Thank you for your contribution. Since there hasn't been recent engagement, we're going to close this out. Feel free to respond with a comment containing /reopen if you'd like to continue working on these changes. Please be sure to use the command to reopen or remove the no-recent-activity label; otherwise, this is likely to be closed again with the next cleanup pass.