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

Allow streaming of file content #588

Open
eric-millin opened this issue Oct 2, 2023 · 7 comments · May be fixed by microsoft/kiota-abstractions-go#158 or microsoft/kiota-http-go#159
Open

Allow streaming of file content #588

eric-millin opened this issue Oct 2, 2023 · 7 comments · May be fixed by microsoft/kiota-abstractions-go#158 or microsoft/kiota-http-go#159
Assignees
Labels
enhancement New feature or request

Comments

@eric-millin
Copy link

ItemItemsItemContentRequestBuilder::Get currently reads an entire file into memory and returns it as a []byte slice. This is a problem when dealing with lots of large files. It would be helpful if there were a way to get an io.Reader so that the response body can be processed as a stream.

@baywet
Copy link
Member

baywet commented Oct 2, 2023

Thanks for reporting this. If we look at the equivalent from the dotnet SDK it returns a stream (which would be equivalent to ReadWriteSeeker + Closer)

http.response body returns a ReadCloser, so piping things to a buffer should not be difficult.

The only challenge with this improvement is that it'd represent a breaking change to the API surface, so we'll probably need to wait the next major version (no plans at this point). I've created an issue on the generator to track that major change microsoft/kiota#3402

@baywet
Copy link
Member

baywet commented Mar 5, 2024

to clarify my prior point, even if we added overloads + deprecated comments on existing API surface for executor methods that either take a []byte as a request body or return []byte as a response body, it'd still only be part of the required change.
In the case of the request body, we'd also need to update the content field of the request information currently []byte) and the types in ParseNode constructor, SerializationWriter GetSerializedContent.
The later is really the block because simply changing the return type would be a breaking change, and adding an overload method to an interface would be as well.
This definitively needs to wait for the next major version, sorry about the miss here and the resulting memory pressure.

@andrueastman
Copy link
Member

In the short term, maybe we can update the sendPrimitive method to allow another type name (io.ReadWriter) so that in a situation where memory pressure is real concern, the user could call the ToGetRequestInformation method and then pass it to the sendPrimitive method with the added type name as a way out.

https://github.com/microsoft/kiota-http-go/blob/51c8f263d7ad3dabe446c1e3e7f1c776ce7edaaf/nethttp_request_adapter.go#L568

This will be non-breaking from our perspective and the rest of the breaking changes can be done later on which will end up calling the function with the updated type name.

@baywet
Copy link
Member

baywet commented Mar 6, 2024

So you'd effectively

  1. move the conversion from []byte to readwritercloser from the request adapter to request information
  2. add the additional field in request information, deprecate the old one.
  3. generate overrides for anything that accepts a []byte on the fluent API surface
  4. have the set content implementations in request information write directly to the new property (and rule the temporary []byte for structured payloads a thing to cleanup in the next major version + low impact on memory)

This way all streaming scenarios with large payloads would not have memory pressure anymore, we'd maintain compatibility, the only downside is the size of the SDK increasing slightly.

correct?

I like this approach, it's not perfect, but it unblocks customers.

@rkodev, can you double check the priority of this relative to other things with @maisarissi please?

@andrueastman
Copy link
Member

So you'd effectively

  1. move the conversion from []byte to readwritercloser from the request adapter to request information
  2. add the additional field in request information, deprecate the old one.
  3. generate overrides for anything that accepts a []byte on the fluent API surface
  4. have the set content implementations in request information write directly to the new property (and rule the temporary []byte for structured payloads a thing to cleanup in the next major version + low impact on memory)

This way all streaming scenarios with large payloads would not have memory pressure anymore, we'd maintain compatibility, the only downside is the size of the SDK increasing slightly.

correct?

I like this approach, it's not perfect, but it unblocks customers.

@rkodev, can you double check the priority of this relative to other things with @maisarissi please?

I was thinking more towards a middle ground without concerns about increasing the size by generation of extra methods by initially providing the means of using readwritercloser in the abstractions/requestadapter and documenting the limitation.
This at least provides a way out for the user who can write custom code that

  • calls the request generator e.g ToGetRequestInformation to create a requestInfo object.
  • pass it to the sendPrimitive method with the readwritercloser as the primitive type

Making the extra step of generation of the extra methods and deprecation of current one, could possibly evaluated separately.

@baywet
Copy link
Member

baywet commented Mar 6, 2024

if we don't generate the overloads, the gains will be hard to discover for customers (they'll effectively have to re-implement the executor method)

@wanghq
Copy link

wanghq commented Mar 6, 2024

This is how I worked around the problem. Hope it helps before the official streaming support.

func (m *microsoftService) GetOnlineMeetingRecordingContentStream(
	ctx context.Context, orgGUID, userGUID, onlineMeetingID, recordingID string) (io.ReadCloser, error) {

	req, err := m.msClient.Me().OnlineMeetings().ByOnlineMeetingId(onlineMeetingID).
		Recordings().ByCallRecordingId(recordingID).Content().ToGetRequestInformation(ctx, nil)
	if err != nil {
		return nil, trace.SetCallError(ctx, err)
	}
	// Use raw http client to get the response so we don't need to read the whole response into memory
	rawReq, err := m.msClient.RequestAdapter.ConvertToNativeRequest(ctx, req)
	if err != nil {
		return nil, trace.SetCallError(ctx, err)
	}
	httpReq, ok := rawReq.(*http.Request)
	if !ok {
		return nil, trace.SetCallError(ctx, errors.New("failed to convert to http request"))
	}
	httpReq.URL.Path = nethttplibrary.ReplacePathTokens(httpReq.URL.Path, map[string]string{"/users/me-token-to-replace": "/me"})
	httpClient := &http.Client{
		Timeout: 10 * time.Minute,
	}
	resp, err := httpClient.Do(httpReq)
	if err != nil {
		return nil, trace.SetCallError(ctx, err)
	}
	if resp.StatusCode != http.StatusOK {
		return nil, trace.SetCallError(ctx, fmt.Errorf("failed to get recording content, status code: %d", resp.StatusCode))
	}
	return resp.Body, nil
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
5 participants