Skip to content

Commit 88c4c1b

Browse files
joscoljoscol
authored
Improve schema matching by considering the tree depth of errors (#52)
1 parent 23a31f5 commit 88c4c1b

File tree

2 files changed

+64
-36
lines changed

2 files changed

+64
-36
lines changed

‎language-service/CHANGELOG.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11

22
#### 0.5.3
33
Improve performance [#PR-51](https://github.com/Microsoft/azure-pipelines-language-server/pull/51)
4+
Improve error messages and suggestions [#PR-52](https://github.com/Microsoft/azure-pipelines-language-server/pull/52)
45

56
#### 0.5.2
67
Fix a regression with YAML structure errors [#PR-49](https://github.com/Microsoft/azure-pipelines-language-server/pull/49)

‎language-service/src/parser/jsonParser.ts‎

Lines changed: 63 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -174,7 +174,7 @@ export class ASTNode {
174174
isValid = (<string[]>schema.type).indexOf('string') >= 0;
175175
}
176176
if (!isValid) {
177-
validationResult.problems.push({
177+
validationResult.addProblem({
178178
location: { start: this.start, end: this.end },
179179
severity: ProblemSeverity.Warning,
180180
getMessage: () => schema.errorMessage || localize('typeArrayMismatchWarning', 'Incorrect type. Expected one of {0}.', (<string[]>schema.type).join(', '))
@@ -186,7 +186,7 @@ export class ASTNode {
186186
if (this.type !== schema.type) {
187187
//count strings that look like numbers as strings
188188
if (this.type != "number" || schema.type != "string") {
189-
validationResult.problems.push({
189+
validationResult.addProblem({
190190
location: { start: this.start, end: this.end },
191191
severity: ProblemSeverity.Warning,
192192
getMessage: () => schema.errorMessage || localize('typeMismatchWarning', 'Incorrect type. Expected "{0}".', schema.type)
@@ -204,7 +204,7 @@ export class ASTNode {
204204
let subMatchingSchemas = matchingSchemas.newSub();
205205
this.validate(schema.not, subValidationResult, subMatchingSchemas);
206206
if (!subValidationResult.hasProblems()) {
207-
validationResult.problems.push({
207+
validationResult.addProblem({
208208
location: { start: this.start, end: this.end },
209209
severity: ProblemSeverity.Warning,
210210
getMessage: () => localize('notSchemaWarning', "Matches a schema that is not allowed.")
@@ -224,7 +224,7 @@ export class ASTNode {
224224
const possibleMatches: JSONSchema[] = (Array.isArray(firstPropMatches) && firstPropMatches.length > 0) ? firstPropMatches : alternatives;
225225

226226
// remember the best match that is used for error messages
227-
let bestMatch: { schema: JSONSchema; validationResult: ValidationResult; matchingSchemas: ISchemaCollector; } = null;
227+
let bestMatch: SchemaMatch = null;
228228
possibleMatches.forEach((subSchema) => {
229229
let subValidationResult = new ValidationResult();
230230
let subMatchingSchemas = matchingSchemas.newSub();
@@ -243,15 +243,15 @@ export class ASTNode {
243243
});
244244

245245
if (matches.length > 1 && maxOneMatch && !this.parserSettings.isKubernetes) {
246-
validationResult.problems.push({
246+
validationResult.addProblem({
247247
location: { start: this.start, end: this.start + 1 },
248248
severity: ProblemSeverity.Warning,
249249
getMessage: () => localize('oneOfWarning', "Matches multiple schemas when only one must validate.")
250250
});
251251
}
252252

253253
if (bestMatch !== null) {
254-
validationResult.merge(bestMatch.validationResult);
254+
validationResult.mergeSubResult(bestMatch.validationResult);
255255
validationResult.propertiesMatches += bestMatch.validationResult.propertiesMatches;
256256
validationResult.propertiesValueMatches += bestMatch.validationResult.propertiesValueMatches;
257257
matchingSchemas.merge(bestMatch.matchingSchemas);
@@ -285,7 +285,7 @@ export class ASTNode {
285285
validationResult.enumValues = schema.enum;
286286
validationResult.enumValueMatch = enumValueMatch;
287287
if (!enumValueMatch) {
288-
validationResult.problems.push({
288+
validationResult.addProblem({
289289
location: { start: this.start, end: this.end },
290290
severity: ProblemSeverity.Warning,
291291
code: ErrorCode.EnumValueMismatch,
@@ -295,7 +295,7 @@ export class ASTNode {
295295
}
296296

297297
if (schema.deprecationMessage && this.parent) {
298-
validationResult.problems.push({
298+
validationResult.addProblem({
299299
location: { start: this.parent.start, end: this.parent.end },
300300
severity: ProblemSeverity.Hint,
301301
getMessage: () => schema.deprecationMessage
@@ -307,15 +307,15 @@ export class ASTNode {
307307

308308
protected validateStringValue(schema: JSONSchema, value: string, validationResult: ValidationResult): void {
309309
if (schema.minLength && value.length < schema.minLength) {
310-
validationResult.problems.push({
310+
validationResult.addProblem({
311311
location: { start: this.start, end: this.end },
312312
severity: ProblemSeverity.Warning,
313313
getMessage: () => localize('minLengthWarning', 'String is shorter than the minimum length of {0}.', schema.minLength)
314314
});
315315
}
316316

317317
if (schema.maxLength && value.length > schema.maxLength) {
318-
validationResult.problems.push({
318+
validationResult.addProblem({
319319
location: { start: this.start, end: this.end },
320320
severity: ProblemSeverity.Warning,
321321
getMessage: () => localize('maxLengthWarning', 'String is longer than the maximum length of {0}.', schema.maxLength)
@@ -326,7 +326,7 @@ export class ASTNode {
326326
const flags: string = ASTNode.getIgnoreValueCase(schema) ? "i" : "";
327327
const regex: RegExp = new RegExp(schema.pattern, flags);
328328
if (!regex.test(value)) {
329-
validationResult.problems.push({
329+
validationResult.addProblem({
330330
location: { start: this.start, end: this.end },
331331
severity: ProblemSeverity.Warning,
332332
getMessage: () => schema.patternErrorMessage || schema.errorMessage || localize('patternWarning', 'String does not match the pattern of "{0}".', schema.pattern)
@@ -442,7 +442,7 @@ export class ArrayASTNode extends ASTNode {
442442
validationResult.mergePropertyMatch(itemValidationResult);
443443
}
444444
} else if (schema.additionalItems === false) {
445-
validationResult.problems.push({
445+
validationResult.addProblem({
446446
location: { start: this.start, end: this.end },
447447
severity: ProblemSeverity.Warning,
448448
getMessage: () => localize('additionalItemsWarning', 'Array has too many items according to schema. Expected {0} or fewer.', subSchemas.length)
@@ -459,15 +459,15 @@ export class ArrayASTNode extends ASTNode {
459459
}
460460

461461
if (schema.minItems && this.items.length < schema.minItems) {
462-
validationResult.problems.push({
462+
validationResult.addProblem({
463463
location: { start: this.start, end: this.end },
464464
severity: ProblemSeverity.Warning,
465465
getMessage: () => localize('minItemsWarning', 'Array has too few items. Expected {0} or more.', schema.minItems)
466466
});
467467
}
468468

469469
if (schema.maxItems && this.items.length > schema.maxItems) {
470-
validationResult.problems.push({
470+
validationResult.addProblem({
471471
location: { start: this.start, end: this.end },
472472
severity: ProblemSeverity.Warning,
473473
getMessage: () => localize('maxItemsWarning', 'Array has too many items. Expected {0} or fewer.', schema.maxItems)
@@ -482,7 +482,7 @@ export class ArrayASTNode extends ASTNode {
482482
return index !== values.lastIndexOf(value);
483483
});
484484
if (duplicates) {
485-
validationResult.problems.push({
485+
validationResult.addProblem({
486486
location: { start: this.start, end: this.end },
487487
severity: ProblemSeverity.Warning,
488488
getMessage: () => localize('uniqueItemsWarning', 'Array has duplicate items.')
@@ -533,7 +533,7 @@ export class NumberASTNode extends ASTNode {
533533

534534
if (typeof schema.multipleOf === 'number') {
535535
if (val % schema.multipleOf !== 0) {
536-
validationResult.problems.push({
536+
validationResult.addProblem({
537537
location: { start: this.start, end: this.end },
538538
severity: ProblemSeverity.Warning,
539539
getMessage: () => localize('multipleOfWarning', 'Value is not divisible by {0}.', schema.multipleOf)
@@ -543,14 +543,14 @@ export class NumberASTNode extends ASTNode {
543543

544544
if (typeof schema.minimum === 'number') {
545545
if (schema.exclusiveMinimum && val <= schema.minimum) {
546-
validationResult.problems.push({
546+
validationResult.addProblem({
547547
location: { start: this.start, end: this.end },
548548
severity: ProblemSeverity.Warning,
549549
getMessage: () => localize('exclusiveMinimumWarning', 'Value is below the exclusive minimum of {0}.', schema.minimum)
550550
});
551551
}
552552
if (!schema.exclusiveMinimum && val < schema.minimum) {
553-
validationResult.problems.push({
553+
validationResult.addProblem({
554554
location: { start: this.start, end: this.end },
555555
severity: ProblemSeverity.Warning,
556556
getMessage: () => localize('minimumWarning', 'Value is below the minimum of {0}.', schema.minimum)
@@ -560,14 +560,14 @@ export class NumberASTNode extends ASTNode {
560560

561561
if (typeof schema.maximum === 'number') {
562562
if (schema.exclusiveMaximum && val >= schema.maximum) {
563-
validationResult.problems.push({
563+
validationResult.addProblem({
564564
location: { start: this.start, end: this.end },
565565
severity: ProblemSeverity.Warning,
566566
getMessage: () => localize('exclusiveMaximumWarning', 'Value is above the exclusive maximum of {0}.', schema.maximum)
567567
});
568568
}
569569
if (!schema.exclusiveMaximum && val > schema.maximum) {
570-
validationResult.problems.push({
570+
validationResult.addProblem({
571571
location: { start: this.start, end: this.end },
572572
severity: ProblemSeverity.Warning,
573573
getMessage: () => localize('maximumWarning', 'Value is above the maximum of {0}.', schema.maximum)
@@ -804,7 +804,7 @@ export class ObjectASTNode extends ASTNode {
804804
if (!hasProperty(propertyName)) {
805805
const key = this.parent && (<PropertyASTNode>this.parent).key;
806806
const location = key ? { start: key.start, end: key.end } : { start: this.start, end: this.start + 1 };
807-
validationResult.problems.push({
807+
validationResult.addProblem({
808808
location: location,
809809
severity: ProblemSeverity.Warning,
810810
getMessage: () => localize('MissingRequiredPropWarning', 'Missing property "{0}".', propertyName)
@@ -872,7 +872,7 @@ export class ObjectASTNode extends ASTNode {
872872

873873
if (generateErrors) {
874874
const childProperty: PropertyASTNode = <PropertyASTNode>(children[childKey].parent);
875-
validationResult.problems.push({
875+
validationResult.addProblem({
876876
location: {start: childProperty.key.start, end: childProperty.key.end},
877877
severity: ProblemSeverity.Error,
878878
getMessage: () => localize('DuplicatePropError', 'Multiple properties found matching {0}', schemaPropertyName)
@@ -945,7 +945,7 @@ export class ObjectASTNode extends ASTNode {
945945
}else{
946946
propertyNode = child;
947947
}
948-
validationResult.problems.push({
948+
validationResult.addProblem({
949949
location: { start: propertyNode.key.start, end: propertyNode.key.end },
950950
severity: ProblemSeverity.Warning,
951951
getMessage: () => schema.errorMessage || localize('DisallowedExtraPropWarning', 'Unexpected property {0}', propertyName)
@@ -958,7 +958,7 @@ export class ObjectASTNode extends ASTNode {
958958

959959
if (schema.maxProperties) {
960960
if (this.properties.length > schema.maxProperties) {
961-
validationResult.problems.push({
961+
validationResult.addProblem({
962962
location: { start: this.start, end: this.end },
963963
severity: ProblemSeverity.Warning,
964964
getMessage: () => localize('MaxPropWarning', 'Object has more properties than limit of {0}.', schema.maxProperties)
@@ -968,7 +968,7 @@ export class ObjectASTNode extends ASTNode {
968968

969969
if (schema.minProperties) {
970970
if (this.properties.length < schema.minProperties) {
971-
validationResult.problems.push({
971+
validationResult.addProblem({
972972
location: { start: this.start, end: this.end },
973973
severity: ProblemSeverity.Warning,
974974
getMessage: () => localize('MinPropWarning', 'Object has fewer properties than the required number of {0}', schema.minProperties)
@@ -983,7 +983,7 @@ export class ObjectASTNode extends ASTNode {
983983
if (Array.isArray(propertyDep)) {
984984
propertyDep.forEach((requiredProp: string) => {
985985
if (!hasProperty(requiredProp)) {
986-
validationResult.problems.push({
986+
validationResult.addProblem({
987987
location: { start: this.start, end: this.end },
988988
severity: ProblemSeverity.Warning,
989989
getMessage: () => localize('RequiredDependentPropWarning', 'Object is missing property {0} required by property {1}.', requiredProp, key)
@@ -1038,14 +1038,14 @@ export class ObjectASTNode extends ASTNode {
10381038
validationResult.firstPropertyProblems++;
10391039

10401040
if (schema.firstProperty.length == 1) {
1041-
validationResult.problems.push({
1041+
validationResult.addProblem({
10421042
location: { start: firstProperty.start, end: firstProperty.end },
10431043
severity: ProblemSeverity.Error,
10441044
getMessage: () => localize('firstPropertyError', "The first property must be {0}", schema.firstProperty[0])
10451045
});
10461046
}
10471047
else {
1048-
validationResult.problems.push({
1048+
validationResult.addProblem({
10491049
location: { start: firstProperty.start, end: firstProperty.end },
10501050
severity: ProblemSeverity.Error,
10511051
getMessage: () => {
@@ -1134,6 +1134,12 @@ export interface ISchemaCollector {
11341134
newSub(): ISchemaCollector;
11351135
}
11361136

1137+
interface SchemaMatch {
1138+
schema: JSONSchema;
1139+
validationResult: ValidationResult;
1140+
matchingSchemas: ISchemaCollector;
1141+
}
1142+
11371143
class SchemaCollector implements ISchemaCollector {
11381144
schemas: IApplicableSchema[] = [];
11391145
constructor(private focusOffset = -1, private exclude: ASTNode = null) {
@@ -1162,6 +1168,7 @@ class NoOpSchemaCollector implements ISchemaCollector {
11621168

11631169
export class ValidationResult {
11641170
public problems: IProblem[];
1171+
public problemDepths: number[]; //count of problems found at each level of the parse tree. problemDepths[0] is the root, problemDepths[1] is one level down, etc.
11651172

11661173
public firstPropertyProblems: number;
11671174
public propertiesMatches: number;
@@ -1172,6 +1179,7 @@ export class ValidationResult {
11721179

11731180
constructor() {
11741181
this.problems = [];
1182+
this.problemDepths = [0];
11751183
this.firstPropertyProblems = 0;
11761184
this.propertiesMatches = 0;
11771185
this.propertiesValueMatches = 0;
@@ -1184,14 +1192,26 @@ export class ValidationResult {
11841192
return !!this.problems.length;
11851193
}
11861194

1187-
public mergeAll(validationResults: ValidationResult[]): void {
1188-
validationResults.forEach((validationResult) => {
1189-
this.merge(validationResult);
1190-
});
1195+
public addProblem(problem: IProblem): void {
1196+
this.problems.push(problem);
1197+
this.problemDepths[0]++;
11911198
}
11921199

1193-
public merge(validationResult: ValidationResult): void {
1200+
public mergeSubResult(validationResult: ValidationResult): void {
11941201
this.problems = this.problems.concat(validationResult.problems);
1202+
1203+
//overlay the problem count array one level down
1204+
1205+
//first make sure the array is long enough
1206+
const subDepth: number = validationResult.problemDepths.length;
1207+
while (this.problemDepths.length <= subDepth) {
1208+
this.problemDepths.push(0);
1209+
}
1210+
1211+
//then add the problem counts shifted lower in the parse tree
1212+
validationResult.problemDepths.forEach((problemCount: number, depth: number) => {
1213+
this.problemDepths[depth + 1] += problemCount;
1214+
});
11951215
}
11961216

11971217
public mergeEnumValues(validationResult: ValidationResult): void {
@@ -1206,7 +1226,7 @@ export class ValidationResult {
12061226
}
12071227

12081228
public mergePropertyMatch(propertyValidationResult: ValidationResult): void {
1209-
this.merge(propertyValidationResult);
1229+
this.mergeSubResult(propertyValidationResult);
12101230
this.firstPropertyProblems += propertyValidationResult.firstPropertyProblems;
12111231
this.propertiesMatches++;
12121232
if (propertyValidationResult.enumValueMatch || !this.hasProblems() && propertyValidationResult.propertiesMatches) {
@@ -1226,6 +1246,13 @@ export class ValidationResult {
12261246
if (hasProblems !== other.hasProblems()) {
12271247
return hasProblems ? -1 : 1;
12281248
}
1249+
if (hasProblems) {
1250+
const depthOfFirstProblem: number = this.problemDepths.findIndex((value: number, index: number) => value > 0);
1251+
const depthOfOtherFirstProblem: number = other.problemDepths.findIndex((value: number, index: number) => value > 0);
1252+
if (depthOfFirstProblem != depthOfOtherFirstProblem) {
1253+
return depthOfFirstProblem - depthOfOtherFirstProblem;
1254+
}
1255+
}
12291256
if (this.enumValueMatch !== other.enumValueMatch) {
12301257
return other.enumValueMatch ? -1 : 1;
12311258
}
@@ -1313,7 +1340,7 @@ export class JSONDocument {
13131340
}
13141341

13151342
//Alternative comparison is specifically used by the kubernetes/openshift schema but may lead to better results then genericComparison depending on the schema
1316-
function alternativeComparison(subValidationResult, bestMatch, subSchema, subMatchingSchemas){
1343+
function alternativeComparison(subValidationResult: ValidationResult, bestMatch: SchemaMatch, subSchema: JSONSchema, subMatchingSchemas: ISchemaCollector): SchemaMatch {
13171344
let compareResult = subValidationResult.compareKubernetes(bestMatch.validationResult);
13181345
if (compareResult > 0) {
13191346
// our node is the best matching so far
@@ -1327,7 +1354,7 @@ function alternativeComparison(subValidationResult, bestMatch, subSchema, subMat
13271354
}
13281355

13291356
//genericComparison tries to find the best matching schema using a generic comparison
1330-
function genericComparison(maxOneMatch, subValidationResult, bestMatch, subSchema, subMatchingSchemas){
1357+
function genericComparison(maxOneMatch: boolean, subValidationResult: ValidationResult, bestMatch: SchemaMatch, subSchema: JSONSchema, subMatchingSchemas: ISchemaCollector): SchemaMatch {
13311358
if (!maxOneMatch && !subValidationResult.hasProblems() && !bestMatch.validationResult.hasProblems()) {
13321359
// no errors, both are equally good matches
13331360
bestMatch.matchingSchemas.merge(subMatchingSchemas);

0 commit comments

Comments
 (0)