albertosantini / node-rio

Integration with Rserve, a TCP/IP server for R framework
https://github.com/albertosantini/node-conpa
MIT License
176 stars 35 forks source link

bufferAndEval Additional function #10

Closed ftrianakast closed 10 years ago

ftrianakast commented 10 years ago

I don't know if this is important, but locally I added a function that take a buffer and evaluate the script in the buffer.

I think that this is important when you have your R scripts in cloud, for example: Amazon S3 or Dropbox. The function is very similar to sourceAndEval. My fast local implementation:

function bufferAndEval(buffer, options) { //cmd = buffer.replace(/(\r\n)/g, "\n"); var cmd = buffer; if (options.entryPoint) { cmd += "\n" + options.entryPoint + "("; if (options.data) { cmd += "'" + JSON.stringify(options.data) + "')"; } else { cmd += ")"; } } evaluate(cmd, options); } exports.bufferAndEval = bufferAndEval;

Rally amazing your library. Thanks for all

albertosantini commented 10 years ago

Thanks for the feedback.

Nice suggestion.

Further we may add gistAndEval accepting a gist url. :) We may modify sourceAndEval accepting a string, a buffer or an url.

Anyway we are talking about injecting the source code from a remote endpoint.

Finally our distributed stack:

Maybe a note about the security between machine A and machine B is mandatory. Malware code could be contained in those remote R scripts, potentially impacting on machine C.

Initially I thought sourceAndEval as an API to load and to eval a script located on the same Rserve machine. Anyway it was too easy reusing evaluate after loading the content of a file.

You are suggesting the next step: distributing the source code.

I was wondering why it is not enough evaluate and why bufferAndEval does not contain the implementation to retrieve the remote script as in a hypothetical gistAndSource.

Is it not possible to extract a string from the Buffer after you retrieved it from the remote machine?

Maybe I am missing something. Any thoughts?

ftrianakast commented 10 years ago

If I understood, the idea is generalize the sourceAndEval function to accept any source. So my first thought is that for some kind of sources (Dropbox or Amazon) need specific adapters for the conection and recovery of the file in a buffer. I believe in the case of Gist the adapter is more simple that in the case of Dropbox or Amazon. In summary I believe that sourceAndEval need and additional parameter that specify the source (maybe a string 's3, dropbox, gist, etc') and an additional parameter (maybe a JSON) with the specific connection needs depending on source. For example a posible implementation of an Amazon adapter could use knox and expose a JavaScript 'class' similar to this class:

Snippet Please check the gist.

So you will need instantiate the S3Utilities "class" and then call the method getBufferFile. Please pay attention that I always enclose in quotes "class" because the class concept in JavaScript is forced. Maybe my solution is not the best, by nowadays it works.

I look forward

albertosantini commented 10 years ago

At the moment it would be strange to include in node-rio a Dropbox or Amazon adapter. Later I think we can create for release 2.x a support for a plugin approach.

Likely your bufferAndEval may be the minimum cross implementation to include.

If it suits to your needs, I would addbufferAndEval to accept a string buffer and an url.

Recap.

Do you see any overhead between bufferAndEval (forget for a while url detail) and evaluate?

If you have the following snippet:

# mySnippet

require(RJSONIO)
main <- function(jsonObj) {
    o = fromJSON(jsonObj)
    res = doSomething(o)
    return(toJSON(res))
}

I may call it using evaluate after loading mySnippet

rio.evaluate(mySnippet + "\nmain(" + JSON.stringify(myParams) + ')\n')

I mean, bufferAndEval would be only a method helper, compared with sourceAndEval, to simplify one line code of evaluate.

If you are aware about this and you have not any thoughts, I would proceed to add a refactor of bufferAndEval. :)

ftrianakast commented 10 years ago

Ok I agree.

Thank you for the attention. In future will be interesting add support to other sources.

Regards.

albertosantini commented 10 years ago

Done: https://github.com/albertosantini/node-rio/commit/11af1cbf0865622547afa402dfca212b700517cb

If the changes suit to your needs close the issue and I will prepare a new npm release.