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()"

Oops! Sent as "anonym"

[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