-
Notifications
You must be signed in to change notification settings - Fork 353
Add support for DateTime filters in PostgreSQL #3728
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
base: main
Are you sure you want to change the base?
Changes from all commits
ff7be90
d7e7c4e
de8b8bd
43e8d82
b0e0d70
a654dd5
877e3c2
c4f7fc4
375d496
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,6 +85,32 @@ public BaseSqlQueryStructure( | |
| } | ||
| } | ||
|
|
||
| /// <inheritdoc /> | ||
| public override string MakeDbConnectionParam(object? value, string? paramName = null, bool lengthOverride = false) | ||
| { | ||
| if (MetadataProvider.GetDatabaseType() is DatabaseType.PostgreSQL && | ||
| !string.IsNullOrEmpty(paramName) && | ||
| value is string stringValue && | ||
|
Comment on lines
+89
to
+93
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for non-string values, do we expect column system type to match the value type? If we cant be certain, shouldn't we do GetParamAsSystemType for kinds of values? |
||
| GetUnderlyingSourceDefinition().Columns.ContainsKey(paramName)) | ||
| { | ||
| Type columnSystemType = GetColumnSystemType(paramName); | ||
| if (columnSystemType != typeof(string)) | ||
| { | ||
| value = GetParamAsSystemType(stringValue, paramName, columnSystemType); | ||
| } | ||
|
Comment on lines
+94
to
+100
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this code seems redundant. can it be optimized? for e.g., |
||
|
|
||
| // Npgsql requires DateTime with Kind=Unspecified for 'timestamp without time zone' columns. | ||
| // ParseParamAsSystemType returns Kind=Utc (via .UtcDateTime), which causes PostgreSQL to | ||
| // apply a UTC-to-local offset during comparison, producing incorrect filter results. | ||
| if (value is DateTime dtValue && dtValue.Kind == DateTimeKind.Utc && columnSystemType == typeof(DateTime)) | ||
| { | ||
| value = DateTime.SpecifyKind(dtValue, DateTimeKind.Unspecified); | ||
| } | ||
| } | ||
|
|
||
| return base.MakeDbConnectionParam(value, paramName, lengthOverride); | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// For UPDATE (OVERWRITE) operation | ||
| /// Adds result of (SourceDefinition.Columns minus MutationFields) to UpdateOperations with null values | ||
|
|
@@ -422,9 +448,9 @@ protected List<LabelledColumn> GenerateOutputColumns() | |
| /// Tries to parse the string parameter to the given system type | ||
| /// Useful for inferring parameter types for columns or procedure parameters | ||
| /// </summary> | ||
| /// <param name="param"></param> | ||
| /// <param name="systemType"></param> | ||
| /// <returns></returns> | ||
| /// <param name="param">The string value to parse.</param> | ||
| /// <param name="systemType">The target system type for the parsed value.</param> | ||
| /// <returns>The parameter parsed as the requested system type.</returns> | ||
| /// <exception cref="NotSupportedException"></exception> | ||
| protected static object ParseParamAsSystemType(string param, Type systemType) | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -295,9 +295,9 @@ private DocumentNode GenerateSqlGraphQLObjects(RuntimeEntities entities, Diction | |
| Dictionary<string, IEnumerable<string>> rolesAllowedForFields = new(); | ||
| SourceDefinition sourceDefinition = sqlMetadataProvider.GetSourceDefinition(entityName); | ||
| bool isStoredProcedure = entity.Source.Type is EntitySourceType.StoredProcedure; | ||
| EntityActionOperation operation = isStoredProcedure ? EntityActionOperation.Execute : EntityActionOperation.Read; | ||
| foreach (string column in sourceDefinition.Columns.Keys) | ||
| { | ||
| EntityActionOperation operation = isStoredProcedure ? EntityActionOperation.Execute : EntityActionOperation.Read; | ||
| IEnumerable<string> roles = _authorizationResolver.GetRolesForField(entityName, field: column, operation: operation); | ||
| if (!rolesAllowedForFields.TryAdd(key: column, value: roles)) | ||
| { | ||
|
|
@@ -309,7 +309,6 @@ private DocumentNode GenerateSqlGraphQLObjects(RuntimeEntities entities, Diction | |
| } | ||
| } | ||
|
|
||
| // The roles allowed for Fields are the roles allowed to READ the fields, so any role that has a read definition for the field. | ||
| // Only add objectTypeDefinition for GraphQL if it has a role definition defined for access. | ||
| if (rolesAllowedForEntity.Any()) | ||
| { | ||
|
|
@@ -397,23 +396,18 @@ private DocumentNode GenerateSqlGraphQLObjects(RuntimeEntities entities, Diction | |
| GenerateSourceTargetLinkingObjectDefinitions(objectTypes, linkingObjectTypes); | ||
| } | ||
|
|
||
| // Return a list of all the object types to be exposed in the schema. | ||
| Dictionary<string, FieldDefinitionNode> fields = new(); | ||
|
|
||
| // Add the DBOperationResult type to the schema | ||
| NameNode nameNode = new(value: GraphQLUtils.DB_OPERATION_RESULT_TYPE); | ||
| FieldDefinitionNode field = GetDbOperationResultField(); | ||
|
|
||
| fields.TryAdd(GraphQLUtils.DB_OPERATION_RESULT_FIELD_NAME, field); | ||
|
|
||
| // Add the DBOperationResult type to the schema | ||
| objectTypes.Add(GraphQLUtils.DB_OPERATION_RESULT_TYPE, new ObjectTypeDefinitionNode( | ||
| location: null, | ||
| name: nameNode, | ||
| description: null, | ||
| new List<DirectiveNode>(), | ||
| new List<NamedTypeNode>(), | ||
| fields.Values.ToImmutableList())); | ||
| ImmutableList.Create(GetDbOperationResultField()))); | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. by doing this arent we losing the key DB_OPERATION_RESULT_FIELD_NAME to value GetDbOperationResultField association? |
||
|
|
||
| // Return a list of all the object types to be exposed in the schema. | ||
| List<IDefinitionNode> nodes = new(objectTypes.Values); | ||
| nodes.AddRange(enumTypes.Values); | ||
| return new DocumentNode(nodes); | ||
|
|
@@ -748,7 +742,7 @@ private static FieldDefinitionNode GetDbOperationResultField() | |
| DocumentNode cosmosResult = GenerateCosmosGraphQLObjects(cosmosDataSourceNames, inputObjects); | ||
| DocumentNode sqlResult = GenerateSqlGraphQLObjects(sql, inputObjects); | ||
| // Create Root node with definitions from both cosmos and sql. | ||
| DocumentNode root = new(cosmosResult.Definitions.Concat(sqlResult.Definitions).ToImmutableList()); | ||
| DocumentNode root = cosmosResult.WithDefinitions(cosmosResult.Definitions.Concat(sqlResult.Definitions).ToImmutableList()); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why are we changing this? it seems we are re-wrapping the same type again.. |
||
|
|
||
| // Merge the inputobjectType definitions from cosmos and sql onto the root. | ||
| return (root.WithDefinitions(root.Definitions.Concat(inputObjects.Values).ToImmutableList()), inputObjects); | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -158,7 +158,7 @@ public virtual string GetSchemaName(string entityName) | |||||
| { | ||||||
| if (!EntityToDatabaseObject.TryGetValue(entityName, out DatabaseObject? databaseObject)) | ||||||
| { | ||||||
| throw new DataApiBuilderException(message: $"Table Definition for {entityName} has not been inferred.", | ||||||
| throw new DataApiBuilderException(message: $"Database object for entity '{entityName}' has not been inferred.", | ||||||
| statusCode: HttpStatusCode.InternalServerError, | ||||||
| subStatusCode: DataApiBuilderException.SubStatusCodes.EntityNotFound); | ||||||
| } | ||||||
|
|
@@ -176,7 +176,7 @@ public string GetDatabaseObjectName(string entityName) | |||||
| { | ||||||
| if (!EntityToDatabaseObject.TryGetValue(entityName, out DatabaseObject? databaseObject)) | ||||||
| { | ||||||
| throw new DataApiBuilderException(message: $"Table Definition for {entityName} has not been inferred.", | ||||||
| throw new DataApiBuilderException(message: $"Database object for entity '{entityName}' has not been inferred.", | ||||||
| statusCode: HttpStatusCode.InternalServerError, | ||||||
| subStatusCode: DataApiBuilderException.SubStatusCodes.EntityNotFound); | ||||||
| } | ||||||
|
|
@@ -189,7 +189,7 @@ public SourceDefinition GetSourceDefinition(string entityName) | |||||
| { | ||||||
| if (!EntityToDatabaseObject.TryGetValue(entityName, out DatabaseObject? databaseObject)) | ||||||
| { | ||||||
| throw new DataApiBuilderException(message: $"Table Definition for {entityName} has not been inferred.", | ||||||
| throw new DataApiBuilderException(message: $"Source definition for entity '{entityName}' has not been inferred.", | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| statusCode: HttpStatusCode.InternalServerError, | ||||||
| subStatusCode: DataApiBuilderException.SubStatusCodes.EntityNotFound); | ||||||
| } | ||||||
|
|
@@ -202,7 +202,7 @@ public StoredProcedureDefinition GetStoredProcedureDefinition(string entityName) | |||||
| { | ||||||
| if (!EntityToDatabaseObject.TryGetValue(entityName, out DatabaseObject? databaseObject)) | ||||||
| { | ||||||
| throw new DataApiBuilderException(message: $"Stored Procedure Definition for {entityName} has not been inferred.", | ||||||
| throw new DataApiBuilderException(message: $"Stored procedure definition for entity '{entityName}' has not been inferred.", | ||||||
| statusCode: HttpStatusCode.InternalServerError, | ||||||
| subStatusCode: DataApiBuilderException.SubStatusCodes.EntityNotFound); | ||||||
| } | ||||||
|
|
@@ -1733,7 +1733,7 @@ private async Task ValidateDatabaseConnection() | |||||
|
|
||||||
| /// <summary> | ||||||
| /// Using a data adapter, obtains the schema of the given table name | ||||||
| /// and adds the corresponding entity in the data set. | ||||||
| /// and adds the corresponding DataTable to the entities data set. | ||||||
| /// </summary> | ||||||
| private async Task<DataTable> FillSchemaForTableAsync( | ||||||
| string schemaName, | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -6,7 +6,7 @@ namespace Azure.DataApiBuilder.Service.GraphQLBuilder.GraphQLTypes | |
| /// <summary> | ||
| /// Only used to group the supported type names under a class with a relevant name. | ||
| /// The type names mentioned here are Hotchocolate scalar built in types. | ||
| /// The corresponding SQL type name may be different for e.g. UUID maps to Guid as the SQL type. | ||
| /// The corresponding SQL type name may be different for e.g. UUID maps to Guid as the .NET type. | ||
| /// </summary> | ||
| public static class SupportedHotChocolateTypes | ||
| { | ||
|
|
@@ -32,7 +32,6 @@ public static class SupportedHotChocolateTypes | |
| // new name so the generated schema does not depend on a deprecated scalar. | ||
| public const string BYTEARRAY_TYPE = "Base64String"; | ||
| public const string DATETIME_TYPE = "DateTime"; | ||
| public const string DATETIMEOFFSET_TYPE = "DateTimeOffset"; | ||
| public const string LOCALTIME_TYPE = "LocalTime"; | ||
| public const string TIME_TYPE = "Time"; | ||
| } | ||
|
|
@@ -46,6 +45,7 @@ public static class SupportedDateTimeTypes | |
| public const string DATE_TYPE = "date"; | ||
| public const string SMALLDATETIME_TYPE = "smalldatetime"; | ||
| public const string DATETIME2_TYPE = "datetime2"; | ||
| public const string DATETIMEOFFSET_TYPE = "DateTimeOffset"; | ||
|
Comment on lines
45
to
+48
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should this be all in lowercase and consistent to the above supported data types? |
||
| } | ||
|
|
||
| /// <summary> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -67,7 +67,7 @@ public async Task PG_real_graphql_single_filter_expectedValues( | |
| [DataRow(BOOLEAN_TYPE, "'false'", "false")] | ||
| [DataRow(STRING_TYPE, "lksa;jdflasdf;alsdflksdfkldj", "\"lksa;jdflasdf;alsdflksdfkldj\"")] | ||
| [DataTestMethod] | ||
| public async Task PGSQL_real_graphql_in_filter_expectedValues( | ||
| public async Task PGSQL_graphql_in_filter_expectedValues( | ||
| string type, | ||
| string sqlValue, | ||
| string gqlValue) | ||
|
|
@@ -99,7 +99,7 @@ protected override string MakeQueryOnTypeTable( | |
| string orderBy = "id", | ||
| string limit = "1") | ||
| { | ||
| string formattedSelect = limit.Equals("1") ? "SELECT to_jsonb(subq3) AS DATA" : "SELECT json_agg(to_jsonb(subq3)) AS DATA"; | ||
| string formattedSelect = limit.Equals("1") ? "SELECT to_jsonb(subq3) AS DATA" : "SELECT COALESCE(json_agg(to_jsonb(subq3)), '[]'::json) AS DATA"; | ||
|
|
||
| return @" | ||
| " + formattedSelect + @" | ||
|
|
@@ -141,15 +141,5 @@ private static string ProperlyFormatTypeTableColumn(string columnName) | |
| return columnName; | ||
| } | ||
| } | ||
|
|
||
| /// <summary> | ||
| /// Bypass DateTime GQL tests for PostreSql | ||
| /// </summary> | ||
| [DataTestMethod] | ||
| [Ignore] | ||
| public new void QueryTypeColumnFilterAndOrderByDateTime(string type, string filterOperator, string sqlValue, string gqlValue, string queryOperator) | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This test should work now. Please keep it and make it functional |
||
| { | ||
| Assert.Inconclusive("Test skipped for PostgreSql."); | ||
| } | ||
| } | ||
| } | ||
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.
If we need this implementation only for PostgreSQL explicitly, please override this function into a PostgreSqlQueryStructure. If such a class doesnt exist, please create it as a derived class inheriting from BaseSqlQueryStructure.