STRLCPY/graphql-engine

fix bug with multiple fields in boolean expressions (#442)

<!-- Thank you for submitting this PR! :) -->
# ⚠️ Behaviour change in query execution

## Description

<!--
  Questions to consider answering:
  1. What user-facing changes are being made?
2. What are issues related to this PR? (Consider adding `(close
#<issue-no>)` to the PR title)
  3. What is the conceptual design behind this PR?
  4. How can this PR be tested/verified?
  5. Does the PR have limitations?
  6. Does the PR introduce breaking changes?
-->

This PR fixes a bug (different behaviour from v2) with boolean
expressions.

Slack thread:
https://hasurahq.slack.com/archives/C066TKMH79R/p1711987325682919

JIRA: https://hasurahq.atlassian.net/browse/V3ENGINE-67

## Changelog

- Add a changelog entry (in the "Changelog entry" section below) if the
changes in this PR have any user-facing impact. See [changelog
guide](https://github.com/hasura/graphql-engine-mono/wiki/Changelog-Guide).
- If no changelog is required ignore/remove this section and add a
`no-changelog-required` label to the PR.

### Product
_(Select all products this will be available in)_
- [ ] community-edition
- [ ] cloud
<!-- product : end : DO NOT REMOVE -->

### Type
<!-- See changelog structure:
https://github.com/hasura/graphql-engine-mono/wiki/Changelog-Guide#structure-of-our-changelog
-->
_(Select only one. In case of multiple, choose the most appropriate)_
- [ ] highlight
- [ ] enhancement
- [ ] bugfix
- [ ] behaviour-change
- [ ] performance-enhancement
- [ ] security-fix
<!-- type : end : DO NOT REMOVE -->

### Changelog entry
<!--
  - Add a user understandable changelog entry
- Include all details needed to understand the change. Try including
links to docs or issues if relevant
  - For Highlights start with a H4 heading (#### <entry title>)
  - Get the changelog entry reviewed by your team
-->

_Replace with changelog entry_

<!-- changelog-entry : end : DO NOT REMOVE -->

<!-- changelog : end : DO NOT REMOVE -->

V3_GIT_ORIGIN_REV_ID: 4fcfc16a9a88ed6362315ca2f47911e0c97b7829

paritosh-08 committed with hasura-bot 1 month ago

e56594fe

1 parent 9a6cb644

Revision indexing in progress... (symbol navigation in revisions will be accurate after indexed)

Total 18 files Show one by one

■ ■ ■ ■ ■ ■

v3/crates/engine/src/execute/ir/filter.rs

		skipped 17 lines
18	18
19	19		#[derive(Debug, Serialize)]
20	20		pub(crate) struct ResolvedFilterExpression<'s> {
21		-	pub expressions: Vec<ndc_models::Expression>,
	21	+	pub expression: Option<ndc_models::Expression>,
22	22		// relationships that were used in the filter expression. This is helpful
23	23		// for collecting relatinships and sending collection_relationships
24	24		pub relationships: BTreeMap<NDCRelationshipName, LocalModelRelationshipInfo<'s>>,
		skipped 12 lines
37	37		build_filter_expression(field, relationship_paths, &mut relationships, usage_counts)?;
38	38		expressions.extend(expression);
39	39		}
	40	+	let expression = ndc_models::Expression::And { expressions };
40	41		let resolved_filter_expression = ResolvedFilterExpression {
41		-	expressions,
	42	+	expression: Some(expression),
42	43		relationships,
43	44		};
44	45		Ok(resolved_filter_expression)
		skipped 23 lines
68	69		for value in values.iter() {
69	70		let resolved_filter_expression =
70	71		resolve_filter_expression(value.as_object()?, usage_counts)?;
71		-	expressions.extend(resolved_filter_expression.expressions);
	72	+	expressions.extend(resolved_filter_expression.expression);
72	73		relationships.extend(resolved_filter_expression.relationships);
73	74		}
74	75
		skipped 13 lines
88	89		for value in values.iter() {
89	90		let resolved_filter_expression =
90	91		resolve_filter_expression(value.as_object()?, usage_counts)?;
91		-	expressions.extend(resolved_filter_expression.expressions);
	92	+	expressions.extend(resolved_filter_expression.expression);
92	93		relationships.extend(resolved_filter_expression.relationships);
93	94		}
94	95
		skipped 7 lines
102	103		field: types::ModelFilterArgument::NotOp,
103	104		},
104	105		)) => {
105		-	let mut expressions = Vec::new();
106	106		let value = field.value.as_object()?;
107	107
108	108		let resolved_filter_expression = resolve_filter_expression(value, usage_counts)?;
109	109		relationships.extend(resolved_filter_expression.relationships);
110	110
111		-	expressions.push(ndc_models::Expression::Not {
112		-	expression: Box::new(ndc_models::Expression::And {
113		-	expressions: resolved_filter_expression.expressions,
114		-	}),
115		-	});
	111	+	let expressions = match resolved_filter_expression.expression {
	112	+	Some(expression) => vec![ndc_models::Expression::Not {
	113	+	expression: Box::new(expression),
	114	+	}],
	115	+	None => Vec::new(),
	116	+	};
116	117		Ok(expressions)
117	118		}
118	119		// The column that we want to use for filtering. If the column happens
		skipped 169 lines

