search

samvera / rubydora

Fedora Commons version 3 REST API ruby library
Other
27 stars 17 forks source link

Patches Rubydora::RestApiClient to not read file-like object #106

Closed dchandekstark closed 7 years ago

dchandekstark commented 7 years ago

When adding or modifying datastream content.

Note: Previously, rubydora read the entire file content into a string and passed the string to rest-client, so the file remained open. Since rest-client closes the file, the changed methods alter the state of passed in file/io objects.

awead commented 7 years ago

@dchandekstark seems fine to me, but is this a breaking change? Are existing users expecting the file or file-like object to be re-wound? Do we need to "be kind and rewind"?

dchandekstark commented 7 years ago

I don't know if it would be considered a breaking change or not. If it is, release 2.0.0! The file doesn't need to be rewound b/c it's closed. It would seem unusual to me to expect to be able to read a file submitted to be persisted, but I wouldn't argue the point. As I mentioned, the spec test did read the file after, but I'm not sure if that qualifies as a promise.

dchandekstark commented 7 years ago

FWIW, in case it's not obvious, this is to handle large files better. Reading a multi-GB file into a string doesn't work very well.

dchandekstark commented 7 years ago

@awead So you approve? Want someone else to weigh in?

awead commented 7 years ago

@dchandekstark we can wait the requisite 24 hours for others to weigh in. After that... :shipit:

dchandekstark commented 7 years ago

Pinging reviewers for input. Also, any opinions on version number bump for release?

jcoyne commented 7 years ago

This reverts the change I did here: https://github.com/projecthydra/rubydora/commit/f1e8d169d60911885ff21459b988c1d9418981c5 I don't remember why I did this as it was several years back, though presumably I had reasons. I can see why one would not want to read a big file into memory though. I haven't used FC3 for a couple of years, so I defer to @cbeer who knows about Stanford's use cases.

jcoyne commented 7 years ago

@dchandekstark If you were able to point ActiveFedora 8.3 at this branch of rubydora and run all the tests it would make me feel a lot better about this.

dchandekstark commented 7 years ago

@jcoyne Fair enough, will do.

dchandekstark commented 7 years ago

@jcoyne AF 8.3.0 test suite results:

Finished in 52.1 seconds (files took 1.34 seconds to load) 859 examples, 0 failures, 1 pending

jcoyne commented 7 years ago

@dchandekstark thanks for the extra reassurance. Would you mind releasing this as a major version bump?