[SNOW-1669514] Honor the Valuer/Stringer methods to resolve #1209

[ChronosMasterOfAllTime]

Description

SNOW-1669514 - Solve the issue with UUID getting double quoted due to new array function #1209

This adds the ability to check the valuer/stringer interfaces exists

Checklist

• Created tests which fail without the change (if possible)
• Extended the README / documentation, if necessary

Proof test fails.

export SKIP_SETUP=true && go test -run ^TestValueToString$ github.com/snowflakedb/gosnowflake
--- FAIL: TestValueToString (0.00s)
    --- FAIL: TestValueToString/UUID_-_should_return_string (0.00s)
        assert_test.go:103: expected "json" to be empty, but was not. . Thrown from gosnowflake/converter_test.go:290 +0xd4
            testing.tRunner(0xc0000bf520, 0x100e3a478)
        assert_test.go:103: expected ""1414b0f8-e56a-4e14-995b-774d87ea9d73"" to be equal to "1414b0f8-e56a-4e14-995b-774d87ea9d73" but was not. . Thrown from gosnowflake/converter_test.go:291 +0x197
            testing.tRunner(0xc0000bf520, 0x100e3a478)

[ChronosMasterOfAllTime]

@sfc-gh-dszmolka can you please explain how the format string is being used by the execBindQuery struct, or what is the purpose of this field?

[sfc-gh-pfus]

Great question! This field is extremely important, when dealing with structured objects/arrays/maps. If a field is structured, we need to inform snowflake backend, that it should parse this value and treat it as structured type instead of plain string.

[sfc-gh-pfus]

Thank you for your contribution! Can you take a look at my comments?

[sfc-gh-pfus]

Great question! This field is extremely important, when dealing with structured objects/arrays/maps. If a field is structured, we need to inform snowflake backend, that it should parse this value and treat it as structured type instead of plain string.

[sfc-gh-pfus]

I don't like such special cases. It may fail depending on the type defined in the table (varchar vs array vs array(byte) vs binary). Can you add such integration tests?

[ChronosMasterOfAllTime]

@sfc-gh-pfus What's the best approach for adding integration tests? I cannot run those locally, unless your team has a method for running them

[ChronosMasterOfAllTime]

This is testing for a very specific type of [16]byte and it checks if the interface satisfies the fmt.Stringer interface. Basically it's a corner case of is it a byte array of len 16 and does it have a String method available. Does that make sense?

[sfc-gh-pfus]

I believe that you have to use your snowflake account to run integration test, or at least use a trial. I believe that if you want to add an integration test, you would have to write a test, which creates a table, performs an insert and performs a select.

Yes, this is something I'm afraid of. Let's imagine you have a UUID (which technically is an array of bytes) and you want to insert it into varchar table. With your solution you would use it like this (guessing without integration test):

myUuid := NewUUID()
db.Exec("INSERT ... VALUES (?)", myUuid)

Am I right? In that case, the actual value goes to backend as a string representation (eg 646313f9-d353-41bc-8ef4-21482bab7020) and it is properly inserted into varchar column. So far so good.

But now, let's suppose, the column type is not varchar, but ARRAY or BINARY. From the driver perspective, you still send use the same construct:

myUuid := NewUUID()
db.Exec("INSERT ... VALUES (?)", myUuid)

But if we send it as the same value (646313f9-d353-41bc-8ef4-21482bab7020) backend won't be able to interpret it as array (expects [16,84,21,47,77...]) nor binary (expects 646313f9d35341bc8ef421482bab7020).

But I may be wrong and maybe backend will be able to somehow magically parse each option, this is why I suggested adding integration test for all cases :)

Generally UUID is not something that is supported by this driver (if it worked before it was only by accident) and if we want to add support for it, we should do it globally, not only in this case. Also, we have to think globally - how other drivers should support similar cases.

One more note - 16 elem byte array is not reserved for uuids, it can be also some checksum, something connected to cryptography (keys, IVs etc) and so on.

To solve your particular case - I believe, that if you have a stringer, you can replace db.Exec("INSERT ...", u) with db.Exec("INSERT ..., u.String()), right? Your option is nicer and I really would like it, if only it wasn't so troublesome :(

[ChronosMasterOfAllTime]

UUIDs on other drivers (SQLite, Postgres, etc.) is already supported.

Right; this issue more is intrinsic to using an ORM on top of the snowflake driver. Because we cannot control the ORM's behavior how it's going to do the final insert statement, we rely on the driver to do the correct thing. I can remove the UUID check in such a way OR I can add additional logic checks that looks for specific methods being available such as Parse,ParseUUID etc.

[ChronosMasterOfAllTime]

I will work on a more complete solution

This behavior was introduced in version 1.11.0 by #1154

Prior to that we never ran into this issue. Most people would write UUIDs as strings to their DB I would think.

We are using GORM in tandem with https://github.com/gorm-snowflake/gorm-snowflake. This is a repository we manage.

[ChronosMasterOfAllTime]

@sfc-gh-pfus when you say read/write integration tests, do you mean the structured types tests for read/write or something else?

I also made it so we're abso-positively sure that it's a UUID implementer of RFC 4122. Are you good with this?

[sfc-gh-pfus]

I mean firstly tests in which you create a table, insert uuid into varchar column and then read it into uuid value in client code again. driver_test.go seems to be a good place for this. Secondly - yes, we should extend structured types tests. This is slightly optional, but if it is possible to add it, it would be great.

Checking your implementation of RFC - it is a great idea! This way we can have only varchar-supported :) So summing things up - your writing part seem mostly fine so far, now let's ensure that reading such UUIDs also work. Good job!

[ChronosMasterOfAllTime]

Are we OK if I test using this?

https://snowflake.localstack.cloud/introduction/

My team prefers I dont use our real Snowflake database 😄

[sfc-gh-pfus]

That is a very interesting indeed, I didn't know that! Answering your question directly - at the end of the day we have to have proper integration tests that run on Github Actions and use real Snowflake instance. You can configure what instance you use using some environment variables (see init in driver_test.go). For you local testing you can define this localstack instance, but our tests will be eventually run on a real Snowflake.