groovy

ConfigSlurper does not resolve within closures references to outer properties

Details

  • Type: Bug Bug
  • Status: Closed Closed
  • Priority: Major Major
  • Resolution: Fixed
  • Affects Version/s: 1.1-beta-3
  • Fix Version/s: 1.1-rc-2
  • Component/s: groovy-jdk
  • Labels:
    None
  • Environment:
    Windows XP sp2
    Java 1.6.0_02-b06
  • Number of attachments :
    1

Description

When processing a scoped closure ConfigSlurper does not resolve references to properties defined out of closure scope.

For example this config file:
a0 = "Goofy"
a1 = "$a0"
a2."$a0" = "Mickey Mouse and " + "$a0"
group1 {
a0 = "Donald Duck"
}
group2 {
a0 = "$a0"
a1 = "$group1.a0"
}
a3 = "$group1.a0"

produces the following:

a0=Goofy
a1=Goofy
a2.Goofy=Mickey Mouse and Goofy
group1.a0=Donald Duck
group2.a0=[:]
group2.a1=[:]
a3=Donald Duck

Applying this patch to ConfigSlurper

— C:/export/java/Groovy/groovy-1.1-beta-3/src/main/groovy/util/ConfigSlurper-save.groovy Fri Sep 21 00:40:58 2007
+++ C:/export/java/Groovy/groovy-1.1-beta-3/src/main/groovy/util/ConfigSlurper.groovy Thu Oct 11 10:10:30 2007
@@ -159,8 +159,9 @@

if(current[name]) { result = current[name] - }

  • else { + } else if (config[name]) { + result = config[name] + } else { result = new ConfigObject() current[name] = result }

makes it correctly resolving "group2" references:
group2.a0=Goofy
group2.a1=Donald Duck

Activity

Hide
Gabriele Garuglieri added a comment -

Sorry, it looks like the cut and paste somehow mangled the patch.
I'm attaching it as a file.

Show
Gabriele Garuglieri added a comment - Sorry, it looks like the cut and paste somehow mangled the patch. I'm attaching it as a file.
Hide
Guillaume Laforge added a comment -

Thanks for the patch, Gabriele.
If you want to add some specific unit test demonstrating this, please feel free to do so.

Show
Guillaume Laforge added a comment - Thanks for the patch, Gabriele. If you want to add some specific unit test demonstrating this, please feel free to do so.
Hide
Guillaume Laforge added a comment -

I reopen the issue cause it seems there's a regression in one of the tests of ConfigSlurperTest, in the test checking the serialization.
Gabriele, could you please have a second look at your patch, and the test cases, please?

Show
Guillaume Laforge added a comment - I reopen the issue cause it seems there's a regression in one of the tests of ConfigSlurperTest, in the test checking the serialization. Gabriele, could you please have a second look at your patch, and the test cases, please?
Hide
Guillaume Laforge added a comment -

The suggested change was breaking some tests.
What shall we do with this issue?

Show
Guillaume Laforge added a comment - The suggested change was breaking some tests. What shall we do with this issue?
Hide
Gabriele Garuglieri added a comment -

Ok guys, i didn't disappear. It's only that i have my real job to pursue and i'm really on a tight schedule.
Testing more closely the current behaviour of ConfigSlurper and the side effects the patch is causing, looks that it is so by design and not by mistake.
This means that the patch i proposed looks to be changing the design in a way that would require to recode ConfigSlurper in a way that it's currently beyond my actual knowledge and beyond the time i have available to spend on it, so i think i have to give up.
So what to say?
The patch is not applicable, so disregard it and feel free to close the bug as WAD if you like.
I wish to thank you for your kind attention anyway.

Show
Gabriele Garuglieri added a comment - Ok guys, i didn't disappear. It's only that i have my real job to pursue and i'm really on a tight schedule. Testing more closely the current behaviour of ConfigSlurper and the side effects the patch is causing, looks that it is so by design and not by mistake. This means that the patch i proposed looks to be changing the design in a way that would require to recode ConfigSlurper in a way that it's currently beyond my actual knowledge and beyond the time i have available to spend on it, so i think i have to give up. So what to say? The patch is not applicable, so disregard it and feel free to close the bug as WAD if you like. I wish to thank you for your kind attention anyway.
Hide
Guillaume Laforge added a comment -

We can keep this issue open till you find some more time to have a deeper look at the issue.
If you and Graeme think it's a desirable change to improve ConfigSlurper even more, then it's certainly worth waiting some more till we all have time to implement that feature properly.
Thanks a lot Gabriele for your time and this patch.

Show
Guillaume Laforge added a comment - We can keep this issue open till you find some more time to have a deeper look at the issue. If you and Graeme think it's a desirable change to improve ConfigSlurper even more, then it's certainly worth waiting some more till we all have time to implement that feature properly. Thanks a lot Gabriele for your time and this patch.
Hide
blackdrag blackdrag added a comment -

fixed

Show
blackdrag blackdrag added a comment - fixed

People

Vote (0)
Watch (0)

Dates

  • Created:
    Updated:
    Resolved: