Add support for DateTime filters in PostgreSQL - #3728
Conversation
…ately describe the source of the exception
There was a problem hiding this comment.
Pull request overview
This PR aims to fix PostgreSQL GraphQL filter failures for date/time columns (issue #3094) by ensuring filter parameter values are converted from raw GraphQL string values into the appropriate .NET types (DateTime / DateTimeOffset) before being added as DB parameters, preventing PostgreSQL from treating them as text.
Changes:
- Override
BaseSqlQueryStructure.MakeDbConnectionParamto pre-parse PostgreSQL column parameters fromstringinto the column’s inferred .NET system type before delegating to the base parameter creation logic. - Minor GraphQL schema-generation refactors/cleanups (including primary key directive usage changes and DocumentNode composition changes).
- Improve/clarify several metadata-provider exception messages and update related comments/constants organization.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/Service.GraphQLBuilder/Sql/SchemaConverter.cs | Adjusts schema generation docs and changes how the primaryKey directive is emitted for PK fields. |
| src/Service.GraphQLBuilder/GraphQLTypes/SupportedTypes.cs | Reorganizes date/time-related type constants and updates comments. |
| src/Core/Services/MetadataProviders/SqlMetadataProvider.cs | Improves exception messages and clarifies a schema-fill comment. |
| src/Core/Services/GraphQLSchemaCreator.cs | Refactors schema assembly and uses WithDefinitions when composing root DocumentNode. |
| src/Core/Resolvers/Sql Query Structures/BaseSqlQueryStructure.cs | Adds PostgreSQL-specific parameter type pre-processing in MakeDbConnectionParam to address date/time filter casting issues. |
| /// <inheritdoc /> | ||
| public override string MakeDbConnectionParam(object? value, string? paramName = null, bool lengthOverride = false) | ||
| { | ||
| if (MetadataProvider.GetDatabaseType() is DatabaseType.PostgreSQL && |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I think there are a couple of issues with this approach. BaseSqlQueryStructure is inherited directly by 7 different concrete classes, each representing a different SQL operation. The inheritance model is based off of the SQL operation instead of the database type, and I don't think it would be worth changing it just for this method override. Even if we were to create a PostgreSqlQueryStructure class and override the MakeDbConnectionParam method, the 7 existing concrete classes would then need to derive from PostgreSqlQueryStructure instead of BaseSqlQueryStructure only when the database is PostgreSQL. The base class is fixed at compile time (it can't vary per instance/runtime), so this would not work in practice. Furthermore, we are just moving the "if PostgreSQL do this, else do this" decision to 7 different places instead of consolidating it here in one place. The codebase already uses runtime DatabaseType checks in SqlMutationEngine.cs and SqlMetadataProvider.cs, so this is nothing new.
There was a problem hiding this comment.
Let me know if this makes sense or if you still have concerns.
| { | ||
| if (MetadataProvider.GetDatabaseType() is DatabaseType.PostgreSQL && | ||
| !string.IsNullOrEmpty(paramName) && | ||
| value is string stringValue && |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I'm not sure I'm following you here. This may need to be explored further.
There was a problem hiding this comment.
So, if I am understanding correctly, the case you are bringing up is when the value is not a string. In that case, it is already coerced to some .NET type. I think that is the responsibility of the code that first coerces the value to make sure it is coerced correctly. We could verify that here but again I don't think that is our responsibility. Let me know if you still have concerns.
| new List<DirectiveNode>(), | ||
| new List<NamedTypeNode>(), | ||
| fields.Values.ToImmutableList())); | ||
| ImmutableList.Create(GetDbOperationResultField()))); |
There was a problem hiding this comment.
by doing this arent we losing the key DB_OPERATION_RESULT_FIELD_NAME to value GetDbOperationResultField association?
There was a problem hiding this comment.
We are, but as far as I can see that association was never utilized anywhere. Let me know if this makes sense or if you still have concerns.
| /// </summary> | ||
| [DataTestMethod] | ||
| [Ignore] | ||
| public new void QueryTypeColumnFilterAndOrderByDateTime(string type, string filterOperator, string sqlValue, string gqlValue, string queryOperator) |
There was a problem hiding this comment.
This test should work now. Please keep it and make it functional
There was a problem hiding this comment.
All the tests are passing. I removed this code because it is ignoring the tests in the QueryTypeColumnFilterAndOrderByDateTime method specifically for PostgreSQL. Now those tests are running.
There was a problem hiding this comment.
Let me know if this makes sense or if you still have concerns.
Aniruddh25
left a comment
There was a problem hiding this comment.
Good description on what is the change. Needs DateTime integration tests.
souvikghosh04
left a comment
There was a problem hiding this comment.
Approved, assuming the comments would be addressed correctly.
| 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()); |
There was a problem hiding this comment.
why are we changing this? it seems we are re-wrapping the same type again..
There was a problem hiding this comment.
Well, I think this is the idiomatic way to do it although both have the same effect. If you look at the implementation of the WithDefinitions method in DocumentNode.cs, it calls new under the hood.
There was a problem hiding this comment.
Let me know if this makes sense or if you still have concerns.
… class summary comment
Why make this change?
This pull request is to address #3094, which is a bug that happens because certain date/time SQL parameters are not being parsed to their appropriate .NET type (
DateTimeorDateTimeOffset) and are instead being treated as raw text. This is leading to type conversion issues when executing the underlying SQL query.What is this change?
I have provided an overridden implementation of
MakeDbConnectionParaminBaseSqlQueryStructure.cs. This method does some extra pre-processing for PostgreSQL parameters specifically by parsing each parameter into its appropriate system type before calling the base class implementation ofMakeDbConnectionParaminBaseQueryStructure.cs. The system type of each parameter is retrieved from metadata of the underlying column that this parameter corresponds to via the methodGetColumnSystemType. This change fixes the relevant issue because now parameters which correspond to an underlyingdatetimecolumn in the schema are cast to their appropriate .NET type instead of falling through as text before building the final SQL query. Furthermore, this change is limited in scope to apply this parameter casting only when the underlying database is PostgreSQL. This ensures we do not see unexpected behavior with other database types where this sort of parameter casting may be unnecessary and/or erroneous.My original thought was to modify the method
ExtractValueFromIValueNodeinExecutionHelper.csto explicitly handle anIValueNodewith a GraphQL scalar type ofSupportedHotChocolateTypes.DATETIME_TYPEby parsing theIValueNodeas aDateTimesystem type before being returned. For all practical purposes, this has the same desired effect as parsing the parameter inMakeDbConnectionParam, but the call toExtractValueFromIValueNodehappens upstream of the call toMakeDbConnectionParam.I believe that casting the parameters to their appropriate .NET type in
MakeDbConnectionParamis appropriate because the method literally builds each parameter for the SQL query and the .NET type casting can be considered a part of this parameter building process. I do not think .NET type casting is as appropriate inExtractValueFromIValueNodebecause this method serves the broader purpose of extracting a value from a GraphQLIValueNode, where theIValueNodecould contain a scalar but it could also contain a variable that needs to be resolved recursively. For parameters that need to be cast to a .NET type, the control flow will go fromExtractValueFromIValueNodeto eventuallyMakeDbConnectionParam, but again,MakeDbConnectionParamserves the sole purpose of building the parameter whileExtractValueFromIValueNodeserves a broader purpose.How was this tested?
Sample Request(s)
With query:
Before:
After: