Add support for more types in gql - #464
Conversation
a760148 to
03c3f71
Compare
3d463b1 to
fb5387e
Compare
| jsonColumns.Add($"{parametrizedCLabel}, CAST({subqueryName}.{QuoteIdentifier(cLabel)} is true as json)"); | ||
| } | ||
| else if (column.ColumnName != SqlQueryStructure.DATA_IDENT && | ||
| structure.GetColumnSystemType(column.ColumnName) == typeof(byte[])) |
There was a problem hiding this comment.
nit: can we use is operator here?
There was a problem hiding this comment.
is requires a constant on the other end and typeof(byte[]) is not.
aaronpowell
left a comment
aaronpowell
left a comment
There was a problem hiding this comment.
Minor nit on one test, otherwsie looks good (and yay for wider type support!)
| { | ||
| if (!IsTypeSupportedType(type, value)) | ||
| { | ||
| return; |
There was a problem hiding this comment.
shouldn't this fail the test? Otherwise we won't know if we're running the test for types that aren't supported.
There was a problem hiding this comment.
Failing would cause the CI to fail and the PR wouldn't be merged.
I ended up settling for Assert.Inconclusive. It skips the test when executed.
| INSERT INTO brokers("ID Number", "First Name", "Last Name") VALUES (1, 'Michael', 'Burry'), (2, 'Jordan', 'Belfort'); | ||
| INSERT INTO stocks_price(categoryid, pieceid, instant, price, is_wholesale_price) VALUES (2, 1, 'instant1', 100.57, True), (1, 1, 'instant2', 42.75, False); | ||
| INSERT INTO type_table(id, int_types, string_types, float_types, boolean_types) VALUES (1, 1, '', 0.33, true), (2, -1, 'lksa;jdflasdf;alsdflksdfkldj', -9.2, false), (3, 123456, 'null', 1555.99, true), (4, NULL, NULL, NULL, NULL); | ||
| INSERT INTO type_table(id, short_types, int_types, long_types, string_types, single_types, float_types, decimal_types, boolean_types, datetime_types, bytearray_types) VALUES |
There was a problem hiding this comment.
Could you make sure we test min/max value for each type?
| short_types smallint, | ||
| int_types int, | ||
| long_types bigint, | ||
| string_types varchar(max), |
There was a problem hiding this comment.
wonder if we support nvarchar(max)?
There was a problem hiding this comment.
Yup I checked.
varchar and nvarchar are both detected as string types, and they are handled accordingly.
HotChocolate supports unicode as well it seems. So nvarchar is fully supported
| return false; | ||
| "ID", | ||
| "Byte", | ||
| "Short", |
There was a problem hiding this comment.
could we use something like SingleType.TypeName
There was a problem hiding this comment.
I added this instead of the "Single" string. Short corresponds to small int.
| new InputValueDefinitionNode(null, new NameNode("lt"), new StringValueNode("Less Than"), new ByteType().ToTypeNode(), null, new List<DirectiveNode>()), | ||
| new InputValueDefinitionNode(null, new NameNode("lte"), new StringValueNode("Less Than or Equal To"), new ByteType().ToTypeNode(), null, new List<DirectiveNode>()), | ||
| new InputValueDefinitionNode(null, new NameNode("neq"), new StringValueNode("Not Equals"), new ByteType().ToTypeNode(), null, new List<DirectiveNode>()), | ||
| new InputValueDefinitionNode(null, new NameNode("isNull"), new StringValueNode("Not null test"), new BooleanType().ToTypeNode(), null, new List<DirectiveNode>()) |
There was a problem hiding this comment.
isNull was used for other types as well, and our filter parsing expects isNull.
| protected abstract string MakeQueryOnTypeTable(List<string> columnsToQuery, int id); | ||
| protected virtual bool IsTypeSupportedType(string type, string value = null) | ||
| { | ||
| return true; |
There was a problem hiding this comment.
throw not implemented? so that we dont use it accidentally?
There was a problem hiding this comment.
I ended up using Assert.Inconclusive which skips the test. Throwing not implemented will fail the test which would block the PR from merging.
This is not technically just not implemented.
e.g. Postgres does not support byte type. That will never be implemented. The case is a bit different for varbinary for MsSql which we need to support at some point
| switch (floatType) | ||
| { | ||
| case SINGLE_TYPE: | ||
| Assert.AreEqual(float.Parse(expectedFloat), float.Parse(actualFloat)); |
There was a problem hiding this comment.
What is float? An instance, a class and is it a member variable? Could you correct its case so it can be easily identified from the name?
There was a problem hiding this comment.
The compiler complains about Float.parse. float is a variable type in this case, and float.Parse is consistent with our other conversions: int.Parse, long.Parse etc.
Aniruddh25
left a comment
Aniruddh25
left a comment
There was a problem hiding this comment.
Thanks for widening our db types support. This is a very nice enhancement.
LGTM after resolving suggestions and some questions!
…into graphql-add-more-types
4 participants
Done:
This is a bit of a tricky one.
DateTimetype is handled by HotChocolate asDateTimeOffset. Also, our db column type detection method givesDateTimefor allDate,DateTime, andDateTimeOffset. Meaning that while the user can define nonDateTimeOffsetcolumns, they will be interpreted asDateTimeOffsetand the gaps will be filled with 00:00:00 time and local offset.HotChocolate also provides us with
Datetype. However, because all db column typesDate,DateTime, andDateTimeOffsetare detected asDateTimesystem types, we cannot make use of the HotChocolateDatetype as of now.A base64 representation of the byte array is expected to be input to the queries and similarly the base64 representation of the byte array will be returned.
MsSql does not support inserting/updating a bytearray (varbinary) to null due to: https://stackoverflow.com/questions/29254690/why-does-dbnull-value-require-a-proper-sqldbtype
Essentially, when passing parameters to the db, the parameters are automatically cast to string. But varbinary does not accept a string even though all other column types have not complained about this across all three sql db-s 😅 . This will likely require us to change the way we construct the parameter dictionary to include the db column type. This change is relatively large and out of the scope of this PR.
Opened an issue for this: Sql: Specify parameter type when passing parameters to the sql conn #475
Todos (might consider doing in a separate PR-s):
The changes required to add a new type supported by HotChocolate:
GraphQLTypes.DefaultValueType.Configure: specify the type of the value the default value directive takes for each type.GraphQLUtils.IsBuiltInType: add the type to built in types so it is not parsed as if it is a model type (aka matched a table in the database).StandardQueryInputs: create a filter input type for the new type and add that input type toStandardQueryInputs.InputTypes.EdmModelBuilder.BuildEntityTypes: add the type to the switch statement for type resolution.SchemaConverter.FromTableDefinition: create the appropriate ObjectValueNode when the column with the type introduced has a default value.SchemaConverter.GetGraphQLTypeForColumnType: add the new type to the switch statement.BaseSqlQueryStructure.GetParamAsColumnSystemType: add the type to the switch statement.If the type is not support supported by HotChocolate (e.g. Single):
SingleTypedefined inGraphQLBuilder.CustomTypes)