Skip to content

Commit 0e9620d

Browse files
ljdelightljdelight
authored
Merge pull request #86 from osmlab/ljdelight/improveRuleListParsing
Improve the validation of rule lists
2 parents 453a01f + a2cf69e commit 0e9620d

File tree

13 files changed

+384
-44
lines changed

13 files changed

+384
-44
lines changed

‎build.gradle‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,9 @@ plugins {
1010
id "com.diffplug.gradle.spotless" version "3.27.0"
1111
id 'org.sonarqube' version '2.8'
1212
id "com.github.spotbugs" version "4.8.0"
13+
14+
// Lombok plugin and gradle compat matrix https://stackoverflow.com/a/63736421
15+
id "io.freefair.lombok" version "5.0.1"
1316
}
1417

1518
apply from: 'dependencies.gradle'

‎src/integrationTest/java/org/maproulette/client/api/ChallengeAPIIntegrationTest.java‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -80,7 +80,7 @@ public void updateTest() throws MapRouletteException
8080
.name("UpdatedName").defaultPriority(ChallengePriority.LOW)
8181
.highPriorityRule(this.getRuleList("OR", "pr.high"))
8282
.mediumPriorityRule(this.getRuleList("AND", "pr.medium"))
83-
.lowPriorityRule(this.getRuleList("or", "pr.low")).defaultZoom(11).minZoom(10)
83+
.lowPriorityRule(this.getRuleList("OR", "pr.low")).defaultZoom(11).minZoom(10)
8484
.maxZoom(12).defaultBasemapId("defaultBaseMap").defaultBasemap(67)
8585
.customBasemap("customBasemap").build();
8686

@@ -115,6 +115,7 @@ public void updateTest() throws MapRouletteException
115115
updatedChallenge.getDefaultBasemap());
116116
Assertions.assertNotEquals(createdChallenge.getCustomBasemap(),
117117
updatedChallenge.getCustomBasemap());
118+
Assertions.assertTrue(updatedChallenge.getHighPriorityRule().getCondition().equals("OR"));
118119
}
119120

120121
@Test
@@ -164,8 +165,9 @@ public void childrenTest() throws MapRouletteException
164165
private RuleList getRuleList(final String condition, final String value)
165166
{
166167
return RuleList.builder().condition(condition).rules(Collections.singletonList(
167-
PriorityRule.builder().operator("equals").type("string").value(value).build()))
168+
PriorityRule.builder().operator("equal").type("string").value(value).build()))
168169
.build();
170+
169171
}
170172

171173
private void compareChallenges(final Challenge challenge1, final Challenge challenge2)

‎src/main/java/org/maproulette/client/exception/MapRouletteRuntimeParseException.java‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,24 @@
1+
package org.maproulette.client.exception;
2+
3+
/**
4+
* An exception class used to identify, mostly, json parsing errors.
5+
*
6+
* @author ljdelight
7+
*/
8+
public class MapRouletteRuntimeParseException extends MapRouletteRuntimeException
9+
{
10+
public MapRouletteRuntimeParseException(final Throwable e)
11+
{
12+
super(e);
13+
}
14+
15+
public MapRouletteRuntimeParseException(final String message, final Throwable e)
16+
{
17+
super(message, e);
18+
}
19+
20+
public MapRouletteRuntimeParseException(final String message)
21+
{
22+
super(message);
23+
}
24+
}

‎src/main/java/org/maproulette/client/model/PriorityRule.java‎

Lines changed: 46 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,23 +1,63 @@
11
package org.maproulette.client.model;
22

33
import java.io.Serializable;
4+
import java.util.Map;
5+
import java.util.Set;
6+
7+
import org.maproulette.client.exception.MapRouletteRuntimeParseException;
48

5-
import lombok.AllArgsConstructor;
69
import lombok.Builder;
7-
import lombok.Data;
8-
import lombok.NoArgsConstructor;
10+
import lombok.Value;
911

