search

padcom / grails-routing

Apache License 2.0
30 stars 31 forks source link

Value of autoStart from config is ignored #44

Closed augustl closed 8 years ago

augustl commented 9 years ago

In commit c82d50f0bb7f251cfe1e36c6bec6c979b585c94d, a change was made that disabled autoStartup at plugin init time, and deferred to a manual start in doWithDynamicMethods.

This startup procedure checks the config for an "autoStartup" flag so you can disable auto start if you want, and manually start the app later. But this check is bugged.

def config = application.config.grails.routing

if (config.grails.routing.autoStartup ?: true) {
    def camelContextId = config.camelContextId ?: 'camelContext'
    application.mainContext.getBean(camelContextId).start()
}

If application.config.grails.routing.grails.routing.autoStartup is set to false, the whole statement will evaluate to true anyway:

false ?: true // Returns true

A better check would be something like:

if (application.config.grails.routing.grails.routing.autoStartup != false) {
    def camelContextId = config.camelContextId ?: 'camelContext'
    application.mainContext.getBean(camelContextId).start()
}

That way, you can set autoStartup to false in config, but if it's not set, it will start up automatically.

As an aside, it would probably be better if the config was application.config.grails.routing.autoStartup instead of application.config.grails.routing.grails.routing.autoStartup. This bug seems to have been introduced in commit 40a241b2cba377d1237564e2374f1208b5fb3708.

aahutsal commented 9 years ago

Thanks August. I'll review later 7 лист. 2014 15:04, користувач "August Lilleaas" notifications@github.com написав:

In commit c82d50f https://github.com/padcom/grails-routing/commit/c82d50f0bb7f251cfe1e36c6bec6c979b585c94d, a change was made that disabled autoStartup at plugin init time, and deferred to a manual start in doWithDynamicMethods.

This startup procedure checks the config for an "autoStartup" flag so you can disable auto start if you want, and manually start the app later. But this check is bugged.

def config = application.config.grails.routing if (config.grails.routing.autoStartup ?: true) { def camelContextId = config.camelContextId ?: 'camelContext' application.mainContext.getBean(camelContextId).start() }

If application.config.grails.routing.grails.routing.autoStartup is set to false, the whole statement will evaluate to true anyway:

false ?: true // Returns true

A better check would be something like:

if (application.config.grails.routing.grails.routing.autoStartup != false) { def camelContextId = config.camelContextId ?: 'camelContext' application.mainContext.getBean(camelContextId).start() }

That way, you can set autoStartup to false in config, but if it's not set, it will start up automatically.

As an aside, it would probably be better if the config was application.config.grails.routing.autoStartup instead of application.config.grails.routing.grails.routing.autoStartup. This bug seems to have been introduced in commit 40a241b https://github.com/padcom/grails-routing/commit/40a241b2cba377d1237564e2374f1208b5fb3708 .

— Reply to this email directly or view it on GitHub https://github.com/padcom/grails-routing/issues/44.

aahutsal commented 9 years ago

Could you commit the patch? 7 лист. 2014 15:11, користувач "Arsen A. Gutsal" < gutsal.arsen@softsky.com.ua> написав:

Thanks August. I'll review later 7 лист. 2014 15:04, користувач "August Lilleaas" notifications@github.com написав:

In commit c82d50f https://github.com/padcom/grails-routing/commit/c82d50f0bb7f251cfe1e36c6bec6c979b585c94d, a change was made that disabled autoStartup at plugin init time, and deferred to a manual start in doWithDynamicMethods.

This startup procedure checks the config for an "autoStartup" flag so you can disable auto start if you want, and manually start the app later. But this check is bugged.

def config = application.config.grails.routing if (config.grails.routing.autoStartup ?: true) { def camelContextId = config.camelContextId ?: 'camelContext' application.mainContext.getBean(camelContextId).start() }

If application.config.grails.routing.grails.routing.autoStartup is set to false, the whole statement will evaluate to true anyway:

false ?: true // Returns true

A better check would be something like:

if (application.config.grails.routing.grails.routing.autoStartup != false) { def camelContextId = config.camelContextId ?: 'camelContext' application.mainContext.getBean(camelContextId).start() }

That way, you can set autoStartup to false in config, but if it's not set, it will start up automatically.

As an aside, it would probably be better if the config was application.config.grails.routing.autoStartup instead of application.config.grails.routing.grails.routing.autoStartup. This bug seems to have been introduced in commit 40a241b https://github.com/padcom/grails-routing/commit/40a241b2cba377d1237564e2374f1208b5fb3708 .

— Reply to this email directly or view it on GitHub https://github.com/padcom/grails-routing/issues/44.

augustl commented 9 years ago

Sure. Do you want me to change the placement of the config property as well, so grails.routing isn't repeated? Seems safe to me, it's not backwards compatible, but the flag seems to never have worked anyway as far as I can tell.

aahutsal commented 9 years ago

