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

requestToExApp: added CURLStringFile support #168

Merged
merged 4 commits into from
Dec 19, 2023
Merged

requestToExApp: added CURLStringFile support #168

merged 4 commits into from
Dec 19, 2023

Conversation

bigcat88
Copy link
Member

@bigcat88 bigcat88 commented Dec 19, 2023

@kyteinsky, please look, is this what you need or not.
If yes, then I will continue and finish it today.

Usage(php watch ref):

$txt = 'test content';
$txt_curlfile = new \CURLStringFile($txt, 'test.txt', 'text/plain');
$this->service->aeRequestToExAppById('appid', '/upload_file', '', 'POST', ['file' => $txt_curlfile, 'notes' => 'lol']);
		

Usage from Python backend(fastapi ref):

@APP.post("/upload_file")
def upload_file(file: UploadFile, notes: Annotated[str | None, Form()] = None):
    print(notes)
    if notes is None:
        return responses.JSONResponse({"filename": file.filename, "size": file.size})
    return responses.JSONResponse({"filename": file.filename, "size": file.size, "notes": notes})

"notes" is a form parameter, it is optional.

@kyteinsky
Copy link
Collaborator

CURLStringFile requires the contents of the whole file read into memory by PHP. This can lead to OOM issues for large files and shouldn't be done for binary data I suppose.
The way it is done in context chat is by putting the file handle in the contents key. It can may still OOM out now that I think about it but seems like a better approach.

'name' => 'someName',
'filename' => $node->getName(),
'contents' => $node->fopen('r'),

And the request is identified by the content-type set by the caller of request method. That can change but I don't know any better ways to do that.

public function request(
  ?string $userId,
  \OCA\AppAPI\Db\ExApp $exApp,
  string $route,
  string $method = 'POST',
  array $params = [],
  ?string $contentType = null,
): mixed {

...

} else {
  if ($contentType === 'multipart/form-data') {
    $options['multipart'] = $params;
  } else {
    $options['body'] = json_encode($params);
  }
}

@bigcat88
Copy link
Member Author

bigcat88 commented Dec 19, 2023

CURLStringFile requires the contents of the whole file read into memory by PHP. This can lead to OOM issues for large files and shouldn't be done for binary data I suppose.
The way it is done in context chat is by putting the file handle in the contents key. It can may still OOM out now that I think about it but seems like a better approach.

Should I add support for CURLFile as well, will it be enough?

And the request is identified by the content-type set by the caller of request method. That can change but I don't know any better ways to do that.

You should specify mime in CURLStringFile or CURLFile for the file.

For the full request contentType will be always 'multipart/form-data', if I understand documentation correctly.
Or I missed something?

@kyteinsky
Copy link
Collaborator

Should I add support for CURLFile as well, will it be enough?

CURLFile takes a file name, which we don't have easily since we're dealing with NC File objects. So we cannot use that.

For the full request contentType will be always 'multipart/form-data', if I understand documentation correctly.
Or I missed something?

Yes this thing but the request content-type (multipart/form-data) is specified by the caller and not inferred by the method.

@bigcat88 bigcat88 marked this pull request as ready for review December 19, 2023 13:25
Copy link
Collaborator

@kyteinsky kyteinsky left a comment

Choose a reason for hiding this comment

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

thanks, looks neat!

@bigcat88
Copy link
Member Author

Additionally, support has been added for specifying custom $options['multipart'] and if this is specified, then what is specified will be passed to the Guzzle client.

This allows passing of file handles.

'multipart' => [
            [
                'name'     => 'file', // Name of the form field
                'contents' => $fileHandle,
                'filename' => 'file.txt' // Optional: specify filename
            ],
            // You can add more form fields here if needed
        ]

@bigcat88 bigcat88 enabled auto-merge (squash) December 19, 2023 13:50
@bigcat88 bigcat88 merged commit 23259d9 into main Dec 19, 2023
28 checks passed
@bigcat88 bigcat88 deleted the multipart branch December 19, 2023 13:55
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