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

Allow uploading files from InputStreams and Files #544

Merged
merged 1 commit into from
Jul 3, 2018

Conversation

drake-stripe
Copy link
Contributor

@drake-stripe drake-stripe commented Jun 29, 2018

This allows users to create a FileUpload with either a File or a InputStream under the file parameter. It defaults to calling the uploaded file "blob".

#299

@brandur-stripe
Copy link
Contributor

@drake-stripe Can you take a look at the failed build before we find you a reviewer? It looks like checkstyle is failing:

> Task :checkstyleMain
[ant:checkstyle] [ERROR] /home/travis/build/stripe/stripe-java/src/main/java/com/stripe/net/MultipartProcessor.java:75: Line is longer than 100 characters (found 102). [LineLength]
> Task :checkstyleMain FAILED

Also, this is a bit of a nit, but those of us who work on these external projects tend to try to keep slightly better commit hygiene than what you might see internally. It might be slightly better to get into a workflow where you pull minor changes/fixes into a single commit with git commit --amend, or use git rebase -i to fix a series of them. It's not the end of the world either way because we can squash them down before merging here, but it does save us from having to strip all the "fix test", "fix error", etc. cruft from commit messages.

Thanks for taking a crack at this!

@ob-stripe
Copy link
Contributor

@drake-stripe Looks good overall, but you'll need to fix the style errors as Brandur mentioned above. You can run both the test suite and the linter locally with ./gradlew check (or ./gradlew checkstyleMain checkstyleTest if you just want to run the linter).

Also, could you add a test case in the functional test for FileUpload? You can copy the testCreate function into a new testCreateWithStream case.

@ob-stripe ob-stripe assigned drake-stripe and unassigned ob-stripe Jul 2, 2018
@drake-stripe drake-stripe force-pushed the drake-implement-InputStream-upload branch from 797e0f7 to 151ea9b Compare July 2, 2018 20:16
@drake-stripe
Copy link
Contributor Author

PR now passes linter, and there is a functional test implemented for InputStream uploading. I also squashed it into one commit with a more descriptive commit message.

+ "\"; filename=\"" + fileName + "\"").append(
LINE_BREAK);
addFileField(name, file.getName(), new FileInputStream(file));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor, but since these methods are meant to be internal anyway, can we just get rid of the File form of addFileField since it's only used in one place anyway? If other places in the library need to do this, they can instantiate their own FileInputStream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@brandur-stripe
Copy link
Contributor

Left a minor comment, but thanks a lot for fixing all that stuff Drake!

(Olivier, you're a lot better learned in Java than I am, so I'll leave a final review to you.)

@drake-stripe drake-stripe force-pushed the drake-implement-InputStream-upload branch 2 times, most recently from c17bede to e9b3f4e Compare July 3, 2018 02:01
This commit adds in suppor for calling
FileUpload.create(Map<String, Object>) with the parameter "file" set to
an InputStream, instead of only allowing Files. You can see an example
of this in src/test/java/com/stripe/functional/FileUploadTest.java.

Furthermore, this commit changes the addFileField method to no longer
accept a file argument, and only allows for InputStream arguments in
addition to a string containing the name of the file to upload.
@drake-stripe drake-stripe force-pushed the drake-implement-InputStream-upload branch from e9b3f4e to 9247989 Compare July 3, 2018 02:03
@ob-stripe ob-stripe merged commit 319e0cd into master Jul 3, 2018
@ob-stripe ob-stripe deleted the drake-implement-InputStream-upload branch July 3, 2018 10:21
@ob-stripe
Copy link
Contributor

Released in 5.48.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants