Merge lp://staging/~hingo/drizzle/drizzle-js_eval into lp://staging/~drizzle-trunk/drizzle/development

Proposed by Henrik Ingo
Status: Merged
Approved by: Brian Aker
Approved revision: 2385
Merged at revision: 2439
Proposed branch: lp://staging/~hingo/drizzle/drizzle-js_eval
Merge into: lp://staging/~drizzle-trunk/drizzle/development
Diff against target: 877 lines (+747/-9)
10 files modified
configure.ac (+1/-1)
drizzled/error.cc (+3/-0)
drizzled/error_t.h (+2/-1)
drizzled/temporal.h (+7/-7)
m4/pandora_have_libv8.m4 (+53/-0)
plugin/js/docs/index.rst (+230/-0)
plugin/js/js.cc (+371/-0)
plugin/js/plugin.ini (+10/-0)
plugin/js/tests/r/js.result (+35/-0)
plugin/js/tests/t/js.test (+35/-0)
To merge this branch: bzr merge lp://staging/~hingo/drizzle/drizzle-js_eval
Reviewer Review Type Date Requested Status
Drizzle Merge Team Pending
Review via email: mp+76674@code.staging.launchpad.net

Description of the change

Hi

I've written a plugin that embeds the v8 javascript engine and exposes it as a function JS(<javascript code snippet>, <arg1>, <arg2>, ...). The code is now feature complete for the first iteration. It still lacks test cases and documentation. I intend to complete those before this is merged, but I'm sending this to ask for a first review at this stage. This is my first Drizzle code I've ever written, so getting feedback at this stage would be appreciated. So please note, I'm asking for review but not yet asking to merge this.

The primary reason to do this is to get a JSON parser + ability to manipulate and return the JSON documents. This is analogous to the MySQL functions ExtractValue() and UpdateXML() for XML strings, there just isn't a counterpart to XPath in JSON. The side effect of being able to execute arbitrary Javascript inside Drizzle is cool, and can be built upon in the future. Currently there is no Drizzle functionality available inside the javascript environment, for instance you couldn't query any tables or do anything else you typically do with stored procedures.

The code comments still contain some todos which are more like questions - things I'd like to learn about Drizzle internals. If the reviewer can answer those in feedback via drizzle-discuss, that would be great.

The code comments also contain todos which I don't intend to fix in the first version, they are mostly performance work in particular related to the fact that v8 is very single threaded (Chrome browser has a separate browser and separate v8 instance for each tab). The intent is to focus short term on feature completeness and leave the performance work for later. (The current code will not degrade Drizzle performance in general, but it would not be a good idea to do a hundred simultaneous calls to JS().)

Usage examples can be seen in the comments to this blog post:
http://openlife.cc/blogs/2011/august/stored-procedures-javascript-my-drizzle-repository-can-do-it

Please note that the function is now called JS(...) and there is no JS_EVAL(...).

To post a comment you must log in.
2385. By Henrik Ingo

Added tests for JS().

Revision history for this message
Olaf van der Spek (olafvdspek) wrote :

Some style comments, I hope you don't mind.

206 +namespace drizzle_plugin {
207 +
208 +namespace js {

Redundant empty line

220 +class JsFunction :public Item_str_func

Missing space after ':'

221 +{
222 +public:
223 + JsFunction() :Item_str_func() {}
224 + ~JsFunction() {}

Empty constructors and destructors aren't necessary.

260 +void emit_drizzle_error(v8::TryCatch* try_catch) {

'{' should be on the next line

Greetings,

Olaf

2386. By Henrik Ingo

Add 2 tests I forgot:
 - connect from 2 clients to make sure we are handling multi threaded
   mode correctly.
 - run a script that gives JavaScript syntax error.

2387. By Henrik Ingo

Wrote end user documentation for JS().
Labeled this as version 0.9.
This is now ready to start the journey towards trunk!

2388. By Henrik Ingo

Small style fixes from Olaf's review (thanks).
Removed @todo items that have been done.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.