-
Notifications
You must be signed in to change notification settings - Fork 3
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
accept predicate arguments #92
Conversation
variable_name: &ndc::VariableName, | ||
expected_type: Option<Type<T::ScalarType>>, | ||
) { | ||
// self.register_variable_use_helper(variable_name, Some(expected_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.
Why is this commented out?
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.
Oh good catch! There was a point where I had split out a helper function to cover some of that function's logic, but then I ended up keeping everything in one function.
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.
Looks good
Accept predicate arguments in native mutations and native queries.
Unfortunately this needs more work - the code for parsing predicate values is incorrect. I expected to receive a value of type
ndc_models::Expression
, but in the integration test in this PR the predicate value is:The parsing code is in the
plan_for_predicate
function incrates/ndc-query-plan/src/plan_for_query_request/plan_for_arguments.rs
.Somehow that value needs to be turned into anExpression
. Unless there is a parser already written for this predicate format we'll have to make one.Edit: I'm told the predicate value is supposed to be a serialized
Expression
. This might be a bug in the engine. If so this PR is ready to go if that bug can be resolved, and the tests pass.I moved logic that had previously been implemented in the connector into ndc-query-plan, creating
plan_for_mutation_request
in the process. Parsing predicates, and matching up types to arguments is now done in database-agnostic code in ndc-query-plan.So
ndc_models::Type
has aPredicate
variant, but I chose not to add a predicate variant tondc_query_plan::Type
. That is because if I did there would be a number of cases where I would have to error out in cases where we have values, but where a predicate doesn't make sense. I don't think predicates are query-time values anyway - they only apply at query build time. They aren't stored in databases, they can't be given in variables, they aren't returned in responses.Following the philosophy of making invalid states unrepresentable I kept the
ndc_query_plan::Type
as-is, and added variants to theArgument
types instead to distinguish predicates from query-time values. For example here is the new version ofndc_query_plan::Argument
:There are similar changes to
RelationArgument
, and to a new type,MutationProcedureArgument
.Completes https://linear.app/hasura/issue/NDC-175/accept-predicate-as-an-argument-type-with-native-mutations