malczak / hashids

Hashids, ported for Swift (http://www.hashids.org)
MIT License
110 stars 35 forks source link

fix hash generation regression from Swift 3 conversion #11

Closed willk37 closed 7 years ago

willk37 commented 7 years ago

I was comparing the changes from the swift-2.3 branch to the master branch and it looks like order the alphabet was being used got reversed (https://github.com/malczak/hashids/blob/swift-2.3/Hashids.swift#L226). This PR fixes that and adds a test to check known hashes that include a hash length and alphabet. This should fix #10.

willk37 commented 7 years ago

After more testing I don't think this is the issue. Modifying the hash length and alphabet still produces different results from the Obj-C version

malczak commented 7 years ago

After more testing I would say they your pull request was a solution. This problem is also present in Swift2 version of the library - that is why I could not locate it by comparing 3 vs. 2. I have not found any examples showing that your changes are not fixing the problem.

I did run few tests against php and javascript versions.

Problem is related only with case then calculated hash is smaller than requested minimum hash length.

willk37 commented 7 years ago

When I was doing some testing I found all cases in this test failed on the branch I created but they pass in the Obj-C implementation.

    func testKnownHashesWithHashLengthAndAlphabet() {
        // known hashes where generated with js Hashids implementation
        // http://codepen.io/anon/pen/MbmpJP
        let knownHashes: [String:[Int]] = [
            "XR5LWHjuQDp3": [1, 2, 3],
            "DXZKmvF3o8nG": [123456, 123456789],
            "PD0gFopnXtj1": [10, 123456, 1]
        ]

        var equalCount = 0
        let hashids = Hashids(salt: "this is my salt", minHashLength: 12, alphabet: "ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890abcdefghijklmnopqrstuvwxyz")
        for expectedHash in knownHashes.keys {
            let values = knownHashes[expectedHash]
            if let hash = hashids.encode(values!) {
                if (hash == expectedHash) {
                    equalCount += 1
                }
            }
        }

        XCTAssertEqual(equalCount, knownHashes.count)
    }
malczak commented 7 years ago

@willk37 thats interesting. for example for [123456, 123456789] in your example expected hash is DXZKmvF3o8nG. I'm getting the same hash using JS code. Same hash I got from Ruby version. Using swift example hash for the same input is DXZKmvf3o8nG, with PHP version - same hash.

Note the difference Library Hash
JS/Ruby DXZKmvF3o8nG
Swift/Php DXZKmvf3o8nG

All test run for: Hashids(salt: "this is my salt", minHashLength: 12, alphabet: "ABCDEFGHIJKLMNOPQRSTUVWXYZ1234567890abcdefghijklmnopqrstuvwxyz")

willk37 commented 7 years ago

@malczak That is weird, especially since hashids.org states Please note that generated ids are case-sensitive by default ("Aaa" is not the same id as "aaa").. I'm not sure which library is correct and which is incorrect though.

malczak commented 7 years ago

@willk37 I have found it, in fact case sensitivity was just a coincidence 😄 After comparing with Ruby version I found out that the problem was related with how an array of possible separators is created.

This line is changing an order of letters in separators array.

    self.seps = intersection(self.alphabet, self.seps)

instead it should be replaced with

    self.seps = intersection(self.seps, self.alphabet)

In fact when you take a look at Ruby version, items from default array of separators are replaced 'in place' while iterating an array.

The same error is in the PHP version

$this->_seps = implode('', array_intersect($alphabet_array, $seps_array));

should be replaced with

$this->_seps = implode('', array_intersect($seps_array, $alphabet_array));

What I will do now, is I will merge this pull request and add my changes on top of it. I think we should also create a pull request on php version.

malczak commented 7 years ago

@willk37 second thought - because php version was 'the initial' one, I would say all ports should reproduce the original library results.

willk37 commented 7 years ago

@malczak Good catch. Yea I would agree all ports should mimic the php library if that was the original one.

malczak commented 7 years ago

@willk37 as for now I will merge your changes, and will not change anything else. I have already wrote an email about our findings to the hashids initial author.