Better support for uploading in-memory images

[praboud]

A fairly common use case for file uploads is to upload an image to Stripe that was just uploaded to the server, or to retrieve an image from a URL, then upload it to Stripe. As it stands, the file uploader seems to expect to be given a file stored on disk, which isn't really necessary. More specifically, the file uploader wants something that has a .read method, and a .name method.

Right now, if I take a StringIO (which supports .read), then monkey-patch it to have .name="foo", this seems to upload fine, and I more or less get the thing I want. This is a bit gross, though. Ideally, we wouldn't require a file name for the file at all, since this doesn't appear to be getting used in any meaningful way. (The upload response or anything else I can find doesn't seem to make reference to the filename.)

Though it doesn't seem like uploads.stripe.com actually does anything with the file name, it does get unhappy if the parameter is omitted or left blank. As far as I can tell, if I patch stripe-python to just pass a hard-coded dummy value as the file name, this works fine. This is clearly a hack, though. In an ideal world, uploadsrv wouldn't look for a filename at all, though I don't really know if that would be a simple change or not.

Thoughts on whether this is something we'd want to support, and if so, what the cleanest way of doing this is?

[brandur]

Hi @praboud, thanks for dropping in :)

I just took a quick look at this, and it looks like although the name of the file is used here in order to add multipart's filename parameter according to RFC2388, as indicated by that same, providing that parameter is optional and not really necessary.

In addition, I just took a peek at the backend implementation, and the paths were using to persist files are entirely based off a generated token identifier rather than any kind of user-specified input.

So for what it's worth, +1 on the approach of using some kind of reader pattern like a StringIO rather than using a file as the basic abstraction here; it's more generic, more "correct", and might allow some users to skip a step in the upload process as in the example that you brought up above. It looks like all or most of the other bindings are doing something similar to Python, and could probably all stand to change in this respect.

