Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with HTTPS or Subversion.

Download ZIP

Loading…

Break filter chain when redirect occurs #526

Merged
merged 4 commits into from

5 participants

Kristian PD Don't Add Me To Your Organization a.k.a The Travis Bot Harry Brundage Jason King Robert Mosolgo
Kristian PD
Owner

@hornairs @nciagra I'm looking to break filter chains if a redirect call is made within a beforeFilter to avoid continuing with remaining filters and avoid action from executing on the controller. This current solution is simply inspecting _afterFilterRedirect to see if a redirect has been requested.

I've noticed an issue which I believe may be related to pushState on the Navigator (possibly unrelated to this change) where the following scenario occurs:

class FoosController extends Batman.Controller
  @beforeFilter: (options) -> 
     @redirect @get('routes.foos.other')  if options.action is 'index'

  index: -> 
  show: ->
  other: -> 

If this is the first request to the application (i.e. a new browser session), a request for
/foos
will result in an address bar URL of
/foos/other as you'd expect.

If a successful dispatch has already occurred, say to
/foos/1
or any other controller action, a request to
/foos
will redirect and render accordingly, but the URL in the address bar will remain as
/foos
ignoring the redirected URL of
/foos/other
though a brief flicker indicates it may have been pushed to the Navigator in some manner

Thoughts about this approach so far or insights into the pushState issue?

Don't Add Me To Your Organization a.k.a The Travis Bot

This pull request passes (merged 86a309a into da0d145).

Harry Brundage
Collaborator

Is it really only redirects that should short circuit the rest of the dispatch? Ithink thats right. I have no insight into the pushState issue, sorry. I will look over the weekend.

src/controller/controller.coffee
@@ -77,7 +77,7 @@ class Batman.Controller extends Batman.Object
parentFrame = @_actionFrames[@_actionFrames.length - 1]
frame = new Batman.ControllerActionFrame {parentFrame, action}, =>
- @_runFilters action, params, 'afterFilters'
+ @_runFilters action, params, 'afterFilters' unless @_afterFilterRedirects
Harry Brundage Collaborator

Redirects? not Redirect?

Kristian PD Owner

Yup, i have this in a follow up commit inbound

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
src/controller/controller.coffee
@@ -87,8 +87,7 @@ class Batman.Controller extends Batman.Object
oldRedirect = Batman.navigator?.redirect
Batman.navigator?.redirect = @redirect
@_runFilters action, params, 'beforeFilters'
-
- result = @[action](params)
+ result = @[action](params) unless @_afterFilterRedirect
Harry Brundage Collaborator

Why not just return?

Kristian PD Owner

I didn't want to mess with the ControllerFrameAction or return result of the executeAction

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Jason King

Very interesting. We had our own redirect issues this week. I'm still working out how to write a good test for it, but we had a problem where if you had one redirect to a view, and another redirect after that view was displayed, then we ended up in an infinite loop because Batman.navigator.redirect pointed to the OurController.redirect

We discovered that oldRedirect - just above the code in this PR was "unravelling" in the wrong order, leaving the wrong value as the last value assigned to Batman.navigator.redirect

My code is here: https://github.com/amco/batman/compare/redirect_loop

Your tests just test your exact change, rather than the underlying URL problem. I wonder if your underlying problem is caused by the same thing my problem was?

Harry Brundage
Collaborator

LGTM :+1: :shipit:

Don't Add Me To Your Organization a.k.a The Travis Bot

This pull request passes (merged 9ffa1e4 into 5d1ff82).

