Augeas fails to parse properties-files with "/" in property names

[felixdoerre]

This java snippet:

import java.io.IOException;
import java.util.Properties;

public class StrangeProperties {
	public static void main(String[] args) throws IOException {
		Properties p = new Properties();
		p.setProperty("a/b", "c");
		p.store(System.out, null);
	}
}

generates this properties-file:

#Sun Apr 19 19:24:23 CEST 2020
a/b=c

however augeas fails to parse this:

# cat > /tmp/augtest/test.props
a/b=c
# augtool -L
augtool> transform Properties.lns incl /tmp/augtest/test.props
augtool> load
augtool> errors
Error in /tmp/augtest/test.props:1.1 (parse_failed)
  Iterated lens matched less than it should
  Lens: /usr/share/augeas/lenses/dist/properties.aug:50.25-.100:
    Last matched: /usr/share/augeas/lenses/dist/properties.aug:45.39-.48:
    Next (no match): /usr/share/augeas/lenses/dist/properties.aug:20.25-.46:
augtool> 

[raphink]

This is a known issue, due to the fact that keys in Augeas cannot contain a slash. One way to fix this would be to split keys on . or /, but that would break compatibility. I thought there was already an issue on this, but I can't find it. @lutter what do you think?

[felixdoerre]

I can't understand why keys cannot contain slashes. They seem to work perfectly when properly escaped. So just removing the slash from the forbidden characters seems to work:

# cat /tmp/augtest/test.props
a/b=c
# augtool -L
augtool> transform Properties.lns incl /tmp/augtest/test.props
augtool> load
augtool> print /files/tmp/augtest/test.props
/files/tmp/augtest/test.props
/files/tmp/augtest/test.props/a\/b = "c"
augtool> set /files/tmp/augtest/test.props/a\\/c d
augtool> save
Saved 1 file(s)
augtool> quit
# cat /tmp/augtest/test.props
a/b=c
a/c=d
# 

regarding compatibility: I can't see how one could break compatibility for any properties file that contains a '/' in a key, as they can currently not be parsed, and the behavior for other properties files shouldn't change in any case.

[raphink]

I don't know if that restriction still makes sense indeed. I'll let @lutter answer.

[raphink]

Duplicate of #176

[felixdoerre]

I think it's wrong to mark this as duplicate. The situation is different here:

• with sysctl / and . need to be parsed to form a hierarchy.

The ’/’ separator is also accepted in place of a ’.’.

• with java properties file a / and . have no special meaning at all and are just names. Additionally augeas does not try to parse java properties files into a hierarchy but represents them as flat key-value mapping.

So while I understand that just accepting the / might be a problem for sysctl, I can't understand why it's a problem here.

Also I can't see why compatiblity is an issue here. The change would just allow more values to be parsed, so for all applications that currently don't experience syntax errors this would not change anything.

[raphink]

Ah you're right sorry. I'll reopen.

[raphink]

So, unless we can lift the "no slash in key" restriction, the other option is to turn this into a seq lens, but that's kind of ugly:

{ "1"
  { "key" = "some.key" }
  { "value" = "some value" } }
{ "2"
  { "key" = "some/other/key" }
  { "value" = "some other value" } }

This would break compatibility (and make the lens quite more complex).

[felixdoerre]

Yes, that would be an option, but I would of course prefer the compatible and clean way.
Can you point me to something where this "no slash in key" restriction is documented and what the rationale is behind it? As I mentioned, augeas itself seems to be able to handle such keys perfectly and therefore I can't see why one would need to be so strict.

[raphink]

The restriction is in the type checker, which makes it impossible to write lenses whose keys accept a slash: https://github.com/hercules-team/augeas/blob/master/src/lens.c#L586

[felixdoerre]

Thanks for this information. So do I understand you correctly, that we now need a statement from @lutter on why this check was put in there and whether it can safely be removed?

[raphink]

Yes, @lutter is the one who would know.

[felixdoerre]

Any news?

[felixdoerre]

@raphink do you know if this issue can get any progress? It seems that there is no reaction. I'd really like this issue to be resolved and would also like to help, if I can. But waiting for a specific person to respond seems non-satisfactory.

Additionally, to me, this issue seems to be less a "feature-request" but more a bug, as the "feature" to parse and handle java properties-files has the bug to not parse a specific properties file correctly. So this is probably a "bug" in the java properties file lens that we try to solve with a "feature request" in the augeas core.

[felixdoerre]

Can we do something to move this issue forward? Having this lingering around without any progress is really unsatisfactory.

[lutter]

The reason that / is not allowed in a key is that it is used to separate path components: if you allowed it, it would be impossible to know whether a/b/c should look for nodes a > b > c or whether it is the b/c child of a.

And I should have read the whole issue - yes, I think you have a point that with proper escaping this would work.

[felixdoerre]

Hi, thank you very much for answering. Proper escaping does work. The question is, if we could remove the restriction from the lens. From experiments with the "broadened" lens, it all seems to work perfectly (of course you have to escape / in key names, when you want to access them). I have not observed any problems.

The question is: can we remove the requirement that node names must not contain / from the type checker (https://github.com/hercules-team/augeas/blob/master/src/lens.c#L586)? This seems to be the only thing that is stopping us from changing the lens.

[lutter]

Off the top of my hat, changing this behavior would require the following:

• On this line use a regexp that only matches unescaped /
• On this line, similarly, only check for unescaped /
• Add tests to tests/modules that make sure the typechecker distinguishes correct and incorrect usage of escaped slashed, and add unit tests/lenses that ensure they are handled correctly
• Add tests to tests/xpath.tests to check that escaping of / is handled correctly; there's probably some headache around \\\\/ vs \\\/ to get this absolutely right

[felixdoerre]

Sorry, but I don't understand why the typechecker has to cope with escaped slashes at all. Probably I've misunderstood the architecture somehow, so just let me sum up my understanding and correct me where I am wrong:

The Lenses transform a text file into a hierarchical structure of key names and values. In this structure only the escaping of the configuration-file format is important (i.e. when does a character split a property name from a value, where do values really end, etc...). In this stage there should be nothing special about the /-character. The typechecker is used to check some constraints on the lenses.

Later the augeas command language is used to modify the configuration file structure. Here proper escaping of '/' is required to correctly address the nodes, as in this language '/' has a special meaning.

That the slash needs to be escaped is a feature/limitation of the "augeas command language". The node name itself can contain slashes and backslashes in any way it wants. This should not matter.

So could you please explain to me, why the type checker needs to understand what slashes are escaped and what slashes aren't?

[lutter]

Yes, you are absolutely right. There is no need for the typechecker to worry about embedded / - when we match path expressions, we match them against trees. The / only comes into play when we parse a string containing a path expression into the internal data structure that gets actually matched against the tree. In theory, we could use something else to separate path components in the textual path expressions (say @) and things would still work the same way even though the typechecker does not care about @ at all.

So I think it is fine to just remove any checks for / in the typechecker.

[felixdoerre]

Thanks for doing and merging #679! I have created the follow-up-PR that modifies the properties lens: #680.

[felixdoerre]

Hi @raphink,
I think this issue is resolved now. However, I am interested when the fix will be released as a new augeas release. The last release seems to be over a year ago and there are also approximately 40 unreleased commits to master, which seems to be around the usual scope of a release. Do you have any information, when a new release can be expected?

[waipeng]

Hi @raphink, @lutter we are also very interested in a new augeas release containing this fix.

Thanks for all the work!