Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: anyOf compare alternatives #1037

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 53 additions & 28 deletions src/languageservice/parser/jsonParser07.ts
Original file line number Diff line number Diff line change
Expand Up @@ -533,6 +533,12 @@ export function findNodeAtOffset(node: ASTNode, offset: number, includeRightBoun
return undefined;
}

interface IValidationMatch {
schema: JSONSchema;
validationResult: ValidationResult;
matchingSchemas: ISchemaCollector;
}

export class JSONDocument {
public isKubernetes: boolean;
public disableAdditionalProperties: boolean;
Expand Down Expand Up @@ -866,7 +872,7 @@ function validate(
const val = getNodeValue(node);
let enumValueMatch = false;
for (const e of schema.enum) {
if (equals(val, e) || (callFromAutoComplete && isString(val) && isString(e) && val && e.startsWith(val))) {
if (equals(val, e) || isAutoCompleteEqualMaybe(callFromAutoComplete, node, val, e)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null value is a good match in auto-completion

enumValueMatch = true;
break;
}
Expand Down Expand Up @@ -898,10 +904,7 @@ function validate(

if (isDefined(schema.const)) {
const val = getNodeValue(node);
if (
!equals(val, schema.const) &&
!(callFromAutoComplete && isString(val) && isString(schema.const) && schema.const.startsWith(val))
) {
if (!equals(val, schema.const) && !isAutoCompleteEqualMaybe(callFromAutoComplete, node, val, schema.const)) {
validationResult.problems.push({
location: { offset: node.offset, length: node.length },
severity: DiagnosticSeverity.Warning,
Expand Down Expand Up @@ -1504,23 +1507,11 @@ function validate(
node: ASTNode,
maxOneMatch,
subValidationResult: ValidationResult,
bestMatch: {
schema: JSONSchema;
validationResult: ValidationResult;
matchingSchemas: ISchemaCollector;
},
bestMatch: IValidationMatch,
subSchema,
subMatchingSchemas
): {
schema: JSONSchema;
validationResult: ValidationResult;
matchingSchemas: ISchemaCollector;
} {
if (
!maxOneMatch &&
!subValidationResult.hasProblems() &&
(!bestMatch.validationResult.hasProblems() || callFromAutoComplete)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(!bestMatch.validationResult.hasProblems() || callFromAutoComplete) didn't solve the previous problem properly
problem was when the order of the subschemas in anyOf changed.
this previous fix allows 'issues' only on the previous bestMatch

) {
): IValidationMatch {
if (!maxOneMatch && !subValidationResult.hasProblems() && !bestMatch.validationResult.hasProblems()) {
// no errors, both are equally good matches
bestMatch.matchingSchemas.merge(subMatchingSchemas);
bestMatch.validationResult.propertiesMatches += subValidationResult.propertiesMatches;
Expand All @@ -1541,19 +1532,30 @@ function validate(
validationResult: subValidationResult,
matchingSchemas: subMatchingSchemas,
};
} else if (compareResult === 0) {
} else if (
compareResult === 0 ||
((node.value === null || node.type === 'null') && node.length === 0) // node with no value can match any schema potentially
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null yaml value is a good match for sub-schemas, similar to isAutoCompleteEqualMaybe for const and enums

) {
// there's already a best matching but we are as good
bestMatch.matchingSchemas.merge(subMatchingSchemas);
bestMatch.validationResult.mergeEnumValues(subValidationResult);
bestMatch.validationResult.mergeWarningGeneric(subValidationResult, [
ProblemType.missingRequiredPropWarning,
ProblemType.typeMismatchWarning,
ProblemType.constWarning,
]);
mergeValidationMatches(bestMatch, subMatchingSchemas, subValidationResult);
}
}
return bestMatch;
}

function mergeValidationMatches(
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only refactor - move to separate function, no functional change

bestMatch: IValidationMatch,
subMatchingSchemas: ISchemaCollector,
subValidationResult: ValidationResult
): void {
bestMatch.matchingSchemas.merge(subMatchingSchemas);
bestMatch.validationResult.mergeEnumValues(subValidationResult);
bestMatch.validationResult.mergeWarningGeneric(subValidationResult, [
ProblemType.missingRequiredPropWarning,
ProblemType.typeMismatchWarning,
ProblemType.constWarning,
]);
}
}

function getSchemaSource(schema: JSONSchema, originalSchema: JSONSchema): string | undefined {
Expand Down Expand Up @@ -1591,3 +1593,26 @@ function getSchemaUri(schema: JSONSchema, originalSchema: JSONSchema): string[]
function getWarningMessage(problemType: ProblemType, args: string[]): string {
return localize(problemType, ProblemTypeMessages[problemType], args.join(' | '));
}

/**
* if callFromAutoComplete than compare value from yaml and value from schema (s.const | s.enum[i])
* allows partial match for autocompletion
*/
function isAutoCompleteEqualMaybe(
callFromAutoComplete: boolean,
node: ASTNode,
nodeValue: unknown,
schemaValue: unknown
): boolean {
if (!callFromAutoComplete) {
return false;
}

// if autocompletion property doesn't have value, then it could be a match
const isWithoutValue = nodeValue === null && node.length === 0; // allows `prop: ` but ignore `prop: null`
if (isWithoutValue) {
return true;
}

return isString(nodeValue) && isString(schemaValue) && schemaValue.startsWith(nodeValue);
}
151 changes: 151 additions & 0 deletions test/autoCompletionFix.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -759,6 +759,157 @@ objB:
expect(completion.items[0].label).to.be.equal('prop');
expect(completion.items[0].insertText).to.be.equal('prop: ${1|const value,null|}');
});
it('should take all sub-schemas when value has not been set (cursor in the middle of the empty space)', async () => {
const schema: JSONSchema = {
anyOf: [
{
properties: {
prop: { type: 'null' },
},
},
{
properties: {
prop: { const: 'const value' },
},
},
{
properties: {
prop: { const: 5 },
},
},
],
};
schemaProvider.addSchema(SCHEMA_ID, schema);
const content = 'prop: | | ';
const completion = await parseCaret(content);
expect(completion.items.map((i) => i.label)).to.be.deep.eq(['const value', '5', 'null']);
});
it('should take only null sub-schema when value is "null"', async () => {
const schema: JSONSchema = {
anyOf: [
{
properties: {
prop: { type: 'null' },
},
},
{
properties: {
prop: { const: 'const value' },
},
},
],
};
schemaProvider.addSchema(SCHEMA_ID, schema);
const content = 'prop: null';
const completion = await parseSetup(content, 0, content.length);
expect(completion.items.map((i) => i.label)).to.be.deep.eq(['null']);
});
it('should take only one sub-schema because first sub-schema does not match', async () => {
const schema: JSONSchema = {
anyOf: [
{
properties: {
prop: { const: 'const value' },
},
},
{
properties: {
prop: { const: 'const value2' },
},
},
],
};
schemaProvider.addSchema(SCHEMA_ID, schema);
const content = 'prop: const value2';
const completion = await parseSetup(content, 0, content.length);
expect(completion.items.map((i) => i.label)).to.be.deep.eq(['const value2']);
});
it('should match only second sub-schema because the first one does not match', async () => {
const schema: JSONSchema = {
anyOf: [
{
properties: {
prop: {
const: 'typeA',
},
propA: {},
},
},
{
properties: {
prop: {
const: 'typeB',
},
propB: {},
},
},
],
};
schemaProvider.addSchema(SCHEMA_ID, schema);
const content = 'prop: typeB\n|\n|';
const completion = await parseCaret(content);
expect(completion.items.map((i) => i.label)).to.be.deep.eq(['propB']);
});
it('should suggest from all sub-schemas even if nodes properties match better other schema', async () => {
// this is a case when we have a better match in the second schema but we should still suggest from the first one
// it works because `prop: ` will evaluate to `enumValueMatch = true` for both schemas
const schema: JSONSchema = {
anyOf: [
{
properties: {
prop: {
const: 'typeA',
},
},
},
{
properties: {
prop: {
const: 'typeB',
},
propB: {},
},
},
],
};
schemaProvider.addSchema(SCHEMA_ID, schema);
const content = 'prop: |\n|\npropB: B';
const completion = await parseCaret(content);
expect(completion.items.map((i) => i.label)).to.be.deep.eq(['typeA', 'typeB'], 'with null value');

const content2 = 'prop: typ|\n|\npropB: B';
const completion2 = await parseCaret(content2);
expect(completion2.items.map((i) => i.label)).to.be.deep.eq(['typeA', 'typeB'], 'with prefix value');
});

it('should suggest both sub-schemas for anyof array', async () => {
const schema: JSONSchema = {
properties: {
entities: {
type: 'array',
items: {
anyOf: [
{
enum: ['enum1'],
},
{
type: 'object',
title: 'entity object',
properties: {
entityProp: { type: 'string' },
},
required: ['entityProp'],
},
],
},
},
},
};
schemaProvider.addSchema(SCHEMA_ID, schema);
const content = 'entities:\n - |\n|';
const completion = await parseCaret(content);
expect(completion.items.map((i) => i.label)).to.be.deep.eq(['enum1', 'entityProp', 'entity object']);
});
});

describe('extra space after cursor', () => {
Expand Down