Adds support for decoding bytes in metadata calls

[bbeaudreault]

Even though we're passing strings into the ResultSets, the implementation of Row.getObject turns many of them into bytes because their type is CHAR or VARCHAR (here). Upstream libraries (like liquibase) expect result.getObject to return Strings for things like TABLE_CAT, etc and are throwing exceptions when trying to cast the Object to a String.

This very simple change passes through the Connection into the ResultSet objects created by Metadata requests. This way when we call getObject, the recently added charset decoding code can detect the byte[] and convert them to strings where appropriate. This also makes the constructors more consistent, in my opinion.

This is a bugfix which fixes the following exception for users of Liquibase:

java.lang.ClassCastException: [B cannot be cast to java.lang.String

[bbeaudreault]

I added a test case for this, which fails against master but is fixed here

[bbeaudreault]

cc @ashudeep-sharma

[ashudeep-sharma-zz]

To accomodate the changes, we are passing connection variable to tthe VitessResultSet. I think its about exposing too much information because one can really execute any query using live connection. Here the information that you mainly require is ConnectionProperties. It would be good if we only expose that while passing to the constructor of ResultSet unless you have any other reason for exposing the Connection object.

[bbeaudreault]

@ashudeep-sharma I just pushed a change -- I'm stilling passing in the same object, but VitessResultSet and FieldWithMetadata see it as just a ConnectionProperties not a full VitessConnection. I changed all references to expect a ConnectionProperties and renamed the variable names to match. This should limit the scope enough to be safe.

If someone really wanted too they could still dig around in the code and figure out that they can just cast the ConnectionProperties to a Connection and do what they want. But if we're concerned about that then there is a lot else someone could do with some simple reflection code and there isn't a huge harm. So I think this is fine.

[bbeaudreault]

@ashudeep-sharma any other thoughts, or does it look good to merge?

[ashudeep-sharma-zz]

@bbeaudreault : Stucked with some internal issues, will definitely look at it tomorrow.

[bbeaudreault]

Thanks!

[harshit-gangal]

Looks good to me.
Will wait for @ashudeep-sharma to reply.

[ashudeep-sharma-zz]

Changes looks good in general and I believe you have run the code-coverage as well. I have just one request based on the code changes that have been done recently. I think we are adding too many comments in between the code. Some of the comments are implicit and the logic can be understood based on the calls being made. Should we spend some efforts in cleaning this up.
Also like in tests i saw somewhere in the middle, some comments have been added. Can we ensure that all the comments for the tests being mentioned in the beginning of function and not being in the middle.

Let me know if you have any thoughts/suggestions?

---

Reviewed 4 of 5 files at r1, 6 of 6 files at r2.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[ashudeep-sharma-zz]

---

Review status: all files reviewed at latest revision, all discussions resolved, some commit checks pending.

---

Comments from Reviewable

[bbeaudreault]

Thanks for the review and comments @ashudeep-sharma -- in terms of comments in code: I'd be happy to trim it down a bit in the future. Comments at the top of tests make sense too. However, I think exceptions should be made in other cases. So maybe on a case-by-case basis

[ashudeep-sharma-zz]

yeah i agree on that, if the code is too complicated we should add a comment just above the line else it should be trimmed. Rest looks good to me.

[bbeaudreault]

@sougou @alainjobart @michael-berlin This got a LGTM. Could one of you merge when you get a chance? Looking to submit a dependent PR today. Thanks!