Skip to content
This repository has been archived by the owner on Oct 30, 2018. It is now read-only.

Fix for issue #865: composite addon error manipulation #978

Merged
merged 6 commits into from Feb 20, 2013
Merged

Fix for issue #865: composite addon error manipulation #978

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8b2e067
adding basic tests for default failures in composite
caridy Feb 12, 2013
0bc42bb
introducing propagateFailure flag for childs in composite addon to de…
caridy Feb 12, 2013
053491f
Merge branch 'develop' of github.com:yahoo/mojito into composite-errors
caridy Feb 15, 2013
66ce33d
refactor to simplify the code and allow others to leverage ac.composi…
caridy Feb 16, 2013
ede0823
Merge branch 'develop' of github.com:yahoo/mojito into composite-errors
caridy Feb 20, 2013
81223a7
implementing feedback from review.
caridy Feb 20, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
298 changes: 149 additions & 149 deletions lib/app/addons/ac/composite.common.js
View file
Open in desktop
Expand Up @@ -12,39 +12,34 @@
/**
* @module ActionContextAddon
*/
YUI.add('mojito-composite-addon', function(Y, NAME) {
YUI.add('mojito-composite-addon', function (Y, NAME) {

function AdapterBuffer(buffer, id, callback) {
this.buffer = buffer;
function AdapterBuffer(id, callback) {
this.id = id;
this.callback = callback;
this.__meta = [];
this.callback = function () {
if (callback) {
callback.apply(this, arguments);
}
callback = function () {
Y.log('ac.done/flush/error called multiple times in child: ' +
id, 'warn', NAME);
};
};
}


AdapterBuffer.prototype = {

done: function(data, meta) {
// HookSystem::StartBlock
Y.mojito.hooks.hook('adapterBuffer', this.hook, 'end', this);
// HookSystem::EndBlock
this.buffer[this.id].meta = meta;
this.buffer[this.id].data += data;
this.buffer.__counter__ -= 1;
if (this.buffer.__counter__ === 0) {
// TODO: Check why this can be called more than once.
this.callback();
}
done: function (data, meta) {
this.callback(null, data, meta);
},


flush: function(data, meta) {
this.buffer[this.id].meta = meta;
this.buffer[this.id].data += data;
flush: function (data, meta) {
// TODO: we do not support flush at the composite level
// what else can we do?
this.callback(null, data, meta);
},


error: function(err) {
error: function (err) {
Y.log("Error executing child mojit at '" + this.id + "':", 'error',
NAME);
if (err.message) {
Expand All @@ -55,11 +50,11 @@ YUI.add('mojito-composite-addon', function(Y, NAME) {
if (err.stack) {
Y.log(err.stack, 'error', NAME);
}
// Pass back some empty data so we don't fail the composite
this.done('');

this.callback(err);
}
};

};

/**
* <strong>Access point:</strong> <em>ac.composite.*</em>
Expand All @@ -71,9 +66,11 @@ YUI.add('mojito-composite-addon', function(Y, NAME) {
this.dispatch = ac._dispatch;
this.ac = ac;
this.adapter = adapter;
this.queue = new Y.Parallel({
context: this
});
}


Addon.prototype = {

namespace: 'composite',
Expand Down Expand Up @@ -132,7 +129,7 @@ Y.mojito.controller = {
* @param {object} templateData The data you want return by the request.
* @param {object} parentMeta Any meta-data required to service the request.
*/
done: function(templateData, parentMeta) {
done: function (templateData, parentMeta) {
var ac = this.ac,
cfg = this.command.instance.config,
children = cfg.children;
Expand All @@ -153,7 +150,8 @@ Y.mojito.controller = {
' composite mojit spec.');
}

this.execute(cfg, function(data, meta) {
this.execute(cfg, function (data, meta) {

parentMeta.assets = Y.mojito.util.metaMerge(
parentMeta.assets || {},
meta.assets || {}
Expand Down Expand Up @@ -212,15 +210,14 @@ callback({
* @param {object} cfg The configuration object to be used.
* @param {function} cb The callback that will be called.
*/
execute: function(cfg, cb) {
execute: function (cfg, cb) {

var ac = this.ac,
buffer = {},
content = {},
my = this,
meta = {};

cfg.children = cfg.children || {};
meta = {},
children = cfg.children || {},
child;

// check to ensure children is an Object, not an array
if (Y.Lang.isArray(cfg.children)) {
Expand All @@ -232,147 +229,150 @@ callback({
Y.mojito.hooks.hook('addon', this.adapter.hook, 'start', my, cfg);
// HookSystem::EndBlock

meta.children = cfg.children;

buffer.__counter__ = Y.Object.size(cfg.children);

this._dispatchChildren(cfg.children, this.command, buffer,
function() {
var name;
// Reference the data we want from the "buffer" into our
// "content" obj Also merge the meta we collected.
for (name in buffer) {
if (buffer.hasOwnProperty(name) &&
name !== '__counter__') {
content[name] = buffer[name].data || '';

if (buffer[name].meta) {
meta = Y.mojito.util.metaMerge(meta,
buffer[name].meta);
}
}
}
meta.children = children;

for (child in children) {
if (children.hasOwnProperty(child)) {
this.add(child, children[child]);
}
}

this.queue.done(function (buffer) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we rename the buffer argument to results or childResults or something like that? buffer isn't very descriptive.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... perhaps it might be clearer to use this.queue.results instead of buffer.

var i;

// Mix in the assets given via the config
if (cfg.assets) {
if (!meta.assets) {
meta.assets = {};
}
ac.assets.mixAssets(meta.assets, cfg.assets);
// HookSystem::StartBlock
Y.mojito.hooks.hook('addon', my.adapter.hook, 'end', my);
// HookSystem::EndBlock

if (my.queue.skip) {
// skiping due to an error during queue process
return;
}

// Reference the data we want from the "buffer" into our
// "content" obj Also merge the meta we collected.
for (i = 0; i < buffer.length; i += 1) {
content[buffer[i].name] = buffer[i].data;
if (buffer[i].meta) {
meta = Y.mojito.util.metaMerge(meta,
buffer[i].meta);
}
}

// HookSystem::StartBlock
Y.mojito.hooks.hook('addon', my.adapter.hook, 'end', my);
// HookSystem::EndBlock
// Mix in the assets given via the config
if (cfg.assets) {
if (!meta.assets) {
meta.assets = {};
}
ac.assets.mixAssets(meta.assets, cfg.assets);
}

cb(content, meta);
});
cb(content, meta);
});
},

abort: function (err) {
var adapter = this.adapter;
// an error occur in a child marked with propagateFailure flag
adapter.error(err);
// cleaning up the house
this.queue.results = [];
Copy link
Contributor

Choose a reason for hiding this comment

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

I understand why we're doing this, but I don't think this is the best approach. Perhaps instead we can do this.queue = null if that works.

In fact, we should be clearing out this.queue regardless of whether we succeed or fail.

// cancel any pending job by invalidating the callback of the queue
this.queue.skip = true;
Copy link
Contributor

Choose a reason for hiding this comment

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

The skip property doesn't appear to be part of the Parallel API. Can we do this.skip instead?

// invalidate this.adapter.done/error
adapter.error = adapter.done = function () {};
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if we can put do some useful logging in this replacement, instead of just no-op.

(The situation is made a little more confusing because ac.composite doesn't actually call this.adapter.done() itself, but instead calls the cb argument to ac.composite.execute(). However, we do call this.adapter.error() whenever a child fails and propagates that failure.)

},

_dispatchChildren: function(children, command, buffer, callback) {
//Y.log('_dispatchChildren()', 'debug', NAME);
add: function (childName, child) {

var childName,
child,
var originalChild = child,
my = this,
childAdapter,
newCommand,
name;

// Process deferred children before dispatching
for (name in children) {
if (children.hasOwnProperty(name)) {
child = children[name];

// check to ensure children doesn't have a null child
// in which case it will be automatically skipped to
// facilitate disabling children based on the context.
if (!child) {
continue;
}
newCommand;

// first off, check to see if this child's execution should be
// deferred
if (child.defer) {
// it doesn't make sense to have a deferred child with a
// proxy, because the defer means to proxy it
// through the LazyLoad mojit
if (Y.Lang.isObject(child.proxy)) {
throw new Error('Cannot specify a child mojit spec' +
' with both \'defer\' and \'proxy\'' +
' configurations, because \'defer\'' +
' assumes a \'proxy\' to the LazyLoad' +
' mojit.');
}
// aha! that means we will give it a proxy to the LazyLoad
// mojit, which will handle lazy execution on the client.
child.proxy = {
type: 'LazyLoad'
};
}
if (Y.Lang.isObject(child.proxy)) {
// found a proxy, replace the child with the proxy and shove
// the child to proxy into it
children[name] = child.proxy;
if (!children[name].config) {
children[name].config = {};
}
// remove any defer or proxy flags so it doesn't reload
// infinitely
child.proxy = undefined;
child.defer = false;
children[name].config.proxied = child;
}
// check to ensure children doesn't have a null child
// in which case it will be automatically skipped to
// facilitate disabling children based on the context.
if (!child) {
return;
}

// first off, check to see if this child's execution should be
// deferred
if (child.defer) {
// it doesn't make sense to have a deferred child with a
// proxy, because the defer means to proxy it
// through the LazyLoad mojit
if (Y.Lang.isObject(child.proxy)) {
throw new Error('Cannot specify a child mojit spec' +
' with both \'defer\' and \'proxy\'' +
' configurations, because \'defer\'' +
' assumes a \'proxy\' to the LazyLoad' +
' mojit.');
}
// aha! that means we will give it a proxy to the LazyLoad
// mojit, which will handle lazy execution on the client.
child.proxy = {
type: 'LazyLoad'
};
}
if (Y.Lang.isObject(child.proxy)) {
// found a proxy, replace the child with the proxy and shove
// the child to proxy into it
child = child.proxy;
child.config = child.config || {};
child.config.proxied = originalChild;
// remove any defer or proxy flags so it doesn't reload
// infinitely
originalChild.proxy = undefined;
originalChild.defer = false;
}

for (childName in children) {
if (children.hasOwnProperty(childName)) {
child = children[childName];
// Make a new "command" that works in the context of this
// composite
newCommand = {
instance: child,
// use action in child spec or default to index
action: child.action || 'index',
context: this.command.context,
params: child.params || this.command.params
};

// check to ensure children doesn't have a null child
// in which case it will be automatically skipped to
// facilitate disabling children based on the context.
if (!child) {
buffer.__counter__ -= 1;
continue;
}
childAdapter = new AdapterBuffer(childName,
this.queue.add(function (err, data, meta) {

// HookSystem::StartBlock
Y.mojito.hooks.hook('adapterBuffer', this.hook, 'end', this);
// HookSystem::EndBlock

// Create a buffer for the child
buffer[childName] = {name: childName, data: '', meta: {}};
if (err && originalChild.propagateFailure) {
my.abort(err);
return;
}

// Make a new "command" that works in the context of this
// composite
newCommand = {
instance: child,
// use action in child spec or default to index
action: child.action || 'index',
context: command.context,
params: child.params || command.params
};
return {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be nice to add a comment here that this datastructure is part of the buffer argument to the callback of this.queue.done. (It took me a little bit to figure this out, in part because the documentation for Y.Parallel is so thin.)

name: childName,
data: (data || ''),
meta: meta
}; // storing data and meta in queue

childAdapter = new AdapterBuffer(buffer, childName,
callback);
}));

// HookSystem::StartBlock
// piping the hook handler into the child mojits
childAdapter.hook = this.adapter.hook;
// HookSystem::EndBlock
// HookSystem::StartBlock
Y.mojito.hooks.hook('adapterBuffer', this.adapter.hook, 'start', childAdapter);
// HookSystem::EndBlock

// HookSystem::StartBlock
Y.mojito.hooks.hook('adapterBuffer', this.adapter.hook, 'start', childAdapter);
// HookSystem::EndBlock
childAdapter = Y.mix(childAdapter, this.adapter);
childAdapter = Y.mix(childAdapter, this.adapter);

this.dispatch(newCommand, childAdapter);
}
}
this.dispatch(newCommand, childAdapter);
}

};

Y.namespace('mojito.addons.ac').composite = Addon;

}, '0.1.0', {requires: [
'parallel',
'mojito',
'mojito-util',
'mojito-hooks',
Expand Down