■ ■ ■ ■ ■ ■

v3/crates/engine/src/execute/ir/model_selection.rs

		skipped 71 lines
72	72		&mut permissions_predicate_relationships,
73	73		usage_counts,
74	74		)?;
75		-	filter_clauses.expressions.push(processed_model_perdicate);
	75	+	filter_clauses.expression = match filter_clauses.expression {
	76	+	Some(existing) => Some(ndc_models::Expression::And {
	77	+	expressions: vec![existing, processed_model_perdicate],
	78	+	}),
	79	+	None => Some(processed_model_perdicate),
	80	+	};
76	81		for (rel_name, rel_info) in permissions_predicate_relationships {
77	82		filter_clauses.relationships.insert(rel_name, rel_info);
78	83		}
		skipped 33 lines

■ ■ ■ ■ ■ ■

v3/crates/engine/src/execute/ir/query_root/apollo_federation.rs

		skipped 158 lines
159	159		let new_selection_set = field.selection_set.filter_field_calls_by_typename(typename);
160	160
161	161		let filter_clauses = ResolvedFilterExpression {
162		-	expressions: filter_clause_expressions,
	162	+	expression: Some(ndc_models::Expression::And {
	163	+	expressions: filter_clause_expressions,
	164	+	}),
163	165		relationships: BTreeMap::new(),
164	166		};
165	167		let mut usage_counts = UsagesCounts::new();
		skipped 25 lines

■ ■ ■ ■ ■ ■

v3/crates/engine/src/execute/ir/query_root/node_field.rs

		skipped 142 lines
143	143		let mut usage_counts = UsagesCounts::new();
144	144
145	145		let filter_clauses = ResolvedFilterExpression {
146		-	expressions: filter_clause_expressions,
	146	+	expression: Some(ndc_models::Expression::And {
	147	+	expressions: filter_clause_expressions,
	148	+	}),
147	149		relationships: BTreeMap::new(),
148	150		};
149	151
		skipped 24 lines

■ ■ ■ ■ ■ ■

v3/crates/engine/src/execute/ir/query_root/select_many.rs

		skipped 51 lines
52	52		let mut limit = None;
53	53		let mut offset = None;
54	54		let mut filter_clause = ResolvedFilterExpression {
55		-	expressions: Vec::new(),
	55	+	expression: None,
56	56		relationships: BTreeMap::new(),
57	57		};
58	58		let mut order_by = None;
		skipped 104 lines

■ ■ ■ ■ ■ ■

v3/crates/engine/src/execute/ir/query_root/select_one.rs

		skipped 114 lines
115	115		count_model(model_name.clone(), &mut usage_counts);
116	116
117	117		let filter_clause = ResolvedFilterExpression {
118		-	expressions: filter_clause_expressions,
	118	+	expression: Some(ndc_models::Expression::And {
	119	+	expressions: filter_clause_expressions,
	120	+	}),
119	121		relationships: BTreeMap::new(),
120	122		};
121	123
		skipped 23 lines

■ ■ ■ ■ ■ ■

