Skip to content

added Object key sort prior to JSON.stringify comparison (fix #5908) #5993

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

Closed
wants to merge 3 commits into from
Closed
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
19 changes: 18 additions & 1 deletion src/shared/util.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,23 @@ export function isTrue (v: any): boolean %checks {
export function isFalse (v: any): boolean %checks {
return v === false
}

/**
* Sorts an object by key to ensure seraialized equality
* by succinct key order. Guaranteed to at least return an
* empty object.
*/
export function keySort (obj: any): Object {
Copy link
Member

@HerringtonDarkholme HerringtonDarkholme Jun 29, 2017

Choose a reason for hiding this comment

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

This function is only used in this file, consider not exporting it. Also, obj is annotated as any which in this case, Object is a better choice, if you perform no recursive call.

if (isObject(obj) === false) {
return {}

Choose a reason for hiding this comment

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

If obj isn't object, this function always return an empty object which makes such call like looseEqual(1, 2) returning true.

}
const sorted = {}
Object.keys(obj).sort().forEach(key => {
sorted[key] = obj[key]

Choose a reason for hiding this comment

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

This only sorts the first level of an object. For nested object like {nested: {a: 1, b: 2}} and {nested: {b:2, a:1}}, it fails.

A recursive call should pay more attention to cyclic references.

})
return sorted
}

/**
* Check if value is primitive
*/
Expand Down Expand Up @@ -240,7 +257,7 @@ export function looseEqual (a: mixed, b: mixed): boolean {
const isObjectB = isObject(b)
if (isObjectA && isObjectB) {
try {
return JSON.stringify(a) === JSON.stringify(b)
return JSON.stringify(keySort(a)) === JSON.stringify(keySort(b))
} catch (e) {
// possible circular reference
return a === b
Expand Down
55 changes: 55 additions & 0 deletions test/unit/modules/shared/key-sort.spec.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
import { keySort, looseEqual } from 'shared/util'

describe('keySort', () => {
const chars = '0123456789abcdefghijklmnopqrstuvwxyz!@#$%^&*()_+./'
const randomWord = () => {
const letters = []
for (let cnt = 0; cnt < 10; cnt++) {
letters.push(chars.charAt(Math.floor(Math.random() * chars.length)))
}
return letters.join('')
}
const unsortedObject = () => {
const unsorted = {}
chars.split('').map(c => {
unsorted[c + '' + randomWord()] = randomWord()
})
return unsorted
}

it('returns Object for empty Object argument', () => {
expect(looseEqual(keySort({}), {})).toBe(true)
})

it('returns Object for Array argument', () => {
expect(looseEqual(keySort([]), {})).toBe(true)

Choose a reason for hiding this comment

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

This changes looseEqual's original behavior.

})

it('returns Object for String argument', () => {
expect(looseEqual(keySort('Test String'), {})).toBe(true)
})

it('returns Object for Number argument', () => {
expect(looseEqual(keySort(Number.MAX_SAFE_INTEGER), {})).toBe(true)
})

it('returns Object for Undefined argument', () => {
expect(looseEqual(keySort(undefined), {})).toBe(true)
})

it('returns Object for Null argument', () => {
expect(looseEqual(keySort(null), {})).toBe(true)
})

it('Sorts an unsorted Object, which are equal', () => {
const unsortedTestObj = unsortedObject()
const sortedObject = keySort(unsortedTestObj)
expect(looseEqual(sortedObject, unsortedTestObj)).toBe(true)
})

it('does not unsort a sorted object', () => {
const sortedObject = keySort(unsortedObject())
const sortedObjectCompare = keySort(sortedObject)
expect(looseEqual(sortedObject, sortedObjectCompare)).toBe(true)
})
})