Skip to content

Commit

Permalink
Fix a bug where ES sends a string and migrations expect a boolean (el…
Browse files Browse the repository at this point in the history
  • Loading branch information
chrisdavies authored and cqliu1 committed Sep 25, 2018
1 parent bb5ab22 commit becfb75
Show file tree
Hide file tree
Showing 2 changed files with 30 additions and 3 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,30 @@ describe('determineMigrationAction', () => {
doc: {
dynamic: 'strict',
properties: {
hello: { dynamic: true, goober: 'pea' },
hello: { dynamic: 'true', goober: 'pea' },
world: { baz: 'bing' },
},
},
};

expect(determineMigrationAction(actual, expected)).toEqual(MigrationAction.None);
});

test('requires no action if mappings differ only by equivalent coerced properties', () => {
const actual = {
doc: {
dynamic: 'strict',
properties: {
hello: { dynamic: 'false', baz: '2', foo: 'bar' },
world: { baz: 'bing' },
},
},
};
const expected = {
doc: {
dynamic: 'strict',
properties: {
hello: { dynamic: false, baz: 2, foo: 'bar' },
world: { baz: 'bing' },
},
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,11 @@ function diffSubProperty(actual: any, expected: any): MigrationAction {
// We have a leaf property, so we do a comparison. A change (e.g. 'text' -> 'keyword')
// should result in a migration.
if (typeof actual !== 'object') {
return _.isEqual(actual, expected) ? MigrationAction.None : MigrationAction.Migrate;
// We perform a string comparison here, because Elasticsearch coerces some primitives
// to string (such as dynamic: true and dynamic: 'true'), so we report a mapping
// equivalency if the string comparison checks out. This does mean that {} === '[object Object]'
// by this logic, but that is an edge case which should not occur in mapping definitions.
return `${actual}` === `${expected}` ? MigrationAction.None : MigrationAction.Migrate;
}

// Recursively compare the sub properties
Expand All @@ -87,5 +91,5 @@ function diffSubProperty(actual: any, expected: any): MigrationAction {
}

function isDynamic(prop: any) {
return prop.dynamic === true || prop.dynamic === 'true';
return prop && `${prop.dynamic}` === 'true';
}

0 comments on commit becfb75

Please sign in to comment.