v3/crates/engine/src/execute/ir/relationship.rs

		skipped 226 lines
227	227		let mut limit = None;
228	228		let mut offset = None;
229	229		let mut filter_clause = ResolvedFilterExpression {
230		-	expressions: Vec::new(),
	230	+	expression: None,
231	231		relationships: BTreeMap::new(),
232	232		};
233	233		let mut order_by = None;
		skipped 292 lines
526	526		name: target_value_variable,
527	527		},
528	528		};
529		-	remote_relationships_ir
530		-	.filter_clause
531		-	.expressions
532		-	.push(comparison_exp);
	529	+	remote_relationships_ir.filter_clause.expression =
	530	+	match remote_relationships_ir.filter_clause.expression {
	531	+	Some(existing) => Some(ndc_models::Expression::And {
	532	+	expressions: vec![existing, comparison_exp],
	533	+	}),
	534	+	None => Some(comparison_exp),
	535	+	};
533	536		}
534	537		let rel_info = RemoteModelRelationshipInfo {
535	538		annotation,
		skipped 88 lines

■ ■ ■ ■ ■ ■

v3/crates/engine/src/execute/plan/model_selection.rs

		skipped 21 lines
22	22		limit: ir.limit,
23	23		offset: ir.offset,
24	24		order_by: ir.order_by.as_ref().map(\|x\| x.order_by.clone()),
25		-	predicate: match ir.filter_clause.expressions.as_slice() {
26		-	[] => None,
27		-	[expression] => Some(expression.clone()),
28		-	expressions => Some(ndc_models::Expression::And {
29		-	expressions: expressions.to_vec(),
30		-	}),
31		-	},
	25	+	predicate: ir.filter_clause.expression.clone(),
32	26		};
33	27		Ok((ndc_query, join_locations))
34	28		}
		skipped 20 lines

■ ■ ■ ■ ■ ■

v3/crates/engine/tests/execute/models/select_many/where/multiple_fields/expected.json

1	+	[
2	+	{
3	+	"data": {
4	+	"ArticleMany": [
5	+	{
6	+	"article_id": 1,
7	+	"title": "The Next 700 Programming Languages"
8	+	},
9	+	{
10	+	"article_id": 4,
11	+	"title": "The Mechanical Evaluation of Expressions"
12	+	}
13	+	]
14	+	}
15	+	},
16	+	{
17	+	"data": {
18	+	"ArticleMany": [
19	+	{
20	+	"article_id": 1,
21	+	"title": "The Next 700 Programming Languages"
22	+	}
23	+	]
24	+	}
25	+	}
26	+	]
27	+

■ ■ ■ ■ ■ ■

v3/crates/engine/tests/execute/models/select_many/where/multiple_fields/metadata.json

