pashagolub / pgxmock

pgx mock driver for golang to test database interactions
Other
386 stars 49 forks source link

improve `RawValues()` implementation #138

Closed sejin-P closed 1 year ago

sejin-P commented 1 year ago

As I raised issue #128, @Eitol resolved it wisely by converting all of types into byte slice by making string value of that value.(#137) However, it could cause a potential vulnerability if pgx call RawValues() to decode column values. As of now, this implementation doesn't cause any vulnerability because pgx doesn't call RawValue to decode column values. But just in case, I wanted to prevent this vulnerability.

The implementation is simple. I applied json Marshal to encode values. We can decode all of primitive types in golang, even time.Time type can be encoded. Therefore, we don't have to concern about if there would be a type can't be converted in pgx. Pgx support all of primitive types, slices of them, and time.Time type. 

I considered about just decoding into binary by using encoding/binary package, but types like time.Time is an obstacle. I don't want to consider types in pgxmock's RawValue method. Json Marshalling was the simplest way to acheive my goal.

p.s I fixed test code to use pgx.RowToAddrOfStructByPos because the method which caused panic was pgx.RowToAddrOfStructByPos, not pgx.RowToStructByPos

pashagolub commented 1 year ago

Would you, please, elaborate why are you using json.Marshall() out of the blue? I cannot get it.

sejin-P commented 1 year ago

@pashagolub Yes, sure. I apologize for confusion.

The way @Eitol resolved to avoid panic was good until we are not using RawValues to fetch real value. If we need to fetch real value by using RawValues, RawValues should provide proper encoded byte slice values.

For example, assume there is a int value(=5) in a row's column. In the previous way, it would be byte value of string "5", not int 5. Therefore, I considered lots of ways to decode all of the primitive types and time.Time into byte slice value. In result, json.Marshal is the simplest and public way to achieve our goal. We can encode each values just like what pgx library does(refer to https://github.com/jackc/pgx/blob/master/pgtype/pgtype.go), but it's not efficient because it's mocking pgx, not implementing pgx.

sejin-P commented 1 year ago

@pashagolub if you need more details, I will do it.

pashagolub commented 1 year ago

Thanks. My question is should we drop RawValues() maybe since we're not using it anyways?

I'll take a look later. We have bank holiday here on Monday.

Thanks for your help!

sejin-P commented 1 year ago

@pashagolub Oh, we still need it because we should implement pgx.Rows interface. I think proper and efficient implementation of this method is still demanded.

Anyway, take a rest for your holidays. Thank you!

pashagolub commented 1 year ago

Thanks