Kristian PD kristianpd merged commit 4bad808 into from
Robert Mosolgo rmosolgo removed the Code Review label
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
36 lib/batman.js
View
@@ -6366,16 +6366,16 @@
var filters, options, _base;
Batman.initializeObject(this);
options = _optionsFromFilterArguments.apply(null, arguments);
- filters = (_base = this._batman).beforeFilters || (_base.beforeFilters = new Batman.SimpleHash);
- return filters.set(options.block, options);
+ filters = (_base = this._batman).beforeFilters || (_base.beforeFilters = []);
+ return filters.push(options);
};
Controller.afterFilter = function() {
var filters, options, _base;
Batman.initializeObject(this);
options = _optionsFromFilterArguments.apply(null, arguments);
- filters = (_base = this._batman).afterFilters || (_base.afterFilters = new Batman.SimpleHash);
- return filters.set(options.block, options);
+ filters = (_base = this._batman).afterFilters || (_base.afterFilters = []);
+ return filters.push(options);
};
Controller.afterFilter(function(params) {
@@ -6430,7 +6430,9 @@
action: action
}, function() {
var _ref;
- _this._runFilters(action, params, 'afterFilters');
+ if (!_this._afterFilterRedirect) {
+ _this._runFilters(action, params, 'afterFilters');
+ }
_this._resetActionFrames();
return (_ref = Batman.navigator) != null ? _ref.redirect = oldRedirect : void 0;
});
@@ -6443,7 +6445,9 @@
_ref1.redirect = this.redirect;
}
this._runFilters(action, params, 'beforeFilters');
- result = this[action](params);
+ if (!this._afterFilterRedirect) {
+ result = this[action](params);
+ }
if (!frame.operationOccurred) {
this.render();
}
@@ -6534,24 +6538,28 @@
};
Controller.prototype._runFilters = function(action, params, filters) {
- var _ref,
- _this = this;
+ var block, options, _i, _len, _ref;
if (filters = (_ref = this.constructor._batman) != null ? _ref.get(filters) : void 0) {
- return filters.forEach(function(_, options) {
- var block;
+ for (_i = 0, _len = filters.length; _i < _len; _i++) {
+ options = filters[_i];
if (options.only && __indexOf.call(options.only, action) < 0) {
- return;
+ continue;
}
if (options.except && __indexOf.call(options.except, action) >= 0) {
+ continue;
+ }
+ if (this._afterFilterRedirect) {
return;
}
block = options.block;
if (typeof block === 'function') {
- return block.call(_this, params);
+ block.call(this, params);
} else {
- return typeof _this[block] === "function" ? _this[block](params) : void 0;
+ if (typeof this[block] === "function") {
+ this[block](params);
+ }
}
- });
+ }
}
};
36 lib/dist/batman.node.js
View
@@ -6366,16 +6366,16 @@
var filters, options, _base;
Batman.initializeObject(this);
options = _optionsFromFilterArguments.apply(null, arguments);
- filters = (_base = this._batman).beforeFilters || (_base.beforeFilters = new Batman.SimpleHash);
- return filters.set(options.block, options);
+ filters = (_base = this._batman).beforeFilters || (_base.beforeFilters = []);
+ return filters.push(options);
};
Controller.afterFilter = function() {
var filters, options, _base;
Batman.initializeObject(this);
options = _optionsFromFilterArguments.apply(null, arguments);
- filters = (_base = this._batman).afterFilters || (_base.afterFilters = new Batman.SimpleHash);
- return filters.set(options.block, options);
+ filters = (_base = this._batman).afterFilters || (_base.afterFilters = []);
+ return filters.push(options);
};
Controller.afterFilter(function(params) {
@@ -6430,7 +6430,9 @@
action: action
}, function() {
var _ref;
- _this._runFilters(action, params, 'afterFilters');
+ if (!_this._afterFilterRedirect) {
+ _this._runFilters(action, params, 'afterFilters');
+ }
_this._resetActionFrames();
return (_ref = Batman.navigator) != null ? _ref.redirect = oldRedirect : void 0;
});
@@ -6443,7 +6445,9 @@
_ref1.redirect = this.redirect;
}
this._runFilters(action, params, 'beforeFilters');
- result = this[action](params);
+ if (!this._afterFilterRedirect) {
+ result = this[action](params);
+ }
if (!frame.operationOccurred) {
this.render();
}
@@ -6534,24 +6538,28 @@
};
Controller.prototype._runFilters = function(action, params, filters) {
- var _ref,
- _this = this;
+ var block, options, _i, _len, _ref;
if (filters = (_ref = this.constructor._batman) != null ? _ref.get(filters) : void 0) {
- return filters.forEach(function(_, options) {
- var block;
+ for (_i = 0, _len = filters.length; _i < _len; _i++) {
+ options = filters[_i];
if (options.only && __indexOf.call(options.only, action) < 0) {
- return;
+ continue;
}
if (options.except && __indexOf.call(options.except, action) >= 0) {
+ continue;
+ }
+ if (this._afterFilterRedirect) {
return;
}
block = options.block;
if (typeof block === 'function') {
- return block.call(_this, params);
+ block.call(this, params);
} else {
- return typeof _this[block] === "function" ? _this[block](params) : void 0;
+ if (typeof this[block] === "function") {
+ this[block](params);
+ }
}
- });
+ }
}
};
22 src/controller/controller.coffee
View
@@ -29,14 +29,14 @@ class Batman.Controller extends Batman.Object
@beforeFilter: ->
Batman.initializeObject @
options = _optionsFromFilterArguments(arguments...)
- filters = @_batman.beforeFilters ||= new Batman.SimpleHash
- filters.set(options.block, options)
+ filters = @_batman.beforeFilters ||= []
+ filters.push(options)
@afterFilter: ->
Batman.initializeObject @
options = _optionsFromFilterArguments(arguments...)
- filters = @_batman.afterFilters ||= new Batman.SimpleHash
- filters.set(options.block, options)
+ filters = @_batman.afterFilters ||= []
+ filters.push(options)
@afterFilter (params) ->
if @autoScrollToHash && params['#']?
@@ -77,7 +77,7 @@ class Batman.Controller extends Batman.Object
parentFrame = @_actionFrames[@_actionFrames.length - 1]
frame = new Batman.ControllerActionFrame {parentFrame, action}, =>
- @_runFilters action, params, 'afterFilters'
+ @_runFilters action, params, 'afterFilters' unless @_afterFilterRedirect
@_resetActionFrames()
Batman.navigator?.redirect = oldRedirect
@@ -85,10 +85,9 @@ class Batman.Controller extends Batman.Object
frame.startOperation({internal: true})
oldRedirect = Batman.navigator?.redirect
- Batman.navigator?.redirect = @redirect
+ Batman.navigator?.redirect = @redirect
@_runFilters action, params, 'beforeFilters'
-
- result = @[action](params)
+ result = @[action](params) unless @_afterFilterRedirect
@render() if not frame.operationOccurred
frame.finishOperation()
@@ -154,9 +153,10 @@ class Batman.Controller extends Batman.Object
_runFilters: (action, params, filters) ->
if filters = @constructor._batman?.get(filters)
- filters.forEach (_, options) =>
- return if options.only and action not in options.only
- return if options.except and action in options.except
+ for options in filters
+ continue if options.only and action not in options.only
+ continue if options.except and action in options.except
+ return if @_afterFilterRedirect
block = options.block
if typeof block is 'function' then block.call(@, params) else @[block]?(params)
95 tests/batman/controller/controller_test.coffee
View
@@ -259,6 +259,49 @@ test 'filters specifying options in arrays should apply to all/none of those opt
controller.dispatch 'index'
equal spy.callCount, 1
+test 'redirect() in beforeFilter halts chain and does not call action or render', 4, ->
+ beforeSpy1 = createSpy()
+ beforeSpy2 = createSpy()
+ renderSpy = createSpy()
+ afterSpy = createSpy()
+
+ class FilterController extends Batman.Controller
+ @beforeFilter beforeSpy1
+ @beforeFilter ->
+ @redirect '/'
+ @beforeFilter beforeSpy2
+ @afterFilter afterSpy
+
+ render: renderSpy
+ index: -> @render false
+
+ controller = new FilterController
+ controller.dispatch 'index'
+ equal beforeSpy1.callCount, 1
+ equal beforeSpy2.callCount, 0
+ equal renderSpy.callCount, 0
+ equal afterSpy.callCount, 0
+
+test 'redirect() in afterFilter halts chain', 3, ->
+ beforeSpy = createSpy()
+ afterSpy1 = createSpy()
+ afterSpy2 = createSpy()
+
+ class FilterController extends Batman.Controller
+ @beforeFilter beforeSpy
+ @afterFilter afterSpy1
+ @afterFilter ->
+ @redirect '/'
+ @afterFilter afterSpy2
+
+ index: -> @render false
+
+ controller = new FilterController
+ controller.dispatch 'index'
+ equal beforeSpy.callCount, 1
+ equal afterSpy1.callCount, 1
+ equal afterSpy2.callCount, 0
+
test 'actions executed by other actions implicitly render', ->
mockClassDuring Batman ,'View', MockView, (mockClass) =>
@controller.test = ->
@@ -289,6 +332,44 @@ test 'actions executed by other actions have their filters run', ->
ok beforeSpy.called
ok afterSpy.called
+test 'beforeFilters and afterFilters are inherited when subclassing controllers', 8, ->
+ beforeSpy1 = createSpy()
+ beforeSpy2 = createSpy()
+ afterSpy1 = createSpy()
+ afterSpy2 = createSpy()
+
+ class TestParentController extends Batman.Controller
+ @beforeFilter beforeSpy1
+ @beforeFilter 'show', beforeSpy2
+ @afterFilter afterSpy1
+ @afterFilter 'show', afterSpy2
+
+ show: -> @render false
+
+ beforeSpy3 = createSpy()
+ beforeSpy4 = createSpy()
+ afterSpy3 = createSpy()
+ afterSpy4 = createSpy()
+
+ class TestChildController extends TestParentController
+ @beforeFilter beforeSpy3
+ @beforeFilter 'show', beforeSpy4
+ @afterFilter afterSpy3
+ @afterFilter 'show', afterSpy4
+
+ controller = new TestChildController
+ controller.dispatch 'show'
+
+ equal beforeSpy1.callCount, 1
+ equal beforeSpy2.callCount, 1
+ equal beforeSpy3.callCount, 1
+ equal beforeSpy4.callCount, 1
+
+ equal afterSpy1.callCount, 1
+ equal afterSpy2.callCount, 1
+ equal afterSpy3.callCount, 1
+ equal afterSpy4.callCount, 1
+
test 'afterFilters should only fire after renders are complete', 2, ->
afterSpy = createSpy()
@@ -338,20 +419,6 @@ test 'afterFilters on outer actions should only fire after inner renders are com
view.fireReady()
ok afterSpy.called
-test 'afterFilters on outer actions should fire after afterFilters on inner actions', 1, ->
- order = []
- class TestController extends Batman.Controller
- @afterFilter 'show', -> order.push 1
- @afterFilter 'test', -> order.push 2
- show: -> @render false
- test: ->
- @render false
- @executeAction 'show'
-
- @controller = new TestController
- @controller.dispatch 'test'
- deepEqual order, [1, 2]
-
test 'dispatching params with a hash scrolls to that hash', ->
@controller.show = -> @render false
Something went wrong with that request. Please try again.