1	+	{
2	+	"version": "v2",
3	+	"subgraphs": [
4	+	{
5	+	"name": "default",
6	+	"objects": [
7	+	{
8	+	"kind": "DataConnectorScalarRepresentation",
9	+	"version": "v1",
10	+	"definition": {
11	+	"dataConnectorName": "db",
12	+	"dataConnectorScalarType": "String",
13	+	"representation": "String",
14	+	"graphql": {
15	+	"comparisonExpressionTypeName": "StringComparisonExp"
16	+	}
17	+	}
18	+	},
19	+	{
20	+	"kind": "DataConnectorScalarRepresentation",
21	+	"version": "v1",
22	+	"definition": {
23	+	"dataConnectorName": "db",
24	+	"dataConnectorScalarType": "Int",
25	+	"representation": "Int",
26	+	"graphql": {
27	+	"comparisonExpressionTypeName": "IntComparisonExp"
28	+	}
29	+	}
30	+	},
31	+	{
32	+	"kind": "ObjectType",
33	+	"version": "v1",
34	+	"definition": {
35	+	"name": "article",
36	+	"fields": [
37	+	{
38	+	"name": "article_id",
39	+	"type": "Int!"
40	+	},
41	+	{
42	+	"name": "title",
43	+	"type": "String!"
44	+	},
45	+	{
46	+	"name": "author_id",
47	+	"type": "Int!"
48	+	}
49	+	],
50	+	"graphql": {
51	+	"typeName": "Article"
52	+	},
53	+	"dataConnectorTypeMapping": [
54	+	{
55	+	"dataConnectorName": "db",
56	+	"dataConnectorObjectType": "article",
57	+	"fieldMapping": {
58	+	"article_id": {
59	+	"column": {
60	+	"name": "id"
61	+	}
62	+	},
63	+	"title": {
64	+	"column": {
65	+	"name": "title"
66	+	}
67	+	},
68	+	"author_id": {
69	+	"column": {
70	+	"name": "author_id"
71	+	}
72	+	}
73	+	}
74	+	}
75	+	]
76	+	}
77	+	},
78	+	{
79	+	"kind": "ObjectBooleanExpressionType",
80	+	"version": "v1",
81	+	"definition": {
82	+	"name": "article_bool_exp",
83	+	"objectType": "article",
84	+	"dataConnectorName": "db",
85	+	"dataConnectorObjectType": "article",
86	+	"comparableFields": [
87	+	{
88	+	"fieldName": "author_id",
89	+	"operators": {
90	+	"enableAll": true
91	+	}
92	+	},
93	+	{
94	+	"fieldName": "article_id",
95	+	"operators": {
96	+	"enableAll": true
97	+	}
98	+	},
99	+	{
100	+	"fieldName": "title",
101	+	"operators": {
102	+	"enableAll": true
103	+	}
104	+	}
105	+	],
106	+	"graphql": {
107	+	"typeName": "Article_Where_Exp"
108	+	}
109	+	}
110	+	},
111	+	{
112	+	"kind": "Model",
113	+	"version": "v1",
114	+	"definition": {
115	+	"name": "Articles",
116	+	"objectType": "article",
117	+	"source": {
118	+	"dataConnectorName": "db",
119	+	"collection": "article"
120	+	},
121	+	"filterExpressionType": "article_bool_exp",
122	+	"orderableFields": [
123	+	{
124	+	"fieldName": "article_id",
125	+	"orderByDirections": {
126	+	"enableAll": true
127	+	}
128	+	},
129	+	{
130	+	"fieldName": "title",
131	+	"orderByDirections": {
132	+	"enableAll": true
133	+	}
134	+	},
135	+	{
136	+	"fieldName": "author_id",
137	+	"orderByDirections": {
138	+	"enableAll": true
139	+	}
140	+	}
141	+	],
142	+	"graphql": {
143	+	"selectUniques": [
144	+	{
145	+	"queryRootField": "ArticleByID",
146	+	"uniqueIdentifier": [
147	+	"article_id"
148	+	]
149	+	}
150	+	],
151	+	"selectMany": {
152	+	"queryRootField": "ArticleMany"
153	+	},
154	+	"orderByExpressionType": "Article_Order_By"
155	+	}
156	+	}
157	+	},
158	+	{
159	+	"kind": "TypePermissions",
160	+	"version": "v1",
161	+	"definition": {
162	+	"typeName": "article",
163	+	"permissions": [
164	+	{
165	+	"role": "admin",
166	+	"output": {
167	+	"allowedFields": [
168	+	"title",
169	+	"article_id",
170	+	"author_id"
171	+	]
172	+	}
173	+	},
174	+	{
175	+	"role": "user",
176	+	"output": {
177	+	"allowedFields": [
178	+	"title",
179	+	"article_id",
180	+	"author_id"
181	+	]
182	+	}
183	+	}
184	+	]
185	+	}
186	+	},
187	+	{
188	+	"kind": "ModelPermissions",
189	+	"version": "v1",
190	+	"definition": {
191	+	"modelName": "Articles",
192	+	"permissions": [
193	+	{
194	+	"role": "admin",
195	+	"select": {
196	+	"filter": null
197	+	}
198	+	},
199	+	{
200	+	"role": "user",
201	+	"select": {
202	+	"filter": {
203	+	"fieldComparison": {
204	+	"field": "article_id",
205	+	"operator": "_eq",
206	+	"value": {
207	+	"sessionVariable": "x-hasura-user-id"
208	+	}
209	+	}
210	+	}
211	+	}
212	+	}
213	+	]
214	+	}
215	+	}
216	+	]
217	+	}
218	+	]
219	+	}
220	+

