replace copy() with a "deeper" copy

[berend]

replace copy() with "deeper" version of copying [][]string.

Caveats:

• no tests yet
• no much research yet, if there is a more elegant way

fixes #136

[berend]

I will add example schemas, and their outcome. However I don't know yet, how to put this in to a test

[berend]

here is a playground link for showing the problem with copy() on [][]string

[prokaktus]

Looks quite similar to problem that I've tried to address in PR https://github.com/nautilus/gateway/pull/132

[AlecAivazis]

Great i'm glad you found this @prokaktus - i was going to tag you to see if you thought they were related. The issue with copy should affect what you were working on right? If so, we probably want to update this PR to tweak your recent contribution

[prokaktus]

@AlecAivazis yeah, I think it is fixed now. Instead of copying nested slices for oldBranch, insertionPoint got copied before passing it to function, so any mutation made to oldBranch should not affect executeStep caller.

https://github.com/nautilus/gateway/blob/39d7af9721bf020864e3197f18b6598a8d38de5b/execute.go#L262-L264

[berend]

Awesome, I will check if #132 fixes my issue.

[AlecAivazis]

@berend can this be closed?

[AlecAivazis]

I'm going to close this as its gone a bit stale

[dibadgo]

Hey @AlecAivazis It looks like this issue is still relevant. After applying the fix that @berend suggested, the problem was fixed. We came across an issue where the gateway misses the insertion points. The problem is that points are being overwritten due to incorrect copying of variables. After deep copying, the problem was fixed right there!

It seems the status of the issue needs to be changed, I will try to write unit tests, after a while.

[AlecAivazis]

Thank you for bringing this up @dibadgo. I've been having a hard time finding the time to setup an example to reproduce the issue in the context of a known query against services but I sounds like you have one. I do suspect there is a "more elegant" way (as @berend put it) but until someone has the time to find it, I think its safe to merge this in to fix the bug.

Thank you for being so patient everyone!

[AlecAivazis]

This was just released under v0.1.16 🚀

[dibadgo]

Thank you @AlecAivazis, our team really need this fix! Fortunately, yesterday I spend some time on reproducing the problem, write a short explanation and even wrote unit tests. But didn't manage to push it.

Well below I've reproduced the issue:

        var accumulator [][]string

    oldBranch := make([][]string, 1)
    oldBranch[0] = []string{"point1", "point2"} // len 2, cap 2

    // append will double the capacity from 2 to 4, but the length will be 3
    oldBranch[0] = append(oldBranch[0], "point3") // len 3, cap 4

    for _, entryPoint := range []string{"point4", "point5"} {
        newBranchSet := make([][]string, len(oldBranch))

        // copy isn't about a multidimensional slice copy
        // I suspect it will be a copy only on 1st level
        copy(newBranchSet, oldBranch)

        for i, newBranch := range newBranchSet{
            // There is culmination of the script
            // newBranch - len 3, cap 4
            // on 3rd position newBranch can be empty or contains the previous value
            // but the length is still 3 and the append function will NOT allocate new memory
            // This leads to the fact the data is overwritten.
            newBranchSet[i] = append(newBranch, entryPoint) // len 3 < cap 4, then no new allocation, fill out 3rd position
        }

        for _, point := range newBranchSet {
            accumulator = append(accumulator, point)
        }
    }

    for _, v := range accumulator {
        fmt.Println(v)
    }
    /* Output:
    [point1 point2 point3 point5]
    [point1 point2 point3 point5]
    */
        // But expected point4 and point5 in the last positions of the slices

This means that the problem appears when the size of the slices (multidimensionality) is deeper or equal to 4.

There are no tests covers this case. Yesterday I've created new one. The test looks like TestFindInsertionPoint_rootList, but with a nesting level of 4. The problem was immediately reproduced. I applied the fix and the problem disappeared.

@AlecAivazis I'm not sure if it is necessary to add a new test exactly for this case. Maybe it was enough to correct the old one. But just in case, I will publish the new test in the new branch, if necessary.

[AlecAivazis]

@dibadgo - thanks for writing the explanation up. I think if this is a possible side-effect of an incorrect implementation it would be good to cover it with a test for future refactors. Worse case scenario the test doesn't really do much, best case it helps us out later.

I'll keep an eye out for the PR 👍