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: do not report duplicate keys errors for keys added by merge keys #25

Merged
merged 2 commits into from
Sep 17, 2019
Merged
Show file tree
Hide file tree
Changes from all 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
36 changes: 36 additions & 0 deletions src/__tests__/fixtures/merge-keys-with-duplicate-props.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
openapi: 3.0.0

x-format-version: "1.0"

info:
title: Merge key issue
description: https://yaml.org/type/merge.html
version: 1.0.0

x-center: &CENTER
x: 1
y: 2

x-left: &LEFT
x: 0
y: 2

x-big: &BIG
r: 10

x-small: &SMALL
r: 1

x-one:
<<: *CENTER
r: 10
label: center/big
x-two:
<<: [*CENTER, *BIG]
label: center/big
x-three:
<<: [*BIG, *LEFT, *SMALL]
x: 1
label: center/big

paths: {}
72 changes: 72 additions & 0 deletions src/__tests__/parseWithPointers.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,10 @@ import { HugeYAML } from './fixtures/huge-yaml';

const diverse = fs.readFileSync(path.join(__dirname, './fixtures/diverse.yaml'), 'utf-8');
const duplicateMergeKeys = fs.readFileSync(path.join(__dirname, './fixtures/duplicate-merge-keys.yaml'), 'utf-8');
const mergeKeysWithDuplicateProperties = fs.readFileSync(
path.join(__dirname, './fixtures/merge-keys-with-duplicate-props.yaml'),
'utf-8'
);
const spectral481 = fs.readFileSync(path.join(__dirname, './fixtures/spectral-481.yaml'), 'utf-8');

describe('yaml parser', () => {
Expand Down Expand Up @@ -428,6 +432,56 @@ european-cities: &cities
});
});

test('handles overrides #2', () => {
const result = parseWithPointers(mergeKeysWithDuplicateProperties, {
mergeKeys: true,
ignoreDuplicateKeys: false,
});

expect(result.data).toEqual({
openapi: '3.0.0',
'x-format-version': '1.0',
info: {
description: 'https://yaml.org/type/merge.html',
title: 'Merge key issue',
version: '1.0.0',
},
'x-center': {
x: 1,
y: 2,
},
'x-left': {
x: 0,
y: 2,
},
'x-big': {
r: 10,
},
'x-small': {
r: 1,
},
'x-one': {
x: 1,
y: 2,
r: 10,
label: 'center/big',
},
'x-two': {
x: 1,
y: 2,
r: 10,
label: 'center/big',
},
'x-three': {
x: 1,
y: 2,
r: 10,
label: 'center/big',
},
paths: {},
});
});

test('handles duplicate merge keys', () => {
const result = parseWithPointers(duplicateMergeKeys, { mergeKeys: true });

Expand All @@ -440,5 +494,23 @@ european-cities: &cities
t: 4,
});
});

test('does not report duplicate merge keys', () => {
const result = parseWithPointers(duplicateMergeKeys, {
mergeKeys: true,
ignoreDuplicateKeys: false,
});

expect(result.diagnostics).toEqual([]);
});

test('does not report duplicate errors for merged keys', () => {
const result = parseWithPointers(mergeKeysWithDuplicateProperties, {
mergeKeys: true,
ignoreDuplicateKeys: false,
});

expect(result.diagnostics).toEqual([]);

Choose a reason for hiding this comment

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

@P0lip In order to protect this from potential future regressions, isn't there a way to also assess the expected result of the merges?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll add such an assertion for sure, before I mark it as ready for review, no worries.

Just in case you were concerned, we already have a few tests covering merge keys :)

describe('merge keys', () => {

I just didn't cover diagnostics. :P

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nulltoken just in case you were interested, a test case has been added.
Thanks for providing an excellent fixture I could use right away. :)

Choose a reason for hiding this comment

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

@nulltoken just in case you were interested, a test case has been added.

image

});
});
});
27 changes: 18 additions & 9 deletions src/parseWithPointers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -73,20 +73,29 @@ export const walkAST = (
case Kind.MAP: {
const container = {};
// note, we don't handle null aka '~' keys on purpose
for (const mapping of (node as YamlMap).mappings) {
// typing is broken, value might be null
if (mapping.key.value in container) {
if (options !== void 0 && options.json === false) {
throw new Error('Duplicate YAML mapping key encountered');
}
const seenKeys: string[] = [];
const handleMergeKeys = options !== void 0 && options.mergeKeys === true;
const handleDuplicates = (options !== void 0 && options.json === false) || duplicatedMappingKeys !== void 0;

if (duplicatedMappingKeys !== void 0) {
duplicatedMappingKeys.push(mapping.key);
for (const mapping of (node as YamlMap).mappings) {
const key = mapping.key.value;

if (handleDuplicates && (!handleMergeKeys || key !== SpecialMappingKeys.MergeKey)) {
if (seenKeys.includes(mapping.key.value)) {
if (options !== void 0 && options.json === false) {
throw new Error('Duplicate YAML mapping key encountered');
}

if (duplicatedMappingKeys !== void 0) {
duplicatedMappingKeys.push(mapping.key);
}
} else {
seenKeys.push(key);
}
}

// https://yaml.org/type/merge.html merge keys, not a part of YAML spec
if (options !== void 0 && options.mergeKeys === true && mapping.key.value === SpecialMappingKeys.MergeKey) {
if (handleMergeKeys && key === SpecialMappingKeys.MergeKey) {
Object.assign(container, reduceMergeKeys(walkAST(mapping.value, options, duplicatedMappingKeys)));
} else {
container[mapping.key.value] = walkAST(mapping.value, options, duplicatedMappingKeys);
Expand Down