■ ■ ■ ■ ■ ■

v3/crates/engine/tests/execute/models/select_many/where/multiple_fields/request.gql

1	+	query MyQuery {
2	+	ArticleMany(where: {_or: [
3	+	{article_id: {_eq: 4}, title: {_eq: "The Mechanical Evaluation of Expressions"}},
4	+	{article_id: {_eq: 1}}
5	+	]}) {
6	+	article_id
7	+	title
8	+	}
9	+	}
10	+

■ ■ ■ ■ ■ ■

v3/crates/engine/tests/execute/models/select_many/where/multiple_fields/session_variables.json

1 + [
2 + {
3 + "x-hasura-role": "admin"
4 + },
5 + {
6 + "x-hasura-role": "user",
7 + "x-hasura-user-id": "1"
8 + }
9 + ]
10 +

All occurrences

■ ■ ■ ■ ■ ■

v3/crates/engine/tests/execution.rs

		skipped 231 lines
232	232		}
233	233
234	234		#[test]
	235	+	fn test_model_select_many_where_multiple_fields() -> anyhow::Result<()> {
	236	+	let test_path_string = "execute/models/select_many/where/multiple_fields";
	237	+	let common_metadata_path_string = "execute/common_metadata/postgres_connector_schema.json";
	238	+	common::test_execution_expectation(test_path_string, &[common_metadata_path_string])
	239	+	}
	240	+
	241	+	#[test]
