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.

Sign up for GitHub

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

Jump to bottom

fixed broken serialization of duplicated entries in sequences #205

Merged
merged 1 commit into from Sep 9, 2015
Merged

fixed broken serialization of duplicated entries in sequences #205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/js-yaml.js
View file
Open in desktop
Expand Up @@ -27,7 +27,7 @@ module.exports.dump = dumper.dump;
module.exports.safeDump = dumper.safeDump;
module.exports.YAMLException = require('./js-yaml/exception');

// Deprecared schema names from JS-YAML 2.0.x
// Deprecated schema names from JS-YAML 2.0.x
module.exports.MINIMAL_SCHEMA = require('./js-yaml/schema/failsafe');
module.exports.SAFE_SCHEMA = require('./js-yaml/schema/default_safe');
module.exports.DEFAULT_SCHEMA = require('./js-yaml/schema/default_full');
Expand Down
12 changes: 6 additions & 6 deletions lib/js-yaml/dumper.js
View file
Open in desktop
Expand Up @@ -715,10 +715,6 @@ function writeNode(state, level, object, block, compact, iskey) {
block = (0 > state.flowLevel || state.flowLevel > level);
}

if ((null !== state.tag && '?' !== state.tag) || (2 !== state.indent && level > 0)) {
compact = false;
}

var objectOrArray = '[object Object]' === type || '[object Array]' === type,
duplicateIndex,
duplicate;
Expand All @@ -728,6 +724,10 @@ function writeNode(state, level, object, block, compact, iskey) {
duplicate = duplicateIndex !== -1;
}

if ((null !== state.tag && '?' !== state.tag) || duplicate || (2 !== state.indent && level > 0)) {
compact = false;
}

if (duplicate && state.usedDuplicates[duplicateIndex]) {
state.dump = '*ref_' + duplicateIndex;
} else {
Expand All @@ -738,7 +738,7 @@ function writeNode(state, level, object, block, compact, iskey) {
if (block && (0 !== Object.keys(state.dump).length)) {
writeBlockMapping(state, level, state.dump, compact);
if (duplicate) {
state.dump = '&ref_' + duplicateIndex + (0 === level ? '\n' : '') + state.dump;
state.dump = '&ref_' + duplicateIndex + state.dump;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this removed completely? Why? AFAIK this was intended for cases like the following:

a: &ref_1
  foo: bar
  answer: 42
b: *ref_1

Copy link
Collaborator

Choose a reason for hiding this comment

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

But probably I'm mistaken.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, I remembered. It's for anchors on top-level nodes. So the question stands: Why is this removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I removed it completely. I will check if your example still works correctly. In the meantime my reasoning to remove this condition:

The problem with the incorrectly prepended YAML alias names only occurred if compact was true. writeNode is called with compact = true from line 536, line 625 and line 830.

(0 === level ? '\n' : '') is responsible for prepending the alias for top level objects, i.e. for calls from line 830. For example, this condition ensures that we get

&ref_0
- *ref_0

instead of

&ref_0- *ref_0

The two other positions which called writeNode with compact = true were not handled correctly by this condition.

Since I set compact to false when an alias is prepended, I think that this condition is not necessary anymore. Without the removal of (0 === level ? '\n' : '') we would now actually get

&ref_0

- *ref_0

(note the extra empty line)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested your example.

var obj = { foo: 'bar', answer: 42 };
var obj2 = { a: obj, b: obj };
console.log(yaml.dump(obj2));

is still correctly serialized as

a: &ref_0
  foo: bar
  answer: 42
b: *ref_0

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, OK. Thanks for explanation.

}
} else {
writeFlowMapping(state, level, state.dump);
Expand All @@ -750,7 +750,7 @@ function writeNode(state, level, object, block, compact, iskey) {
if (block && (0 !== state.dump.length)) {
writeBlockSequence(state, level, state.dump, compact);
if (duplicate) {
state.dump = '&ref_' + duplicateIndex + (0 === level ? '\n' : '') + state.dump;
state.dump = '&ref_' + duplicateIndex + state.dump;
}
} else {
writeFlowSequence(state, level, state.dump);
Expand Down
27 changes: 27 additions & 0 deletions test/issues/0205.js
View file
Open in desktop
@@ -0,0 +1,27 @@
'use strict';


var assert = require('assert');
var yaml = require('../../');


test('Duplicated objects within array', function () {
var obj = { test: 'canary' };
var arrayWithRefs = [ obj, obj ];

var obtained = yaml.load(yaml.dump(arrayWithRefs));

assert.equal(obtained[0].test, 'canary');
assert.equal(obtained[0], obtained[1]);
});

test('Duplicated arrays within array', function () {
var array = [ 0, 1 ];
var arrayWithRefs = [ array, array ];

var obtained = yaml.load(yaml.dump(arrayWithRefs));

assert.equal(obtained[0][0], 0);
assert.equal(obtained[0][1], 1);
assert.equal(obtained[0], obtained[1]);
});