canonical / sqlair

Friendly type mapping for SQL databases
Apache License 2.0
16 stars 8 forks source link

bugfix: Fix duplicate output error #129

Closed Aflynn50 closed 5 months ago

Aflynn50 commented 6 months ago

Fix error where a single map member can be used as an output multiple times in a query.

The outputUsed map mapped typeinfo.Output to booleans recording if the output had been used. For maps the mapKey object that implemented the typeinfo.Output has a pointer receiver. It was possible to have two identical mapKey structs with different pointers.

Here, the error is fixed by indexing outputUsed with a string representation of the typeinfo.Output using its String method.

letFunny commented 6 months ago

The change itself looks good but I think we need to investigate deeper. Do we know why there are two pointers in the first case? I remember discussing this but I cannot see it in the PR description.

letFunny commented 6 months ago

I see that the problem is that we are creating a pointer here. Why don't we remove the pointer receiver for mapInfo which solves the problem directly. I do not think copying the reflect.Type field is that bad because it is already an interface.

Aflynn50 commented 6 months ago

I see that the problem is that we are creating a pointer here. Why don't we remove the pointer receiver for mapInfo which solves the problem directly. I do not think copying the reflect.Type field is that bad because it is already an interface.

We could remove the pointer receiver for mapInfo but it would be a bit of a smell because structField would still be a pointer and we would have one thing implementing the interface with a pointer and the other without.

I've added a method IDString instead which is a identifier string for the mapKey/structField that can be used for outputUsed in bindTypes.