georgysavva / scany

Library for scanning data from a database into Go structs and more
MIT License
1.31k stars 68 forks source link

Don't reset the destination slice in ScanAll method #101

Closed CyganFx closed 1 year ago

CyganFx commented 1 year ago

Hi, I found out that ScanAll resets the destination slice image

Is there any way to not empty the slice and keep slice elements' pointers, or can you explain why exactly it's required? Maybe you could add a new function/modify existing one that will solve this problem.

Here's a simplified example of a real case that can occur - when some data references the slice elements:

image image

image

Code: https://go.dev/play/p/smM5FLMDrHA

georgysavva commented 1 year ago

Hi! I am not sure what you are trying to demonstrate with this example. Maybe you can tell the real-world problem you've encountered. But I will try to break down and comment on your code the best I can.

In your example, the require.NotNil() assertion doesn't work not because of scany. It behaves that way because when you copy slice elements values (pointers) into the tmp variable, they are nil. Later, when the insertMultipleRows() function changes the slice elements of the testRows variable, it doesn't affect the tmp variable because they aren't connected in any way.

Nonetheless, In your example, scany indeed changes the addresses of the slice elements. It happens because the destination slice is not empty, and the library resets it to zero length, completely ignoring all existing values. Hence, it needs to create new instances of the testRow struct for each scanned database row.

The decision to reset the slice length ensures that the slice length always equals the number of rows scanned. It makes it easier to reason about the operation. Besides, another popular database scanning library sqlx uses the same approach (reset the destination slice length to zero).

Let me know if you have any other comments or questions.

CyganFx commented 1 year ago

My bad, I initialized it in the wrong place. I've updated the example case, I hope now it will become more clear what I mean

georgysavva commented 1 year ago

Thanks for updating the example. Now it's more coherent.

Yet I still don't quite understand what behavior would be desirable for you in that case. Would you expect scany to append all new items to the slice destination and keep those elements already in the slice?

If yes, then I need to disagree. I think it's less intuitive and can cause problems if you want to implement retries, for example. Besides, as I pointed out before, another popular database scanning library sqlx behaves exactly the same way: it resets the slice destination. So I don't see any problem here.

Let me know if I missed something.

georgysavva commented 1 year ago

Closing due to inactivity. Feel free to reopen.