fix: implements a behaviour change to zero slice values

[tlm]

This PR changes the contract that GetAll has with the caller around how slices are populated for queries. Prior to this change GetAll would append all query values to the slice supplied resulting in existing slice elements being retained.

This poses a problem when failed transactions are retried because should a call have previously succeeded but the transaction fails for some other reason then the slice will get the same values again or at least the changed subset.

I am unsure if this behaviour was on purpose in the original implementation but it does fight the standard SQL package where values are always overwritten in a query.

If a previous caller to this func was relying on this behaviour then it makes sense that an AppendAll variant could also be provided.

Due to the change in behaviour of this commit it MUST be rolled out in a version bump of Sqlair. I have also added regression testing to the package to catch this going forward.

[tlm]

It may be slightly better to set the length of the slice at the end of the func when we assign back to it. That way we can say for any failure case the existing slice is unmodified. A better contract I feel.

[SimoneDutto]

Hello, sorry to jump in but i thought it was an interesting pr to review.

Wouldn't it be better to just return an error if the slice is not empty?

if v.Len() > 0 {
	fmt.Println("you shouldn't do this")
}

So people upgrading to a newer version of sqlair will immediately find the places where they were using (willingly or not) this behavior.

[gfouillet]

Hello, sorry to jump in but i thought it was an interesting pr to review.

Wouldn't it be better to just return an error if the slice is not empty?

if v.Len() > 0 {
	fmt.Println("you shouldn't do this")
}

So people upgrading to a newer version of sqlair will immediately find the places where they were using (willingly or not) this behavior.

The issue with this approach is that the error will be triggered in some case of transaction failure, so it would be quite undeterministic. Instead of having inconsistant result you will have inconstant failure, which is not better 😸

To me, resetting the slice length is the right approach, both in term of memory consumption and correctness.

[gfouillet]

Great spot, thanks.

I believe we should have two more tests cases:

• A case where the dbVals in the second query is longer than the expected result (dbVals = append(dbVals, Person{}) before second call
• A case where the dbVals in the second query is shorter than the expected result (dbVals = dbVals[1:]). For this one it would be worthy to also check the content of the retrieved values.

Both test case may help to find edge case in term of reusing memory slot (even if there is no bug in the implementation at the moment)

[manadart]

Thanks for this. There is already minor tweak that I landed, which is yet to be rolled out. We'll bump and get a new version in to the 4.0 branch.

[tlm]

Hello, sorry to jump in but i thought it was an interesting pr to review.

Wouldn't it be better to just return an error if the slice is not empty?

if v.Len() > 0 {
	fmt.Println("you shouldn't do this")
}

So people upgrading to a newer version of sqlair will immediately find the places where they were using (willingly or not) this behavior.

As per the PR description this change needs to occur in a major version bump because of a behaviour change. Your approach is interesting but it is also a behaviour change resulting in the same version bump. @gfouillet comments then come into play.

If there is legitimate use case for wanting the behaviour we can add an 1AppendAll1 version in a minor bump to the lib.

[tlm]

@manadart performed a drive by docs fix for CI. Just fixing a broken link for ErrTxDone to now point to stdlib as the errro was never defined in this package. See last commit in PR.