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

Feature request / question: file upload helpers #195

Open
Slijkhuis opened this issue Sep 14, 2023 · 11 comments
Open

Feature request / question: file upload helpers #195

Slijkhuis opened this issue Sep 14, 2023 · 11 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@Slijkhuis
Copy link

What is the current recommended approach for file upload?

Let's say there's an endpoint POST /photos that allows you to upload a file using a multipart-form request. I think it would be amazing if you could do something like:

meta {
  name: Upload photo
  type: http
  seq: 1
}

post {
  url: {{baseUrl}}/photos
  body: multipartForm
}

script:pre-request {
  bru.setVar("image", readCollectionFile("example.jpg"));
}

body:multipart-form {
  title: my photo
  file: {{image}}
}

Where readCollectionFile (just an example name) would be a built-in function to read a file relative to root folder of the collection. It could simply wrap fs.readFileSync.

Even better would be to be able to reference a file directly in the body:multipart-form block without requiring a script tag and setting a variable. But I'm not sure how that would look like since there can only be dictionary or text blocks right? Maybe the variable syntax could be used for a function call:

body:multipart-form {
  title: my photo
  file: {{readCollectionFile("example.jpg")}}
}

The problem with setting a variable is that the UI currently shows the variables as text when you hover over the eye-icon in the right-top corner. Having image data in variables doesn't really render nicely, you won't be able to see the full variable list.

The problem with allowing function calls within the variable syntax is that when a variable is set from the response of a request, it's a security issue if then the next request might interpret the value as a function call instead of just text. I currently (= after reading the docs, I haven't read the source yet) don't understand how Bruno distinguishes between the literal string Greeting: {{hello}} (expected result: Greeting: {{hello}}) and the variable substitution Greeting: {{hello}} (expected result: Greeting: hi). (where variable hello is equal to hi). Is there any documentation about that? Maybe it could help me come up with a good syntax to reference files.

Is there something for files already that I missed in the docs? If not I will try to find time to create a PR, but then I'd like some consensus on the preferred approach beforehand.

@dot-bm
Copy link

dot-bm commented Sep 18, 2023

Yes, it will be great feature

@helloanoop
Copy link
Contributor

What is the current recommended approach for file upload?

Currently file upload support is not present. It'd be great to get a PR for this.

I haven't read the source yet) don't understand how Bruno distinguishes between the literal string Greeting: {{hello}} (expected result: Greeting: {{hello}}) and the variable substitution Greeting: {{hello}} (expected result: Greeting: hi). (where variable hello is equal to hi)

I actually haven't tried this (since it is a highly unlikely corner case). We use MustacheJS under the hood for interpolating variables. You can check the implementation

On the syntax support for supporting file uploads, I would propose calling it as @file
We can also support @files later

body:multipart-form {
  name: photo
  img: @file("example.jpg")
}


body:multipart-form {
  name: photo
  img: @files("example1.jpg", example2.jpg")
}

It would be a built-in function to read a file relative to root folder of the collection.

I think we should support both absolute and relative paths here. If the path is relative, then as you suggested - the it can be read relative to the to root folder of the collection.

@Slijkhuis If you concur with the approach, the changes need to support this would have to be done here.
I would do something like check if the value begins with @file, parse the file name and do a fs.readFileSync and attach it to the axios request

The UI changes to make it a smoother ux experience would be a tad bit harder.
I am thinking of something like
image

@helloanoop helloanoop added good first issue Good for newcomers help wanted Extra attention is needed labels Sep 19, 2023
@mkdillard
Copy link

mkdillard commented Sep 30, 2023

I made an attempt at implementing support for this in Pull Request #258. I feel like there is probably more work to do, but the initial implementation works in the manual testing I've done locally with text and png files.

@helloanoop
Copy link
Contributor

@ajaishankar

Do you have any feedback on the DSL

body:multipart-form {
  name: photo
  img: @file("example.jpg")
}


body:multipart-form {
  name: photo
  img: @files("example1.jpg", example2.jpg")
}

@mkdillard
Copy link

For what it's worth, my implementation didn't include quotes for each path as it was additional parsing I wanted to avoid. If we want each file or path to be quoted that's a change that will have to be made.

@helloanoop
Copy link
Contributor

Until this PR gets merged. If someone is blocked, please see if fs access inside scripting can be a temporary solution

#306

@helloanoop
Copy link
Contributor

If you are willing to get your hand's dirty, please use this script to upload files.

I know it's not optimal and the best ux.

I would like to take some time to think through our choice of using @file() annotation. Because once it makes into bru lang, it becomes hard to change it later.

@ajaishankar has some DSL inputs in #344 which I am thinking through.

@AienTech
Copy link

AienTech commented Oct 5, 2023

We just started testing bruno, and this very feature is quite crucial for us. We had to unfortunately move to insomnia for now. I hope we see the feature coming to bruno ui soon.

@laucourt
Copy link

This option is essential !

@peter65374
Copy link

We just started testing bruno, and this very feature is quite crucial for us. We had to unfortunately move to insomnia for now. I hope we see the feature coming to bruno ui soon.

Yes, first try on bruno and block me on file uploading with multipart form, which is widely used in REST API i think. Wish the feature is added soon.

@richardwcollins
Copy link

Another request for multipart-form - highly critical for testing on multiple endpoints for us

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

8 participants