(We may not get to this right away, but I'm happy to tag this "future" and leave the issue open.)

[praboud]

So it sounds like the path forwards would be to change uploadsrv to not require filename as part of the request (this should be transparent to existing clients), then stop posting filename in stripe-python so that StringIO and friends work. I'm happy to contribute the patch for stripe-python (and the rest of the bindings), though obviously I can't make the serverside changes. I'll tag this as future for now, then.

Edit: oh right, I can't tag issues because I'm not a contributor.

[brandur]

So it sounds like the path forwards would be to change uploadsrv to not require filename as part of the request (this should be transparent to existing clients), then stop posting filename in stripe-python so that StringIO and friends work. I'm happy to contribute the patch for stripe-python (and the rest of the bindings), though obviously I can't make the serverside changes.

Hey! So I looked into this a bit, and our implementation doesn't seem to care as much about whether filename as whether Rack does. As far as I can tell, a blank filename triggers this conditional in Rack's multipart parser which leaves the blob as a just a big string (or possibly StringIO; admittedly I didn't look too carefully) in the parameter rather than the hash that's expected from the backend (and thus fails a validation).

We could change the backend to handle either case, but it might be easier just to send in a "symbolic" filename from the bindings instead. The difficulty of this will vary a bit by language, but should be relatively easy in Python where we have a custom multipart form implementation. Thoughts?

[praboud]

Hmm. I'd prefer to fix the backend to deal with the string case, myself. Passing filename=<some arbitrary value> in all of the clients seems pretty cludgy. It's also a bit of a gotcha for people that implement their own file upload code (particularly since the error message that's returned when filename is omitted is pretty cryptic).

[brandur]

Hmm. I'd prefer to fix the backend to deal with the string case, myself. Passing filename= in all of the clients seems pretty cludgy. It's also a bit of a gotcha for people that implement their own file upload code (particularly since the error message that's returned when filename is omitted is pretty cryptic).

Yeah, I tend to agree here, but TBH in this case it seems to be Rack which isn't behaving sanely (it should handle the omission of a filename much more elegantly given that the spec is very explicit in that clients are allowed to omit one), and I'm not sure I want to try and get in there and compensate for that. It's certainly possible, but a bit more of a yakshave than I think is merited for this sort of change.

[praboud]

Erg. I think I didn't quite grok the Rack parser code you linked to. Rereading it now, it seems like the content-type is not fed through if filename is omitted, which is going to be a pain to fix, since this is something uploadsrv presumably needs to have. I'm going to do a bit more digging, since I'd really like to avoid this, but passing dummy filenames may be the path of least resistance. :(

[brandur]

Erg. I think I didn't quite grok the Rack parser code you linked to. Rereading it now, it seems like the content-type is not fed through if filename is omitted, which is going to be a pain to fix, since this is something uploadsrv presumably needs to have. I'm going to do a bit more digging, since I'd really like to avoid this, but passing dummy filenames may be the path of least resistance. :(

Cool :) Let me know if you find something. I'm definitely interested in helping to facilitate this improvement if we can find a way to do so that doesn't involve a huge amount of churn.

[praboud]

Actually, it looks like uploadsrv doesn't care about the content-type at all - MultipartDataGenerator just passes application/octet-stream for all data. I wasn't paying enough attention before when I assumed that this was a significant value. Presumably uploadsrv ignores this value anyway, and gets the image type from the header of the actual image data (which makes sense).

If that's the case, then rack parsing the request into a string (ie: no filename/content-type/etc) doesn't actually drop any information that uploadsrv cares about, so it shouldn't be hard to get uploadsrv to handle the string case. Unless I'm misreading things, that seems like the cleanest solution.

It's a bit more work than just passing dummy file names, but it does make the interface exposed by the actual HTTP API cleaner (and anecdotally, people do seem to like writing their own uploadsrv clients, so I'm +1 for this).

[praboud]

Separately, the rack multipart parser is a bit of a mess. I'm 99% sure that https://github.com/rack/rack/blob/4b8378b8d40b4529420ac0e8e00d1c14f88e77f7/lib/rack/multipart/parser.rb#L98 is unreachable, which confused me for a while.

[brandur]

@praboud Hey! Sorry, I was away on vacation for a few weeks and totally dropped the ball on this one.

+1 that working around Rack's input does seem like a cleaner solution overall, and bindings in other languages could probably benefit from th change as well. You're back next month aren't you? Any interest in picking that project back up then? ;)

[praboud]

No worries!

I'd be happy to take that one when I get back.

Relatedly, when I opened this issue, I hadn't realized that the api supports file uploads from a publishable key. This seems like a strictly better alternative to the pass-through case (which was my main reason for opening this issue). While it's probably still worthwhile to clean up the whole filename thing with uploadsrv, I think it's also worth changing the documentation to better advertise that publishable key file uploads are a thing, and to point people towards that approach. (I think I discussed this with @olivierbellone out of band a while ago - did this get logged as an issue somewhere?)

[brandur]

I'd be happy to take that one when I get back.

Awesome :)

Relatedly, when I opened this issue, I hadn't realized that the api supports file uploads from a publishable key. This seems like a strictly better alternative to the pass-through case (which was my main reason for opening this issue). While it's probably still worthwhile to clean up the whole filename thing with uploadsrv, I think it's also worth changing the documentation to better advertise that publishable key file uploads are a thing, and to point people towards that approach. (I think I discussed this with @olivierbellone out of band a while ago - did this get logged as an issue somewhere?)

TIL! That is really handy solution. +1 to trying to get this cleaned up regardless though. Aside from where it's being used, being able to pass a byte string (or IO reader) would be a more consistent fit for the APIs of some languages.

[olivierbellone]

(I think I discussed this with @olivierbellone out of band a while ago - did this get logged as an issue somewhere?)

Yep. For now, a short paragraph has been added here, in the aside box on the right, so users know that this is a possibility. We could improve this by providing a full-fledged example -- I'll try to tackle this one of these days.

[praboud]

@olivierbellone fantastic! +1

[brandur]

Cool! Alright, I'll leave this open for the time being. Once Peter is back, we can figure out whether we should convert this into an internal ticket.