[patch] Support for escaping pipe character in remote control addtags parameters

[SanderH]

What steps will reproduce the problem?

1. Follow a link with the | escaped (suggest \|) like: ​https://localhost:8112/load_object?new_layer=true&objects=n123456&addtags=test%3Atag1=true%7C%5C%7Ctest%3Atag2=true%7Ctest%3Atag3=contains%5C%7Cpipe

What is the expected result?

JOSM should add the tags "test:tag1=true", "|test:tag2=true" and "test:tag3=contains|pipe"

What happens instead?

JOSM add the tags "test:tag1=true", "test:tag2=true" and "test:tag3=contains\"
So with tag2, the preceding pipe is lost in the tag name and with tag3 the tag value is cut off.

Please provide any additional information below. Attach a screenshot if possible.

For an existing plugin (​https://github.com/gidema/josm-ods-bag) we're using some temporary tags to make better selections in JOSM which will be removed by the plugin before uploading. The tags used happen to start with a pipe symbol. Without a way to escape the pipe character I cannot add some tags so a list of objects.

I also noticed that the data cannot contain pipes, so it makes sense to enhance the behavior.

Seems that changing the split in https://josm.openstreetmap.de/browser/josm/trunk/src/org/openstreetmap/josm/io/remotecontrol/AddTagsDialog.java?rev=10973#L272 to one using a Pattern.quote (​http://stackoverflow.com/questions/18677762/handling-delimiter-with-escape-characters-in-java-string-split-method) should do the trick.

[ToniE]

Different result for the same reason.

The confirm dialog box is completely empty for:

​http://127.0.0.1:8111/load_object?new_layer=false&relation_members=true&objects=r1549761&addtags=gtfs:route_id=de:mvv-muenchen:19-210|210|RegionalBus:1179_3|gtfs:trip_id:sample=de:mvv-muenchen:19-210|210|RegionalBus:1179-1-1-H-0-We#3-320-333

where two pipe symbols are in the value.

[stoecker]

Not Pattern.quote is the relevant part, but the negative look-behind (?<!\\)\| or "(?<!\\\\)\\|" in Java. That means that it will only split at "|", when there is no "\" before it (wonderful how the "\" multiply :-).

Can you provide a patch with added tests?

[ToniE]

Can you provide a patch with added tests?

Am I supposed to provide something?

[stoecker]

Yes. It would be fine, when you provide a patch including tests for new and old functionality. Changing the split is easy, but making the tests is a bit more work and I have little time for JOSM nowadays.

[ToniE]

I will try my best, but I've never coded in Java.

I guess, for the fix, I have to change line 286 in "AddTagsDialog.java" = Map<String, String> tags = TextTagParser.readTagsByRegexp(urlSection, "\\|", "(.*?)=(.*?)", false);

For the test, I have to create a new file called "AddTagsDialogTest.java" and copy and paste some things from "TextTagParserTest.java", right?

[stoecker]

Ah. No Java experience. I assumed you have... :-) Well, please try anyway, but I'll do it if you have no progress. Luckily it's easy:

The test is already there. Only needs a few additional test cases inside of testParseUrlTagsToKeyValues()
trunk/test/unit/org/openstreetmap/josm/io/remotecontrol/AddTagsDialogTest.java​

Yes. trunk/src/org/openstreetmap/josm/io/remotecontrol/AddTagsDialog.java​ needs "\\|" replaced by "(?<!\\\\)\\|".

If you have trouble with the compile test, it would be fine when you send a patch for the AddTagsDialogTest.java file catching different possibilities (escaped, not escaped, ...) and I'll do the check and test calling.

Remember to escape in Java you need to add another \ :-)

[anonymous]

Thanks, "ant" is running and current tests are OK, new tests cases still needed: thanks for the link.

Besides replacing the "\\|" ... I need to unescape the "\|" by "|" in "readTagsByRegexp()".
So I added a new parameter for this: "boolean unescapePipe" which is true only when called from "parseUrlTagsToKeyValues()"

[ToniE]

Patch file (svn diff) for this ticket

[ToniE]

Did the necessary coding, added 4 test cases, tested in real life.
See the attached "svn diff" patch.

[ToniE]

Patch file (svn diff) for this ticket

[ToniE]

Sorry, the file name of the first attachment has a typo, a wrong ticket number

[ToniE]

Following the developer guidelines: change name of patch file
Add more more docu for the test cases
Please ignore the first two patches

[ToniE]

patch with correct file name

[stoecker]

Whoah. What an escaping orgy :-) \\\\\\|

[ToniE]

Yeah! "6" works as expected, "4" didn't show the expected results, I didn't try other values though.

[ToniE]

Uuh! I didn't add a test AddTagsDialog.parseUrlTagsToKeyValues("gtfs:route_id=de:mvv-muenchen:19-210\|210"); with a single escape char. I don't know much about Java's escaping orgy, other languages aren't better though.

But! that's the exact string sent to JOSM via http(s), and it worked fine.

[ToniE]

Ah, a single escape char before a pipe is an error:
"gtfs:route_id=de:mvv-muenchen:19-210\|210"
Invalid escape sequence (valid ones are \b \t \n \f \r \" \' \\ )Java(1610612990)

[stoecker]

You want to replace the text \| in the regexp, but \ ist the escape character and | an regexp symbol, so you get \\\|. And this needs to be escaped to pass through java (normal string escapes like \r, \n), so 6 is the result. Ugly, but correct.

Yes. In any Java (code) text \ must be presented as \\. That's better solved in other languages like perl, where there is no second stage escaping.

[ToniE]

Second stage or not does depend on whether you use double or single quotes in Perl, PHP, ...

IIRC

"\n" gives you an LF
'\n' gives you a \ and an n

[stoecker]

True, but that's not what I meant. In Perl you directly use a RegExp, e.g. $text =~ /(?<!\\)\|/, so the second escape isn't necessary (but you also CAN do it like in Java :-)

In Java you must always go through the normal escaping. That's caused by the history, because Java is derived from C and RegExp came much later.

Uuh! I didn't add a test AddTagsDialog.parseUrlTagsToKeyValues("gtfs:route_id=de:mvv-muenchen:19-210\|210"); with a single escape char. I don't know much about Java's escaping orgy, other languages aren't better though.

Will you add this and provide a new patch?

[ToniE]

No, I can't. The string has an invalid escape sequence.
See last line of comment 15.

[stoecker]

Seems you misunderstood last sentence in comment:16.
AddTagsDialog.parseUrlTagsToKeyValues("gtfs:route_id=de:mvv-muenchen:19-210\|210");
must be
AddTagsDialog.parseUrlTagsToKeyValues("gtfs:route_id=de:mvv-muenchen:19-210\\|210");
because you need to escape EACH string in Java. :-) That's the main reason for the many escapes. \| would create the ESC+\ character which does not exist, so you get an error. Other languages silently accept that as \+| although at least one other language (python?) recently started to complain as well.

BTW you need to escape each ' in Java as well for all GUI elements. Main work for maintainer in translations is to either replace ' in translated strings with '' or ’.

Java: The escaping language...

[ToniE]

Every thing is fine.
A test with ...\\|... is included, should be the first new one, IIRC.

[stoecker]

In 19571/josm:

fix #14490 - patch by ToniE - Support for escaping pipe character in remote control addtags parameters

[stoecker]

In 19572/josm:

see #14490 - ooops - fix revision