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

Add support for InputStream #292

Merged
merged 2 commits into from
Sep 13, 2024

Conversation

ctasada
Copy link
Contributor

@ctasada ctasada commented Jun 21, 2024

Fabrikt already offers support for ByteArray, but when dealing with large files, for example, the usage of ByteArray puts too much pressure in memory.

This MR provides BYTEARRAY_AS_INPUTSTREAM. A new --type-overrides that allows to provide an InputStream when dealing with binary data.

Fabrikt already offers support for `ByteArray`, but when dealing with large files, for example, the usage of `ByteArray` puts too much pressure in memory.

This MR provides `BYTEARRAY_AS_INPUTSTREAM`. A new `--type-overrides` that allows to provide an `InputStream` when dealing with binary data.
@cjbooms
Copy link
Owner

cjbooms commented Jun 21, 2024

Thanks for the contribution. It'll be Monday before I get to look at this. Unless @averabaq can handle it before then

@averabaq
Copy link
Collaborator

This is a good one that's been on the radar for some time. I'll try to put some eyes on it over the weekend. Thank you so much for this @ctasada

@averabaq
Copy link
Collaborator

The server side code-generation looks good to me, no objections on this. On the client side, however, the code generated is incorrect:

    public fun postExample(
        applicationOctetStream: InputStream,
        additionalHeaders: Map<String, String> = emptyMap(),
    ): ApiResponse<Unit> {
        ...

        val request: Request = Request.Builder()
           ....
            .post(objectMapper.writeValueAsString(applicationOctetStream).toRequestBody("application/octet-stream".toMediaType()))          
           ....
    }

Supporting streaming on the client side is not that simple. This is only for OkHttp and we also have support for OpenFeign. Since this is a new feature and not a breaking change, I wouldn't mind to go ahead with this, but we should either:

  1. Allow input streams on the models for now and resolve to byte-arrays on request bodies regardless.
  2. Don't allow input streams on client-side generation at all (i.e. ignore the byte_array_as_input_stream cli option)
  3. Add full stream support on client side.

I can't proceed as is, sorry. Let's see what @cjbooms thinks about it next week.

@cjbooms
Copy link
Owner

cjbooms commented Jul 8, 2024

The server side code-generation looks good to me, no objections on this. On the client side, however, the code generated is incorrect:

    public fun postExample(
        applicationOctetStream: InputStream,
        additionalHeaders: Map<String, String> = emptyMap(),
    ): ApiResponse<Unit> {
        ...

        val request: Request = Request.Builder()
           ....
            .post(objectMapper.writeValueAsString(applicationOctetStream).toRequestBody("application/octet-stream".toMediaType()))          
           ....
    }

Supporting streaming on the client side is not that simple. This is only for OkHttp and we also have support for OpenFeign. Since this is a new feature and not a breaking change, I wouldn't mind to go ahead with this, but we should either:

  1. Allow input streams on the models for now and resolve to byte-arrays on request bodies regardless.
  2. Don't allow input streams on client-side generation at all (i.e. ignore the byte_array_as_input_stream cli option)
  3. Add full stream support on client side.

I can't proceed as is, sorry. Let's see what @cjbooms thinks about it next week.

Option 2 sounds good to me. Allow it on server-side generation only

@cjbooms
Copy link
Owner

cjbooms commented Jul 20, 2024

What is need to get this across the line @averabaq ? Could it be merged as is with the configuration option as protection against the client side generation being incorrect?

@averabaq
Copy link
Collaborator

averabaq commented Jul 23, 2024

Could it be merged as is with the configuration option as protection against the client side generation being incorrect?

Yes, I didn't make it go through because I was waiting for getting a PR ready with the required changes, but haven't had the chance to work on it. We can proceed and release it to unblock @ctasada. I'll try to add the cli changes later on

@averabaq averabaq merged commit 696c2b0 into cjbooms:master Sep 13, 2024
1 check passed
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