I've replaced FELIX-4332 with FELIX-4338 and FELIX-4339. I have attached a patch for FELIX-4338 and hope that someone can have a look at it and possibly commit it.
FELIX-4339 is trickier but I would appreciate a discussion about how this should be handled. /Bengt 2013/11/29 Bengt Rodehav <[email protected]> > I've tested more with the proposed change in order to stop FileInstall to > incorrectly change the contents of the configuration file (problem b) from > my previous post). It seems to work fine. I would really like that to be > fixed. Would you like me to create a patch atttached to the JIRA? > > Problem a) is probably not trivial to fix. I've experimented a lot and > it's very hard for me to foresee how many escape characters I need in > different circumstances. One real life example for me is how I configure an > integration service that uses a Camel route underneath. If I put the > followiing contents in a test.cfg file: > > *mydir=C:/temp* > > *timestampedfile=$\\\\{file:onlyname\\\\}-$\\\\{date:now:yyyyMMddHHmmssSSS\\\\}.$\\\\{file:ext\\\\}* > *move=${mydir}/archive/$\\{date:now:yyyyMMdd\\}/${timestampedfile}* > *moveFailed=${mydir}/failed/${timestampedfile}* > *fromUri=file:${mydir}?move=${move}&moveFailed=${moveFailed}* > > And execute the following command: > > > *config:list "(service.pid=test)"* > > I get the following output: > > *----------------------------------------------------------------* > *Pid: test* > *BundleLocation: null* > *Properties:* > * moveFailed = > C:/temp/failed/${file:onlyname}-${date:now:yyyyMMddHHmmssSSS}.${file:ext}* > * mydir = C:/temp* > * timestampedfile = > $\{file:onlyname\}-$\{date:now:yyyyMMddHHmmssSSS\}.$\{file:ext\}* > * service.pid = test* > * fromUri = > file:C:/temp?move=C:/temp/archive//-.&moveFailed=C:/temp/failed/-.* > * move = > C:/temp/archive/${date:now:yyyyMMdd}/${file:onlyname}-${date:now:yyyyMMddHHmmssSSS}.${file:ext}* > * felix.fileinstall.filename = > file:/C:/dev/karaf/connect/common/etc/test.cfg* > > Thus, the variables "move" and "moveFailed" looks the way I want but the > final variable "fromUri" is messed up because of an extra variable > substitution. > > I haven't managed to come up with any number of backslashes that will > produce the correct result for me. > > The only workaround I have right now is to not use variables at all. It > does, however, make the configuration files extremely verbose and it's easy > to introduce errors that way. > > Presently, variable substitution is very unpredictable since it's being > done in a recursive way. I would prefer doing it in an iterative manner to > make it predictable. E g "${a}" should always evaluate to the same value no > matter where in the configuration file it is referenced. > > /Bengt > > > > > > > > > 2013/11/28 Bengt Rodehav <[email protected]> > >> I've investigated this a bit more. There are actually two different >> problems: >> >> a) The number of escape characters I need depends on from where I >> reference the variable. For every indirection I need to double the number >> of backslashes. This also means that all uses of a variable containing >> escape characters must be used from the same level of indirection. A bit >> complicated but it's due to the fact that all variables are evaluated >> dynamically. This means that unescaping can occur several times. >> >> b) FileInstall incorrectly thinks that a configuration property is >> changed and therefore overwrites the property with the evaluated value. >> >> I think I've found the reason (and possibly a solution) to b). >> >> In the ConfigInstaller.setConfig() method the properties are read from a >> configuration file and propagated as a configuration. Here is an excerpt >> from that method: >> >> * final Properties p = new Properties();* >> * in.mark(1);* >> * boolean isXml = in.read() == '<';* >> * in.reset();* >> * if (isXml) {* >> * p.loadFromXML(in);* >> * } else {* >> * p.load(in);* >> * }* >> * InterpolationHelper.performSubstitution((Map) p, >> context);* >> * ht.putAll(p);* >> >> Note that the file is read using Java's standard Properties class. The >> unescaping is also done by that class. Then, at the end, the variable >> substitution is done as a separate call. >> >> Then look at the ConfigInstaller.configurationEvent() method: >> >> * if (configurationEvent.getType() == >> ConfigurationEvent.CM_UPDATED)* >> * {* >> * try* >> * {* >> * Configuration config = >> getConfigurationAdmin().getConfiguration(* >> * configurationEvent.getPid(),* >> * >> configurationEvent.getFactoryPid());* >> * Dictionary dict = config.getProperties();* >> * String fileName = (String) dict.get( >> DirectoryWatcher.FILENAME );* >> * File file = fileName != null ? fromConfigKey(fileName) : >> null;* >> * if( file != null && file.isFile() ) {* >> * if( fileName.endsWith( ".cfg" ) )* >> * {* >> * org.apache.felix.utils.properties.Properties >> props = new org.apache.felix.utils.properties.Properties( file, context );* >> >> Note that now the configuration file is read using >> org.apache.felix.utils.properties.Properties class. It turns out that they >> don't produce identical results. I haven't investigated exactly how they >> differ but they do. >> >> A simple test: >> >> 1. Create a configuration file with the following content: >> >> a=$\\\\{var} >> ab=${a}b >> abc=${ab}c >> >> 2. Add the following line at the end: >> >> d=foo >> >> 3. FileInstall will now incorrectly change the contents of the >> configuration file to: >> >> a=$\\\\{var} >> ab=${a}b >> abc = ${var}bc >> d=foo >> >> Now if I change the ConfigInstaller.setConfig() method to the following: >> >> *org.apache.felix.utils.properties.Properties p = new >> org.apache.felix.utils.properties.Properties( f, context );* >> *InterpolationHelper.performSubstitution((Map) p, context);* >> >> Then FileInstall will not incorrectly change the contents of the >> configuration file. >> >> I propose to do this change in order to solve problem b) above. I >> appreciate if you have any thoughts on this. >> >> I realize that problem a) is trickier due to the dynamic nature of >> variable substitution. I haven't yet determined how I think the escape >> characters should be handled but the current situation is not ideal. >> >> /Bengt >> >> >> >> >> 2013/11/28 Bengt Rodehav <[email protected]> >> >>> JIRA created: >>> >>> https://issues.apache.org/jira/browse/FELIX-4332 >>> >>> /Bengt >>> >>> >>> 2013/11/28 Bengt Rodehav <[email protected]> >>> >>>> I've come up with easily reproducable errors using Karaf 2.3.3: >>>> >>>> - Install a fresh Karaf 2.3.3 >>>> - Add the following line to etc/custom.properties: >>>> felix.fileinstall.enableConfigSave = true >>>> >>>> Create a file etc/test.cfg with the following contents: >>>> >>>> a=$\\{var} >>>> ab=${a}b >>>> abc=${ab}c >>>> >>>> I expect this to be evaluated to: >>>> a=$\{var} >>>> ab=$\{var}b >>>> abc=$\{var}bc >>>> >>>> But if I execute the Karaf command: >>>> >>>> config:list "(service.pid=test)" >>>> >>>> I get: >>>> >>>> ---------------------------------------------------------------- >>>> Pid: test >>>> BundleLocation: null >>>> Properties: >>>> service.pid = test >>>> a = ${var} >>>> abc = bc >>>> felix.fileinstall.filename = >>>> file:/C:/dev/Karaf/apache-karaf-2.3.3/etc/test.cfg >>>> ab = b >>>> >>>> My interpretation of this is that the variable "a" has been correctly >>>> evaluated. But, when evalutating the variable "ab" it seems that the >>>> variable "a" is evaluated again despite the fact that it has already been >>>> evaluated. FileInstall now looks for the value of a variable called "var" >>>> which evalutes to an empty string because there is no such variable. >>>> >>>> The variable "abc" consequently evaluates to "bc" since the variable >>>> "ab" has been evaluated to "b". >>>> >>>> To make it even worse, now change the first row in test.cfg to: >>>> >>>> a=$\\\\{var} >>>> >>>> We now get: >>>> >>>> ---------------------------------------------------------------- >>>> Pid: test >>>> BundleLocation: null >>>> Properties: >>>> service.pid = test >>>> a = $\{var} >>>> abc = ${var}bc >>>> felix.fileinstall.filename = >>>> file:/C:/dev/Karaf/apache-karaf-2.3.3/etc/test.cfg >>>> ab = ${var}b >>>> >>>> Thus we get the same phenomenom. The variable "a" is evaluated >>>> differently if it is evaluated on its own or as part of another expression. >>>> But, due to having configured FileInstall to write back changes, the >>>> contents of the test.cfg is now changed by FileInstall despite the fact >>>> that the configuration has not changed at all. The contents of test.cfg is >>>> now: >>>> >>>> a=$\\\\{var} >>>> ab=${a}b >>>> abc = ${var}bc >>>> >>>> The "abc" variable has been altered. FileInstall has incorrectly >>>> determined that its value has changed. >>>> >>>> This is clearly a bug. I will create a JIRA. >>>> >>>> /Bengt >>>> >>>> >>>> >>>> 2013/11/26 Bengt Rodehav <[email protected]> >>>> >>>>> I'm using Apache Karaf 2.3.3 which comes with FileInstall 3.2.6. I >>>>> have set the felix.fileinstall.enableConfigSave property to true in order >>>>> to have FileInstall write back configuration changes to the file. Normally >>>>> all configuration changes are done by editing the configuration file but >>>>> there is one property that I change programmatically using ConfigAdmin (an >>>>> "enable" property to start/stop my service). I am dependent on that >>>>> property being persisted in the configuration file which is why I set the >>>>> enableConfigSave property to true. >>>>> >>>>> When configuring FileInstall to write back configuration changes to >>>>> the configuration file, it is important that variables are not substituted >>>>> for the evaluated value. This normally works since FileInstall evalutates >>>>> the property in the configuration file and compares it with the >>>>> configuration admin's value. If they are the same, the value in the >>>>> configuration file is kept unchanged. >>>>> >>>>> However, when using the escape character this is broken. In my case >>>>> I'm using Apache Camel underneath. When configuring routes via the config >>>>> admin, I sometimes need to set a value to >>>>> "${expression-to-be-evaluated-by-camel}". I therefore escape the "{" and >>>>> "}" to stop FileInstall from trying to evaluate the expression. Like this: >>>>> >>>>> $\\{expression-to-be-evaluated-by-camel\\} >>>>> >>>>> This also normally works but not when I have an indirection. E g when >>>>> specifying the following: >>>>> >>>>> a=$\\{var} >>>>> ab=${a}b >>>>> >>>>> FileInstall will change the configuration file to: >>>>> >>>>> a=$\\{var} >>>>> ab = ${var}b >>>>> >>>>> Note that the variable "ab" has now been expanded and written back to >>>>> the configuration file even if neither of the variables "a" and "ab" have >>>>> been changed. >>>>> >>>>> I think this is because FileInstall does the following: >>>>> >>>>> 1. Calculates the value of "a" to "$\{var} >>>>> 2. Calculates the value of "b" to "${var}b >>>>> >>>>> Note that every evaluation will perform "unescaping". This means that >>>>> an extra "unescaping" will be done for every indirection which fools >>>>> FileInstall into thinking that the property has been changed. >>>>> >>>>> I'm not exactly sure how this should be fixed in FileInstall. One idea >>>>> is to never "unescape" already evaluated variables. Actually I think this >>>>> is probably what would fix this... >>>>> >>>>> Does anybody have any ideas about this? Should I create a JIRA? >>>>> >>>>> /Bengt >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>>> >>>> >>> >> >
