imrefazekas / connect-rest

Exceptionally featureful Restful web services middleware for connect node.js
MIT License
100 stars 29 forks source link

return binary file content #13

Closed pussinboots closed 10 years ago

pussinboots commented 10 years ago

Hello,

i found your great rest framework and want to use to return binary file content instead of json or xml. Is that supported now by return a filestream or file object?

great work so far

pussinboots commented 10 years ago

I get it working by reading complete file and set the appropiate header, but there a way to use the stream capabilities of the nodejs connect web server.

Project can be found here https://github.com/pussinboots/heroku-softcover

imrefazekas commented 10 years ago

Hello,

You are right, no feature for streams. Good idea, thanks! Give me a few hours to figure out something and make a new release. To be honest, the best would be to take advance of streams in node, but the concept of REST must be intact, so user must be informed about any reading issues on REST-way, so I might seem to be forced to read all stream into a buffer and send it back.

I'm working on a new feature on that area anyhow, so good catch, thanks again!

imrefazekas commented 10 years ago

Made a release. You can use readable streams now as follows:

rest.get('/handlers/stream', function( request, content, callback ){
    return callback(null, fs.createReadStream( './test/data/answer.text', { encoding : 'utf-8'} ) );
});

Thanks again to give new ideas to extend this lib!

pussinboots commented 10 years ago

Thanks for the fast replay and the new implemented feature will use it immediatly.

imrefazekas commented 10 years ago

Let me know if there is anything I can/should add! :)

pussinboots commented 10 years ago

Thanks for the fast reply,

but the actual binary support not satisfy my needs because it doesn't really use streaming. That broke the resulted binary data. Good news follow your implementation i get it to work as aspects by changing the isReadableStream code.

function finalResult(err, result, resOptions, contentType, callback){ .... if( isReadableStream(result) ){ return callback( err, result, resOptions ); } ..... } ...

function processRequest(req, res, matching, bodyObj){ .... if( err ){ res.statusCode = err.statusCode || 500; res.end( 'Error occurred: ' + err ); } else if( isReadableStream(result.result) ){ var headers = result.resOptions.headers || { }; if( !headers['Content-Type'] ) headers['Content-Type'] = result.contentType || 'application/json'; res.writeHead( result.resOptions.statusCode || 200, headers ); result.result.on('error',function(msg){ res.end("" + msg); }); result.result.pipe(res); } ... }

With that patch above it return chunked data that are not broken should i prepare a Pull Request ? But the patch i guess it is not complete maybe you got the way i want do realize it. Use the esponse.pipe method instead of res.write string data.

Thanks for your attention.

imrefazekas commented 10 years ago

Could you write a test case when it does not work? Pipe is a dangerous case which I wanted to avoid because if piping fails, there is no real chance to give back correct status code and answer as REST requires.

pussinboots commented 10 years ago

A simple error handling can be achieved with may this following code result.result.on('error',function(msg){ res.writeHead(404); res.end("" + msg); });

imrefazekas commented 10 years ago

Please test it. Version 1.2.1 My mocha tests are running, but yours are also needed.

Thanks!

pussinboots commented 10 years ago

Okay i test it now.

pussinboots commented 10 years ago

Okay it seem's almost alright but the headers ar not set in success case.

i gues the following line is missing. res.writeHead( result.resOptions.statusCode || 200, headers ); result.result.pipe( res );

imrefazekas commented 10 years ago

Correct, will fix as soon as I arrive home, or feel free to issue a pull request.

Thanks!

pussinboots commented 10 years ago

I upload a project that conatins test for the expected behavoir.

https://github.com/pussinboots/connect-rest-binary

pussinboots commented 10 years ago

Added pull request https://github.com/imrefazekas/connect-rest/pull/14.