1012
/**
1113
* @author mcuthbert
1214
*/
1315
@Builder
14-
@Data
15-
@NoArgsConstructor
16-
@AllArgsConstructor
16+
@Value
1717
public class PriorityRule implements Serializable
1818
{
19+
private static final Set<String> NUMBER_COMPARISON_OPERATORS = Set.of("==", "!=", "<", "<=",
20+
">", ">=");
21+
private static final Set<String> STRING_COMPARISON_OPERATORS = Set.of("equal", "not_equal",
22+
"contains", "not_contains", "is_empty", "is_not_empty");
23+
private static final Map<String, Set<String>> TYPE_TO_OPERATORS = Map.of("bounds",
24+
Set.of("contains", "not_contains"), "string", STRING_COMPARISON_OPERATORS, "integer",
25+
NUMBER_COMPARISON_OPERATORS, "long", NUMBER_COMPARISON_OPERATORS, "double",
26+
NUMBER_COMPARISON_OPERATORS);
1927
private static final long serialVersionUID = -7443371611488972313L;
2028
private String value;
2129
private String type;
2230
private String operator;
31+
32+
public static PriorityRuleBuilder builder()
33+
{
34+
return new PriorityRuleBuilderCustom();
35+
}
36+
37+
/**
38+
* Extend the generated PriorityRuleBuilder for additional validation of the fields.
39+
*/
40+
private static class PriorityRuleBuilderCustom extends PriorityRuleBuilder
41+
{
42+
public PriorityRule build()
43+
{
44+
if (!TYPE_TO_OPERATORS.containsKey(super.type))
45+
{
46+
throw new MapRouletteRuntimeParseException(
47+
String.format("Type '%s' is not supported by Priority Rules", super.type));
48+
}
49+
if (!TYPE_TO_OPERATORS.get(super.type).contains(super.operator))
50+
{
51+
throw new MapRouletteRuntimeParseException(String.format(
52+
"Operator '%s' is not supported by type '%s'", super.operator, super.type));
53+
}
54+
if (super.value.split("\\.").length != 2)
55+
{
56+
throw new MapRouletteRuntimeParseException(
57+
String.format("value '%s' must contain exactly one '.'", super.value));
58+
}
59+
60+
return new PriorityRule(super.value, super.type, super.operator);
61+
}
62+
}
2363
}

‎src/main/java/org/maproulette/client/model/RuleList.java‎

Lines changed: 74 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,9 @@
55
import java.io.StringWriter;
66
import java.util.ArrayList;
77
import java.util.List;
8+
import java.util.Set;
9+
10+
import org.maproulette.client.exception.MapRouletteRuntimeParseException;
811

912
import com.fasterxml.jackson.core.JsonFactory;
1013
import com.fasterxml.jackson.core.JsonGenerator;
@@ -23,6 +26,7 @@
2326
import lombok.Data;
2427
import lombok.NoArgsConstructor;
2528
import lombok.NonNull;
29+
import lombok.Value;
2630
import lombok.extern.slf4j.Slf4j;
2731

