diff --git a/src/java/org/codehaus/groovy/grails/plugins/web/filters/FilterToHandlerAdapter.groovy b/src/java/org/codehaus/groovy/grails/plugins/web/filters/FilterToHandlerAdapter.groovy index 544f4cfb63b..15000c5611e 100644 --- a/src/java/org/codehaus/groovy/grails/plugins/web/filters/FilterToHandlerAdapter.groovy +++ b/src/java/org/codehaus/groovy/grails/plugins/web/filters/FilterToHandlerAdapter.groovy @@ -49,6 +49,7 @@ class FilterToHandlerAdapter implements HandlerInterceptor, InitializingBean { def useRegex // standard regex def invertRule // invert rule def useRegexFind // use find instead of match + def dependsOn = [] // any filters that need to be processed before this one void afterPropertiesSet() { def scope = filterConfig.scope @@ -211,4 +212,4 @@ class FilterToHandlerAdapter implements HandlerInterceptor, InitializingBean { String toString() { return "FilterToHandlerAdapter[$filterConfig, $configClass]" } -} \ No newline at end of file +} diff --git a/src/java/org/codehaus/groovy/grails/plugins/web/filters/FiltersGrailsPlugin.groovy b/src/java/org/codehaus/groovy/grails/plugins/web/filters/FiltersGrailsPlugin.groovy index 5ea8d235de0..a457f81109c 100644 --- a/src/java/org/codehaus/groovy/grails/plugins/web/filters/FiltersGrailsPlugin.groovy +++ b/src/java/org/codehaus/groovy/grails/plugins/web/filters/FiltersGrailsPlugin.groovy @@ -16,6 +16,7 @@ package org.codehaus.groovy.grails.plugins.web.filters import grails.util.GrailsUtil +import java.util.ArrayList import org.apache.commons.logging.LogFactory @@ -129,17 +130,107 @@ class FiltersGrailsPlugin { log.info "reloadFilters" def filterConfigs = application.getArtefacts(TYPE) def handlers = [] - for (c in filterConfigs) { + + def sortedFilterConfigs = [] // the new ordered filter list + def list = new ArrayList(Arrays.asList(filterConfigs)) + def addedDeps = [:] + + while (list.size() > 0) { + def filtersAdded = 0; + + log.debug("Current filter order is '"+filterConfigs.join(",")+"'") + + for (Iterator iter = list.iterator(); iter.hasNext(); ) { + def c = iter.next(); + def filterClass = applicationContext.getBean("${c.fullName}Class") + def bean = applicationContext.getBean(c.fullName) + log.debug("Processing filter '${bean.class.name}'") + + def dependsOn = null + if (bean.metaClass.hasProperty(bean, "dependsOn")) { + dependsOn = bean.dependsOn + log.debug(" depends on '"+dependsOn.join(",")+"'") + } + + if (dependsOn != null) { + // check dependencies to see if all the filters it depends on are already in the list + log.debug(" Checking filter '${bean.class.name}' dependencies (${dependsOn.size()})") + + def failedDep = false; + for (dep in dependsOn) { + log.debug(" Checking filter '${bean.class.name}' dependencies: ${dep.name}") + //if (sortedFilterConfigs.find{def b = applicationContext.getBean(it.fullName); b.class == dep} == null) { + if (!addedDeps.containsKey(dep)) { + // dep not in the list yet, we need to skip adding this to the list for now + log.debug(" Skipped Filter '${bean.class.name}', since dependency '${dep.name}' not yet added") + failedDep = true + break + } else { + log.debug(" Filter '${bean.class.name}' dependency '${dep.name}' already added") + } + } + + if (failedDep) { + // move on to next dependency + continue + } + } + + log.debug(" Adding filter '${bean.class.name}', since all dependencies have been added") + sortedFilterConfigs.add(c) + addedDeps.put(bean.class, null); + iter.remove() + filtersAdded++ + } + + // if we didn't add any filters this iteration, then we have a cyclical dep problem + if (filtersAdded == 0) { + // we have a cyclical dependency, warn the user and load in the order they appeared originally + log.warn(":::::::::::::::::::::::::::::::::::::::::::::::") + log.warn(":: Cyclical Filter dependencies detected ::") + log.warn(":: Continuing with original filter order ::") + log.warn(":::::::::::::::::::::::::::::::::::::::::::::::") + for (c in list) { + def filterClass = applicationContext.getBean("${c.fullName}Class") + def bean = applicationContext.getBean(c.fullName) + + // display this as a cyclical dep + log.warn(":: Filter "+bean.class.name) + def dependsOn = null + if (bean.metaClass.hasProperty(bean, "dependsOn")) { + dependsOn = bean.dependsOn + for (dep in dependsOn) { + log.warn(":: depends on "+dep.name) + } + } else { + // we should only have items left in the list with deps, so this should never happen + // but a wise man once said...check for true, false and otherwise...just in case + log.warn(":: Problem while resolving cyclical dependencies.") + log.warn(":: Unable to resolve dependency hierarchy.") + } + log.warn(":::::::::::::::::::::::::::::::::::::::::::::::") + } + break + // if we have processed all the filters, we are done + } else if (sortedFilterConfigs.size() == filterConfigs.size()) { + log.debug("Filter dependency ordering complete") + break + } + } + + // add the filter configs in dependency sorted order + log.debug("Resulting handlers:") + for (c in sortedFilterConfigs) { def filterClass = applicationContext.getBean("${c.fullName}Class") def bean = applicationContext.getBean(c.fullName) for (filterConfig in filterClass.getConfigs(bean)) { def handlerAdapter = new FilterToHandlerAdapter(filterConfig:filterConfig, configClass:bean) handlerAdapter.afterPropertiesSet() handlers << handlerAdapter + log.debug(" $handlerAdapter") } } - log.debug("resulting handlers: $handlers") applicationContext.getBean('filterInterceptor').handlers = handlers } } diff --git a/src/test/org/codehaus/groovy/grails/plugins/web/filters/FilterExecutionTests.groovy b/src/test/org/codehaus/groovy/grails/plugins/web/filters/FilterExecutionTests.groovy index 4b3a2a6df90..c48e9890ac8 100644 --- a/src/test/org/codehaus/groovy/grails/plugins/web/filters/FilterExecutionTests.groovy +++ b/src/test/org/codehaus/groovy/grails/plugins/web/filters/FilterExecutionTests.groovy @@ -156,6 +156,111 @@ class Filters { } } } +} + ''') + + gcl.parseClass('''\ +import junit.framework.Assert + +class TestController { + def index = {} + def list = {} +} + +class Group1Filters { + def dependsOn = [Group3Filters.class] + + def afterClosure = { + println "afterClosure!!!" + } + + def afterCompleteClosure = { + println "afterCompleteClosure!!!" + } + + def filters = { + filter1(uri:"/dependsOn") { + before = { + Assert.assertTrue(request.filter3); // should come first, since we have deps and it doesn't + Assert.assertNull(request.filter4);// shouldn't be called for /dependsOn + Assert.assertTrue(request.filter5); // depends on filter + Assert.assertTrue(request.filter6); // depends on filter + request.filter1 = true + } + after = afterClosure + afterView = afterCompleteClosure + } + filter2(uri:"/dependsOn") { + before = { + Assert.assertTrue(request.filter1); // declared earlier in this filter + Assert.assertTrue(request.filter3); // should come first, since we have deps and it doesn't + Assert.assertNull(request.filter4);// shouldn't be called for /dependsOn + Assert.assertTrue(request.filter5); // depends on filter + Assert.assertTrue(request.filter6); // depends on filter + request.filter2 = true + } + after = afterClosure + afterView = afterCompleteClosure + } + } +} + +class Group2Filters { + def afterClosure = { + println "afterClosure!!!" + } + + def afterCompleteClosure = { + println "afterCompleteClosure!!!" + } + + def filters = { + filter3(uri:"/dependsOn") { + before = { + request.filter3 = true + } + after = afterClosure + afterView = afterCompleteClosure + } + filter4(uri:"/neverCall") { + before = { + request.filter4 = true + } + after = afterClosure + afterView = afterCompleteClosure + } + } +} + +class Group3Filters { + def afterClosure = { + println "afterClosure!!!" + } + + def afterCompleteClosure = { + println "afterCompleteClosure!!!" + } + + def filters = { + filter5(uri:"/dependsOn") { + before = { + Assert.assertTrue(request.filter3); // should come first, since we are defined second + Assert.assertNull(request.filter4);// shouldn't be called for /dependsOn + request.filter5 = true + } + after = afterClosure + afterView = afterCompleteClosure + } + filter6(uri:"/dependsOn") { + before = { + Assert.assertTrue(request.filter3); // should come first, since we are defined second + Assert.assertNull(request.filter4);// shouldn't be called for /dependsOn + request.filter6 = true + } + after = afterClosure + afterView = afterCompleteClosure + } + } } ''') } @@ -287,5 +392,20 @@ class Filters { ModelAndView mv = new ModelAndView() filterInterceptor.postHandle(request, response, null, mv) assertEquals "/item/happyPath", mv.viewName + + // check dependsOn filters + request.clearAttributes() + + request.setAttribute(WebUtils.FORWARD_REQUEST_URI_ATTRIBUTE, "/dependsOn") + request.setAttribute(GrailsApplicationAttributes.CONTROLLER_NAME_ATTRIBUTE, "test") + request.setAttribute(GrailsApplicationAttributes.ACTION_NAME_ATTRIBUTE, "index") + + filterInterceptor.preHandle(request, response, null) + assertTrue request.filter1 + assertTrue request.filter2 + assertTrue request.filter3 + assertNull request.filter4 + assertTrue request.filter5 + assertTrue request.filter6 } }