Index: /trunk/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java =================================================================== --- /trunk/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java (revision 8264) +++ /trunk/src/org/openstreetmap/josm/data/validation/tests/MapCSSTagChecker.java (revision 8265) @@ -141,10 +141,95 @@ } + /** + * Represents a fix to a validation test. The fixing {@link Command} can be obtained by {@link #createCommand(OsmPrimitive, Selector)}. + */ + static abstract class FixCommand { + /** + * Creates the fixing {@link Command} for the given primitive. The {@code matchingSelector} is used to + * evaluate placeholders (cf. {@link org.openstreetmap.josm.data.validation.tests.MapCSSTagChecker.TagCheck#insertArguments(Selector, String)}). + */ + abstract Command createCommand(final OsmPrimitive p, final Selector matchingSelector); + + private static void checkObject(final Object obj) { + CheckParameterUtil.ensureThat(obj instanceof Expression || obj instanceof String, "instance of Exception or String expected, but got " + obj); + } + + /** + * Evaluates given object as {@link Expression} or {@link String} on the matched {@link OsmPrimitive} and {@code matchingSelector}. + */ + private static String evaluateObject(final Object obj, final OsmPrimitive p, final Selector matchingSelector) { + final String s; + if (obj instanceof Expression) { + s = (String) ((Expression) obj).evaluate(new Environment().withPrimitive(p)); + } else if (obj instanceof String) { + s = (String) obj; + } else { + return null; + } + return TagCheck.insertArguments(matchingSelector, s); + } + + /** + * Creates a fixing command which executes a {@link ChangePropertyCommand} on the specified tag. + */ + static FixCommand fixAdd(final Object obj) { + checkObject(obj); + return new FixCommand() { + @Override + Command createCommand(OsmPrimitive p, Selector matchingSelector) { + final Tag tag = Tag.ofString(evaluateObject(obj, p, matchingSelector)); + return new ChangePropertyCommand(p, tag.getKey(), tag.getValue()); + } + + @Override + public String toString() { + return "fixAdd: " + obj; + } + }; + + } + + /** + * Creates a fixing command which executes a {@link ChangePropertyCommand} to delete the specified key. + */ + static FixCommand fixRemove(final Object obj) { + checkObject(obj); + return new FixCommand() { + @Override + Command createCommand(OsmPrimitive p, Selector matchingSelector) { + final String key = evaluateObject(obj, p, matchingSelector); + return new ChangePropertyCommand(p, key, ""); + } + + @Override + public String toString() { + return "fixRemove: " + obj; + } + }; + } + + /** + * Creates a fixing command which executes a {@link ChangePropertyKeyCommand} on the specified keys. + */ + static FixCommand fixChangeKey(final String oldKey, final String newKey) { + return new FixCommand() { + @Override + Command createCommand(OsmPrimitive p, Selector matchingSelector) { + return new ChangePropertyKeyCommand(p, oldKey, newKey); + } + + @Override + public String toString() { + return "fixChangeKey: " + oldKey + " => " + newKey; + } + }; + } + } + final MultiMap checks = new MultiMap<>(); static class TagCheck implements Predicate { protected final GroupedMapCSSRule rule; - protected final List change = new ArrayList<>(); - protected final Map keyChange = new LinkedHashMap<>(); + protected final List fixCommands = new ArrayList<>(); protected final List alternatives = new ArrayList<>(); protected final Map errors = new HashMap<>(); @@ -155,41 +240,4 @@ TagCheck(GroupedMapCSSRule rule) { this.rule = rule; - } - - /** - * A function mapping the matched {@link OsmPrimitive} to a {@link Tag}. - */ - abstract static class PrimitiveToTag implements Utils.Function { - - private PrimitiveToTag() { - // Hide implicit public constructor for utility class - } - - /** - * Creates a new mapping from an {@code MapCSS} object. - * In case of an {@link Expression}, that is evaluated on the matched {@link OsmPrimitive}. - * In case of a {@link String}, that is "compiled" to a {@link Tag} instance. - */ - static PrimitiveToTag ofMapCSSObject(final Object obj, final boolean keyOnly) { - if (obj instanceof Expression) { - return new PrimitiveToTag() { - @Override - public Tag apply(OsmPrimitive p) { - final String s = (String) ((Expression) obj).evaluate(new Environment().withPrimitive(p)); - return keyOnly? new Tag(s) : Tag.ofString(s); - } - }; - } else if (obj instanceof String) { - final Tag tag = keyOnly ? new Tag((String) obj) : Tag.ofString((String) obj); - return new PrimitiveToTag() { - @Override - public Tag apply(OsmPrimitive ignore) { - return tag; - } - }; - } else { - return null; - } - } } @@ -233,23 +281,13 @@ } } else if ("fixAdd".equals(ai.key)) { - final PrimitiveToTag toTag = PrimitiveToTag.ofMapCSSObject(ai.val, false); - if (toTag != null) { - check.change.add(toTag); - } else { - Main.warn("Invalid value for "+ai.key+": "+ai.val); - } + check.fixCommands.add(FixCommand.fixAdd(ai.val)); } else if ("fixRemove".equals(ai.key)) { CheckParameterUtil.ensureThat(!(ai.val instanceof String) || !(val != null && val.contains("=")), "Unexpected '='. Please only specify the key to remove!"); - final PrimitiveToTag toTag = PrimitiveToTag.ofMapCSSObject(ai.val, true); - if (toTag != null) { - check.change.add(toTag); - } else { - Main.warn("Invalid value for "+ai.key+": "+ai.val); - } + check.fixCommands.add(FixCommand.fixRemove(ai.val)); } else if ("fixChangeKey".equals(ai.key) && val != null) { CheckParameterUtil.ensureThat(val.contains("=>"), "Separate old from new key by '=>'!"); final String[] x = val.split("=>", 2); - check.keyChange.put(Tag.removeWhiteSpaces(x[0]), Tag.removeWhiteSpaces(x[1])); + check.fixCommands.add(FixCommand.fixChangeKey(Tag.removeWhiteSpaces(x[0]), Tag.removeWhiteSpaces(x[1]))); } else if ("fixDeleteObject".equals(ai.key) && val != null) { CheckParameterUtil.ensureThat(val.equals("this"), "fixDeleteObject must be followed by 'this'"); @@ -408,19 +446,11 @@ */ Command fixPrimitive(OsmPrimitive p) { - if (change.isEmpty() && keyChange.isEmpty() && !deletion) { + if (fixCommands.isEmpty() && !deletion) { return null; } final Selector matchingSelector = whichSelectorMatchesPrimitive(p); Collection cmds = new LinkedList<>(); - for (PrimitiveToTag toTag : change) { - final Tag tag = toTag.apply(p); - final String key = insertArguments(matchingSelector, tag.getKey()); - final String value = insertArguments(matchingSelector, tag.getValue()); - cmds.add(new ChangePropertyCommand(p, key, value)); - } - for (Map.Entry i : keyChange.entrySet()) { - final String oldKey = insertArguments(matchingSelector, i.getKey()); - final String newKey = insertArguments(matchingSelector, i.getValue()); - cmds.add(new ChangePropertyKeyCommand(p, oldKey, newKey)); + for (FixCommand fixCommand : fixCommands) { + cmds.add(fixCommand.createCommand(p, matchingSelector)); } if (deletion) { Index: /trunk/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java =================================================================== --- /trunk/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java (revision 8264) +++ /trunk/test/unit/org/openstreetmap/josm/data/validation/tests/MapCSSTagCheckerTest.java (revision 8265) @@ -8,4 +8,5 @@ import java.io.StringReader; +import java.util.Iterator; import java.util.LinkedHashSet; import java.util.List; @@ -17,6 +18,11 @@ import org.openstreetmap.josm.Main; import org.openstreetmap.josm.command.ChangePropertyCommand; +import org.openstreetmap.josm.command.ChangePropertyKeyCommand; +import org.openstreetmap.josm.command.Command; +import org.openstreetmap.josm.command.PseudoCommand; +import org.openstreetmap.josm.command.SequenceCommand; import org.openstreetmap.josm.data.osm.Node; -import org.openstreetmap.josm.data.osm.Tag; +import org.openstreetmap.josm.data.osm.OsmPrimitive; +import org.openstreetmap.josm.data.osm.OsmUtils; import org.openstreetmap.josm.data.validation.Severity; import org.openstreetmap.josm.data.validation.tests.MapCSSTagChecker.TagCheck; @@ -49,7 +55,7 @@ assertThat(check, notNullValue()); assertThat(check.getDescription(null), is("{0.key}=null is deprecated")); - assertThat(check.change.get(0).apply(null), is(new Tag("{0.key}"))); - assertThat(check.change.get(1).apply(null), is(new Tag("natural", "wetland"))); - assertThat(check.change.get(2).apply(null), is(new Tag("wetland", "marsh"))); + assertThat(check.fixCommands.get(0).toString(), is("fixRemove: {0.key}")); + assertThat(check.fixCommands.get(1).toString(), is("fixAdd: natural=wetland")); + assertThat(check.fixCommands.get(2).toString(), is("fixAdd: wetland=marsh")); final Node n1 = new Node(); n1.put("natural", "marsh"); @@ -65,4 +71,19 @@ assertThat(MapCSSTagChecker.TagCheck.insertArguments(check.rule.selectors.get(0), "The key is {0.key} and the value is {0.value}"), is("The key is natural and the value is marsh")); + } + + @Test + public void test10913() throws Exception { + final OsmPrimitive p = OsmUtils.createPrimitive("way highway=tertiary construction=yes"); + final TagCheck check = TagCheck.readMapCSS(new StringReader("way {" + + "throwError: \"error\";" + + "fixChangeKey: \"highway => construction\";\n" + + "fixAdd: \"highway=construction\";\n" + + "}")).get(0); + final Command command = check.fixPrimitive(p); + assertThat(command instanceof SequenceCommand, is(true)); + final Iterator it = command.getChildren().iterator(); + assertThat(it.next() instanceof ChangePropertyKeyCommand, is(true)); + assertThat(it.next() instanceof ChangePropertyCommand, is(true)); }