Yes please. Duplicated grails-routing is a bug also 7 лист. 2014 15:13, користувач "August Lilleaas" notifications@github.com написав:

Sure. Do you want me to change the placement of the config property as well, so grails.routing isn't repeated? Seems safe to me, it's not backwards compatible, but the flag seems to never have worked anyway as far as I can tell.

— Reply to this email directly or view it on GitHub https://github.com/padcom/grails-routing/issues/44#issuecomment-62140240 .

augustl commented 9 years ago

Pull request is up, https://github.com/padcom/grails-routing/pull/45

Any idea when this fix will make it into a release? As you probably guessed, I need to disable auto startup in my app :) I can disable auto start of individual routes and then start individual routes, but it's much safer and easier to just defer the starting of the entire routing stack.

aahutsal commented 9 years ago

Will try to release today 7 лист. 2014 15:21, користувач "August Lilleaas" notifications@github.com написав:

Pull request is up, #45 https://github.com/padcom/grails-routing/pull/45

Any idea when this fix will make it into a release? As you probably guessed, I need to disable auto startup in my app :) I can disable auto start of individual routes and then start individual routes, but it's much safer and easier to just defer the starting of the entire routing stack.

— Reply to this email directly or view it on GitHub https://github.com/padcom/grails-routing/issues/44#issuecomment-62141063 .

augustl commented 9 years ago

Awesome :)

aahutsal commented 9 years ago

v1.3.3 released, please check and lemme know if everything is OK

2014-11-07 13:28 GMT+00:00 August Lilleaas notifications@github.com:

Awesome :)

— Reply to this email directly or view it on GitHub https://github.com/padcom/grails-routing/issues/44#issuecomment-62141735 .

Arsen

augustl commented 9 years ago

Grails finds 1.3.2, but it complains that 1.3.3 isn't released yet. Which repo did you release to?

aahutsal commented 9 years ago

No repo man :) I released on github. You can include direct URL and get it from there. I'm not sure I've permissions to release to grails repo, but I'll check 7 лист. 2014 16:32, користувач "August Lilleaas" notifications@github.com написав:

Grails finds 1.3.2, but it complains that 1.3.3 isn't released yet. Which repo did you release to?

— Reply to this email directly or view it on GitHub https://github.com/padcom/grails-routing/issues/44#issuecomment-62152021 .

augustl commented 9 years ago

Ah. Do you know how you add a URL as a dependency? Tried compile "https://github.com/padcom/grails-routing.git" but that didn't work.

augustl commented 9 years ago

Think I figured it out :) Used a inline plugin with the path to my local clone.

Everything is working nicely for me now. It starts up if the config property is not set, or set to anything but "false".

augustl commented 9 years ago

Hold your horses.. Seems I spoke to soon. Testing some more :)

aahutsal commented 9 years ago

Ok. I'll ask padcom to release 1.3.3 to grails repo 7 лист. 2014 16:43, користувач "August Lilleaas" notifications@github.com написав:

Think I figured it out :) Used a inline plugin with the path to my local clone.

Everything is working nicely for me now. It starts up if the config property is not set, or set to anything but "false".

— Reply to this email directly or view it on GitHub https://github.com/padcom/grails-routing/issues/44#issuecomment-62153474 .

augustl commented 9 years ago

It seems the camel context is started up much earlier than doWithDynamicMethods. I added some break points to RoutingGrailsPlugin.groovy and can see that this log line:

INFO org.apache.camel.spring.SpringCamelContext - Apache Camel 2.13.2 (CamelContext: camelContext) is starting

..occurs before the if-test and the call to start in doWithDynamicMethods.

augustl commented 9 years ago

Seems it's just me being a bit confused :) The context itself seems to start, but none of the routes are starting until the .start() method is called.

So it's all good!

aahutsal commented 9 years ago

Thought so. Ok 7 лист. 2014 17:16, користувач "August Lilleaas" notifications@github.com написав:

Seems it's just me being a bit confused :) The context itself seems to start, but none of the routes are starting until the .start() method is called.

So it's all good!

— Reply to this email directly or view it on GitHub https://github.com/padcom/grails-routing/issues/44#issuecomment-62158298 .

augustl commented 9 years ago

Did you hear back from the padcom peeps? Seems 1.3.3 hasn't been released to repo yet.

aahutsal commented 9 years ago

Padcom is busy person 10 лист. 2014 11:26, користувач "August Lilleaas" notifications@github.com написав:

Did you hear back from the padcom peeps? Seems 1.3.3 hasn't been released to repo yet.

— Reply to this email directly or view it on GitHub https://github.com/padcom/grails-routing/issues/44#issuecomment-62359630 .

aahutsal commented 9 years ago

@augustl sorry man. That too waaaaayyy too long to release it on grails central. Seems I've just done it. 1.3.3 should be accessible soon. Seems that project gets out of contributors and I just don't have time to work on it. Basically I'm looking for ppl who is still interested and would contribute.

augustl commented 8 years ago

Just verified that this works in 1.4.0, closing. Thanks :)