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 multipart content type #30

Merged
merged 4 commits into from
Nov 12, 2024
Merged

Conversation

jellejurre
Copy link
Contributor

No description provided.

@jellejurre
Copy link
Contributor Author

This is required for vrchatapi/specification#408 to be merged

Copy link
Collaborator

@C0D3-M4513R C0D3-M4513R left a comment

Choose a reason for hiding this comment

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

The sed statement changes nothing for me on the master branch.
Even grep "TODO: support file upload for" src/apis/files_api.rs comes back with nothing.

Did you want this to be a Draft PR?

@jellejurre
Copy link
Contributor Author

That's corrcect, it only changes stuff from the following pr:

This is required for vrchatapi/specification#408 to be merged

@C0D3-M4513R
Copy link
Collaborator

Okay, but then the documentation of the specification is wrong.
In the specification you write:

                file:
                  type: string
                  format: binary
                  description: The binary blob of the png file.

Yet in this pr you interpret file as a filename to open and read

@jellejurre
Copy link
Contributor Author

Yes, that's how the generators create it, the generators create a PathBuf, which (as I understand it) store a Path to a file on disk

@C0D3-M4513R
Copy link
Collaborator

Then the description should read The path to the file to be uploaded instead.

@C0D3-M4513R
Copy link
Collaborator

C0D3-M4513R commented Nov 12, 2024

Also using a blocking file read in an async context is not ideal either in this scenario (though I can totally understand that choice)

@jellejurre
Copy link
Contributor Author

Then the description should read The path to the file to be uploaded instead.

The issue with this is that it depends on the generator :)

The C# generator takes in a byte stream

@jellejurre
Copy link
Contributor Author

Also using a blocking file read in an async context is not ideal either in this scenario (though I can totally understand that choice)

Honestly the choice was because I don't know almost anything about rust async, I didnt realise that the file reading was sync, I can see if I can turn it into async in a bit

@C0D3-M4513R
Copy link
Collaborator

I can see if I can turn it into async in a bit

With the current dependencies std it's imo not possible.
We'll have to pull in some helper crate(s). Something like async-std or tokio (though I think the first choice is more versatile for consumers)

@jellejurre
Copy link
Contributor Author

Ah

Well realistically the images won't be too big so do we think that's worth it?

I feel like it might be fine like this

@C0D3-M4513R C0D3-M4513R self-requested a review November 12, 2024 11:55
@C0D3-M4513R
Copy link
Collaborator

Oh also you use .unwrap(), which is 100% off-limits for us as a crate.

@jellejurre
Copy link
Contributor Author

Yeah that's fair lol

@C0D3-M4513R
Copy link
Collaborator

@jellejurre Can you look at these changes?

@C0D3-M4513R C0D3-M4513R force-pushed the fix/add-multipart-contenttype branch from d22df3b to fa7b449 Compare November 12, 2024 12:41
@C0D3-M4513R
Copy link
Collaborator

C0D3-M4513R commented Nov 12, 2024

Wait, now that I'm thinking about it again, couldn't we just patch the sdk and have the user supply s byte stream and mime type?

Is that a better approach?

@jellejurre
Copy link
Contributor Author

Looks good to me, works and gives a proper error when handing a non-existent file

@jellejurre
Copy link
Contributor Author

Wait, now that I'm thinking about it again, couldn't we just patch the sdk and have the user supply s byte stream and mime type?

Is that a better approach?

Also possible, fine by me and probably more extendable in the long run

@C0D3-M4513R
Copy link
Collaborator

Writing a regex replace in regex101.com for that is no problem, but I can't get the regex to actually work.

@jellejurre
Copy link
Contributor Author

honestly, throw the regex in chatgpt, it's great at regexes lol

Copy link
Collaborator

@C0D3-M4513R C0D3-M4513R left a comment

Choose a reason for hiding this comment

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

I finally got the regex.
I couldn't use sed, because it isn't powerful enough.

@C0D3-M4513R
Copy link
Collaborator

Only thing left is to test this again and then to merge (if you have perms, feel free to merge yourself. If not, I'll merge it, when you said, that it works)

@jellejurre
Copy link
Contributor Author

let mut buffer = Vec::new();
let a = File::open("./image.png").unwrap().read_to_end(&mut buffer);
let output = vrchatapi::apis::files_api::upload_icon(&config, buffer, "image.png", "image/png")
    .await
    .unwrap();

this works great, LGTM

@jellejurre jellejurre merged commit 402dddb into main Nov 12, 2024
3 checks passed
@jellejurre jellejurre deleted the fix/add-multipart-contenttype branch November 12, 2024 15:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants