-
Notifications
You must be signed in to change notification settings - Fork 112
feat(spanner): support for type UUID #2235
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
69cf752 to
5738698
Compare
| } | ||
|
|
||
| if (uuid.validate(value)) { | ||
| return {type: 'unspecified'}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why unspecified here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cause we are not sure if the customer has inserted the UUID value in the column of type UUID/String. Hence, we are returning type as unspecified implicitly, but while making gRPC call we are making sure that if the type is unspecified we don't pass param type with the gaxOptions, this way are letting backend to figure out the type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sakthivelmanii uuid value can be send for String column as well. So we want to make use of "unspecified" type and let backend decide it.
Ideally we should not have removed the type mapping when backend started suplorting untyped params, but we did not remove this because of breaking changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But will this work for parameterized queries. I think parameterized queries does not support UNSPECIFIED type.
@alkatrivedi can you check if parameterized query is working when passing a UUID value?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The case that we are talking about here is when customer triggers a following parameterized query without an explicit param type
const query = {
sql: 'SELECT @v',
params: {
v: values,
},
};
I think this fails if value is of UUID type and param type is not mentioned because client now sends a UNSPECIFIED type to backend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion here is
Ideally, if param type is not mentioned then treat the value as string.
If customer wants to treat that value as UUID then they can explicitly specify the param type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
based on the discussion with @harshachinta and @surbhigarg92
we will return the type as unspecified, allowing the backend to determine the type of the value provided
regarding the query - 'SELECT @v'
failure in this case, where the parameter type is not inferred, is expected behavior. since the query does not target a specific table, deriving type dynamically is not a valid scenario
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flash forward one year later, when this has been merged: it breaks usage of implicitly-string parameters containing a UUID, at least with the emulator.
They are detected as UUID by this line of code, and this results in a type-less struct type being passed to the backend, which it doesn't like. For example, the parameter user contains a UUID (the same query worked fine before UUID "support" was introduced):
Error: 3 INVALID_ARGUMENT: Type code must be specified, found ;
When parsing field #0 of code: STRUCT
struct_type {
fields {
name: \"user\"
type {
}
}
}
My suggestion here is
Ideally, if param type is not mentioned then treat the value as string.
If customer wants to treat that value as UUID then they can explicitly specify the param type.
This seemed like a much more reasonable way of introducing a new feature and preserving backward compatibility.
I'll open an issue when I have time, but I'd highly appreciate proactive effort from the team to fix this, as the issue had been identified during the review process and was ignored.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flovouin Thanks for flagging this! I will take a look
| } | ||
|
|
||
| if (uuid.validate(value)) { | ||
| return {type: 'unspecified'}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sakthivelmanii uuid value can be send for String column as well. So we want to make use of "unspecified" type and let backend decide it.
Ideally we should not have removed the type mapping when backend started suplorting untyped params, but we did not remove this because of breaking changes.
10f3bc6 to
b686046
Compare
b686046 to
944efcd
Compare
|
closing this PR as I have raised a new PR |
This PR contains support for UUID type on Cloud Spanner.