2832
/**
@@ -34,9 +38,14 @@
3438
@AllArgsConstructor
3539
@JsonDeserialize(using = RuleList.RuleListDeserializer.class)
3640
@JsonSerialize(using = RuleList.RuleListSerializer.class)
41+
@Value
3742
@Slf4j
3843
public class RuleList implements Serializable
3944
{
45+
public static final String CONDITION_AND = "AND";
46+
public static final String CONDITION_OR = "OR";
47+
private static final Set<String> VALID_CONDITIONS = Set.of(CONDITION_AND, CONDITION_OR);
48+
4049
private static final String KEY_CONDITION = "condition";
4150
private static final String KEY_RULES = "rules";
4251
private static final long serialVersionUID = -1085774480815117637L;
@@ -55,11 +64,13 @@ public class RuleList implements Serializable
5564

5665
public boolean isSet()
5766
{
58-
// A condition is needed
59-
if (this.condition == null || this.condition.isEmpty())
67+
// The rule list is "set" if the condition is empty, and there are no rules, and there are
68+
// no nested priority rules
69+
if (this.condition.isEmpty() && this.rules.isEmpty() && this.ruleList.isEmpty())
6070
{
6171
return false;
6272
}
73+
6374
// It is possible to have an empty 'rules' and a non-empty nested rule list.
6475
// It is invalid for both to be empty at the same time.
6576
if (this.rules.isEmpty() && this.ruleList.isEmpty())
@@ -90,22 +101,25 @@ private static void serializeRuleListHelper(final List<RuleList> ruleListList,
90101
{
91102
for (final RuleList ruleList : ruleListList)
92103
{
104+
final String condition = ruleList.getCondition();
105+
// For nested rule lists (eg rule lists that have a parent), these REQUIRE the
106+
// condition to be non-empty and valid.
107+
if (!CONDITION_AND.equals(condition) && !CONDITION_OR.equals(condition))
108+
{
109+
throw new MapRouletteRuntimeParseException(
110+
String.format("Condition '%s' is not known", condition));
111+
}
112+
93113
gen.writeStartObject();
94-
gen.writeStringField("condition", ruleList.getCondition());
95-
gen.writeArrayFieldStart("rules");
96-
if (ruleList.getRuleList() != null)
114+
gen.writeStringField(KEY_CONDITION, ruleList.getCondition());
115+
gen.writeArrayFieldStart(KEY_RULES);
116+
for (final RuleList nestedRuleList : ruleList.getRuleList())
97117
{
98-
for (final RuleList nestedRuleList : ruleList.getRuleList())
99-
{
100-
serializeRuleListHelper(nestedRuleList.getRuleList(), gen);
101-
}
118+
serializeRuleListHelper(nestedRuleList.getRuleList(), gen);
102119
}
103-
if (ruleList.getRules() != null)
120+
for (final PriorityRule priorityRule : ruleList.getRules())
104121
{
105-
for (final PriorityRule priorityRule : ruleList.getRules())
106-
{
107-
gen.writeObject(priorityRule);
108-
}
122+
gen.writeObject(priorityRule);
109123
}
110124
gen.writeEndArray();
111125
gen.writeEndObject();
@@ -115,21 +129,24 @@ private static void serializeRuleListHelper(final List<RuleList> ruleListList,
115129
private void serializeRuleListAsObject(final RuleList value, final JsonGenerator gen,
116130
final SerializerProvider serializers) throws IOException
117131
{
118-
gen.writeStartObject();
119-
gen.writeStringField("condition", value.getCondition());
120-
gen.writeArrayFieldStart("rules");
121-
if (value.getRuleList() != null)
132+
final String condition = value.getCondition();
133+
// A condition must be valid
134+
if (!VALID_CONDITIONS.contains(condition))
122135
{
123-
serializeRuleListHelper(value.getRuleList(), gen);
136+
throw new MapRouletteRuntimeParseException(
137+
String.format("Condition '%s' is not known", condition));
124138
}
125139

126-
if (value.getRules() != null)
140+
gen.writeStartObject();
141+
gen.writeStringField(KEY_CONDITION, condition);
142+
gen.writeArrayFieldStart(KEY_RULES);
143+
144+
serializeRuleListHelper(value.getRuleList(), gen);
145+
for (final PriorityRule priorityRule : value.getRules())
127146
{
128-
for (final PriorityRule priorityRule : value.getRules())
129-
{
130-
gen.writeObject(priorityRule);
131-
}
147+
gen.writeObject(priorityRule);
132148
}
149+
133150
gen.writeEndArray();
134151
gen.writeEndObject();
135152
gen.flush();
@@ -188,35 +205,58 @@ public RuleListDeserializer(final Class<?> valueClass)
188205
private static RuleList buildRuleListHelper(final JsonNode node,
189206
final DeserializationContext ctxt)
190207
{
191-
// When the serialized format lacks required values, just create an empty RuleList to
192-
// avoid NPE.
193-
if (node.get("condition") == null)
208+
final JsonNode conditionNode = node.get(KEY_CONDITION);
209+
final JsonNode rulesNode = node.get(KEY_RULES);
210+
211+
// If NEITHER a condition nor rules is in the object, return a dummy RuleList which will
212+
// serialize to '{}'.
213+
if (conditionNode == null && rulesNode == null)
194214
{
195215
return RuleList.builder().build();
196216
}
197-
final RuleList ret = RuleList.builder().condition(node.get("condition").asText())
198-
.ruleList(new ArrayList<>()).rules(new ArrayList<>()).build();
199217

200-
for (final JsonNode jsonNode : node.withArray("rules"))
218+
// Both the 'condition' and 'rules' must appear, otherwise it is an invalid object.
219+
if (conditionNode == null || rulesNode == null)
220+
{
221+
throw new MapRouletteRuntimeParseException(
222+
"parse error: the rulelist requires both a 'condition' and 'rules' field");
223+
}
224+
225+
if (!VALID_CONDITIONS.contains(conditionNode.asText()))
201226
{
202-
if (jsonNode.get("condition") != null)
227+
throw new MapRouletteRuntimeParseException(
228+
String.format("Condition '%s' is not known", conditionNode.asText()));
229+
}
230+
231+
final RuleListBuilder ret = RuleList.builder().condition(conditionNode.asText())
232+
.ruleList(new ArrayList<>()).rules(new ArrayList<>());
233+
234+
for (final JsonNode jsonNode : node.withArray(KEY_RULES))
235+
{
236+
if (jsonNode.get(KEY_CONDITION) != null || jsonNode.get(KEY_RULES) != null)
203237
{
204238
// If the child is a PriorityRule, do a recursive call to build the rule and add
205239
// it to the list.
206240
final RuleList child = buildRuleListHelper(jsonNode, ctxt);
207-
ret.getRuleList().add(child);
241+
ret.ruleList$value.add(child);
208242
}
209243
else
210244
{
245+
if (jsonNode.get("type") == null || jsonNode.get("operator") == null
246+
|| jsonNode.get("value") == null)
247+
{
248+
throw new MapRouletteRuntimeParseException(
249+
"Nested object is not a PriorityRule nor a RuleList! Parsing failed!");
250+
}
211251
final PriorityRule priorityRule = PriorityRule.builder()
212252
.type(jsonNode.get("type").asText())
213253
.operator(jsonNode.get("operator").asText())
214254
.value(jsonNode.get("value").asText()).build();
215-
ret.getRules().add(priorityRule);
255+
ret.rules$value.add(priorityRule);
216256
}
217257
}
218258

219-
return ret;
259+
return ret.build();
220260
}
221261

222262
/**

‎src/test/java/org/maproulette/client/model/ChallengeTest.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,7 @@ public void challengePriorityBuilderTest()
4646
.instruction("TestInstruction")
4747
.highPriorityRule(RuleList.builder().condition("AND")
4848
.rules(Collections.singletonList(PriorityRule.builder().operator("equal")
49-
.type("string").value("testValue").build()))
49+
.type("string").value("test.value").build()))
5050
.build())
5151
.build();
5252
final var rule = challenge.getHighPriorityRule();
@@ -57,7 +57,7 @@ public void challengePriorityBuilderTest()
5757
final var onlyRule = ruleList.get(0);
5858
Assertions.assertEquals("equal", onlyRule.getOperator());
5959
Assertions.assertEquals("string", onlyRule.getType());
60-
Assertions.assertEquals("testValue", onlyRule.getValue());
60+
Assertions.assertEquals("test.value", onlyRule.getValue());
6161
}
6262

6363
@Test

0 commit comments

Comments
 (0)