235	242		fn test_model_select_many_select_with_args() -> anyhow::Result<()> {
236	243		let test_path_string = "execute/models/select_many/select_with_args";
237	244		let common_metadata_path_string = "execute/common_metadata/postgres_connector_schema.json";
		skipped 734 lines

■ ■ ■ ■ ■ ■

v3/crates/engine/tests/generate_ir/get_by_id/expected.json

		skipped 108 lines
109	109		"collection": "article",
110	110		"arguments": {},
111	111		"filter_clause": {
112		-	"expressions": [
113		-	{
114		-	"type": "binary_comparison_operator",
115		-	"column": {
116		-	"type": "column",
117		-	"name": "id",
118		-	"path": []
119		-	},
120		-	"operator": "_eq",
121		-	"value": {
122		-	"type": "scalar",
123		-	"value": 1
	112	+	"expression": {
	113	+	"type": "and",
	114	+	"expressions": [
	115	+	{
	116	+	"type": "binary_comparison_operator",
	117	+	"column": {
	118	+	"type": "column",
	119	+	"name": "id",
	120	+	"path": []
	121	+	},
	122	+	"operator": "_eq",
	123	+	"value": {
	124	+	"type": "scalar",
	125	+	"value": 1
	126	+	}
124	127		}
125		-	}
126		-	],
	128	+	]
	129	+	},
127	130		"relationships": {}
128	131		},
129	132		"limit": null,
		skipped 42 lines

■ ■ ■ ■ ■ ■

v3/crates/engine/tests/generate_ir/get_many/expected.json

		skipped 101 lines
102	102		"collection": "article",
103	103		"arguments": {},
104	104		"filter_clause": {
105		-	"expressions": [],
	105	+	"expression": null,
106	106		"relationships": {}
107	107		},
108	108		"limit": 1,
		skipped 47 lines

■ ■ ■ ■ ■ ■

v3/crates/engine/tests/generate_ir/get_many_model_count/expected.json

		skipped 489 lines
490	490		"collection": "article",
491	491		"arguments": {},
492	492		"filter_clause": {
493		-	"expressions": [],
	493	+	"expression": null,
494	494		"relationships": {}
495	495		},
496	496		"limit": null,
		skipped 19 lines
516	516		"collection": "author",
517	517		"arguments": {},
518	518		"filter_clause": {
519		-	"expressions": [],
	519	+	"expression": null,
520	520		"relationships": {}
521	521		},
522	522		"limit": null,
		skipped 19 lines
542	542		"collection": "article",
543	543		"arguments": {},
544	544		"filter_clause": {
545		-	"expressions": [],
	545	+	"expression": null,
546	546		"relationships": {}
547	547		},
548	548		"limit": null,
		skipped 438 lines

■ ■ ■ ■ ■ ■

v3/crates/engine/tests/generate_ir/get_many_user_2/expected.json

		skipped 108 lines
109	109		"collection": "article",
110	110		"arguments": {},
111	111		"filter_clause": {
112		-	"expressions": [
113		-	{
114		-	"type": "and",
115		-	"expressions": [
116		-	{
117		-	"type": "binary_comparison_operator",
	112	+	"expression": {
	113	+	"type": "and",
	114	+	"expressions": [
	115	+	{
	116	+	"type": "binary_comparison_operator",
	117	+	"column": {
	118	+	"type": "column",
	119	+	"name": "title",
	120	+	"path": []
	121	+	},
	122	+	"operator": "_like",
	123	+	"value": {
	124	+	"type": "scalar",
	125	+	"value": "%Functional%"
	126	+	}
	127	+	},
	128	+	{
	129	+	"type": "not",
	130	+	"expression": {
	131	+	"type": "unary_comparison_operator",
118	132		"column": {
119	133		"type": "column",
120		-	"name": "title",
	134	+	"name": "author_id",
121	135		"path": []
122	136		},
123		-	"operator": "_like",
124		-	"value": {
125		-	"type": "scalar",
126		-	"value": "%Functional%"
127		-	}
128		-	},
129		-	{
130		-	"type": "not",
131		-	"expression": {
132		-	"type": "unary_comparison_operator",
133		-	"column": {
134		-	"type": "column",
135		-	"name": "author_id",
136		-	"path": []
137		-	},
138		-	"operator": "is_null"
139		-	}
	137	+	"operator": "is_null"
140	138		}
141		-	]
142		-	}
143		-	],
	139	+	}
	140	+	]
	141	+	},
144	142		"relationships": {}
145	143		},
146	144		"limit": null,
		skipped 47 lines

■ ■ ■ ■ ■ ■

v3/crates/engine/tests/generate_ir/get_many_where/expected.json

		skipped 101 lines
102	102		"collection": "article",
103	103		"arguments": {},
104	104		"filter_clause": {
105		-	"expressions": [
106		-	{
107		-	"type": "binary_comparison_operator",
108		-	"column": {
109		-	"type": "column",
110		-	"name": "title",
111		-	"path": []
112		-	},
113		-	"operator": "_like",
114		-	"value": {
115		-	"type": "scalar",
116		-	"value": "random"
	105	+	"expression": {
	106	+	"type": "and",
	107	+	"expressions": [
	108	+	{
	109	+	"type": "binary_comparison_operator",
	110	+	"column": {
	111	+	"type": "column",
	112	+	"name": "title",
	113	+	"path": []
	114	+	},
	115	+	"operator": "_like",
	116	+	"value": {
	117	+	"type": "scalar",
	118	+	"value": "random"
	119	+	}
117	120		}
118		-	}
119		-	],
	121	+	]
	122	+	},
120	123		"relationships": {}
121	124		},
122	125		"limit": 1,
		skipped 156 lines
279	282		"collection": "author",
280	283		"arguments": {},
281	284		"filter_clause": {
282		-	"expressions": [
283		-	{
284		-	"type": "not",
285		-	"expression": {
286		-	"type": "unary_comparison_operator",
287		-	"column": {
288		-	"type": "column",
289		-	"name": "first_name",
290		-	"path": []
291		-	},
292		-	"operator": "is_null"
	285	+	"expression": {
	286	+	"type": "and",
	287	+	"expressions": [
	288	+	{
	289	+	"type": "not",
	290	+	"expression": {
	291	+	"type": "unary_comparison_operator",
	292	+	"column": {
	293	+	"type": "column",
	294	+	"name": "first_name",
	295	+	"path": []
	296	+	},
	297	+	"operator": "is_null"
	298	+	}
293	299		}
294		-	}
295		-	],
	300	+	]
	301	+	},
296	302		"relationships": {}
297	303		},
298	304		"limit": null,
		skipped 47 lines

1	+	[
2	+	{
3	+	"x-hasura-role": "admin"
4	+	},
5	+	{
6	+	"x-hasura-role": "user",
7	+	"x-hasura-user-id": "1"
8	+	}
9	+	]
10	+