feat: JSON type filter #3122

nasdf · 2024-10-08T17:46:30Z

Relevant issue(s)

Resolves #3106

Description

This PR enables filtering on JSON field types.

Tasks

I made sure the code is well commented, particularly hard-to-understand areas.
I made sure the repository-held documentation is changed accordingly.
I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Added integration tests.

Specify the platform(s) on which this was tested:

MacOS

codecov · 2024-10-08T18:07:39Z

Codecov Report

Attention: Patch coverage is 91.30435% with 8 lines in your changes missing coverage. Please review.

Project coverage is 79.98%. Comparing base (858f4f1) to head (2975ef9).
Report is 1 commits behind head on develop.

Files with missing lines	Patch %	Lines
internal/planner/mapper/targetable.go	64.71%	6 Missing ⚠️
internal/request/graphql/schema/generate.go	92.31%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3122      +/-   ##
===========================================
+ Coverage    79.91%   79.98%   +0.07%     
===========================================
  Files          353      353              
  Lines        28466    28406      -60     
===========================================
- Hits         22747    22719      -28     
+ Misses        4097     4073      -24     
+ Partials      1622     1614       -8

Flag	Coverage Δ
all-tests	`79.98% <91.30%> (+0.07%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines	Coverage Δ
internal/connor/all.go	`76.67% <100.00%> (+1.67%)`	⬆️
internal/connor/any.go	`83.33% <100.00%> (+1.19%)`	⬆️
internal/planner/mapper/mapper.go	`88.88% <100.00%> (+0.21%)`	⬆️
internal/request/graphql/schema/manager.go	`98.63% <ø> (-0.04%)`	⬇️
internal/request/graphql/schema/types/base.go	`100.00% <ø> (ø)`
internal/request/graphql/schema/generate.go	`88.15% <92.31%> (+0.28%)`	⬆️
internal/planner/mapper/targetable.go	`76.84% <64.71%> (-2.65%)`	⬇️

... and 15 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 858f4f1...2975ef9. Read the comment docs.

jsimnz

Looking good!

Might need input from others, but I think now that there is a more expansive/expressive filter functionality for json types (even though it does share some implementation details with a regular document object) we should move and group all json related tests to its own section. Instead of adding it to the inline_array folder and the regular simple/with_filter folder.

AndrewSisley

Feature looks really good, but I have a handful of todos for you before merge.

AndrewSisley · 2024-10-09T14:46:29Z

internal/planner/mapper/mapper.go

 //
-// Return key will either be an int (field index), or a string (operator).
-func toFilterMap(
+// Return key will either be a property index, object property, or operator.


suggestion: It is not clear why it might return either a property index, or an object property - I think it is well worth explaining that an object property is a 'last resort' of sorts, and should typically only be returned for json object queries

AndrewSisley · 2024-10-09T14:47:09Z

internal/planner/mapper/mapper.go

+			Index: indexes[0],
+		}
+	} else {
+		// Key is an object property


nitpick: This comment does not add any value IMO and we'd be better off without it. (I can see that key is an object property, because the code says key = &ObjectProperty :))

AndrewSisley · 2024-10-09T14:49:48Z

internal/planner/mapper/mapper.go

 		default:
-			return key, sourceClause
+			returnClauses[i] = innerSourceClause


todo: This looks important to cover with integration tests.

I think this statement is unreachable. Only compound operators (_and, _not, _or) will call this function and their GQL types will already be validated before reaching the mapper.

Would you prefer for the type switch to be removed?

Compound operators are not supported on json? If not, why not, and is that documented in a user-visible location?

I'm referring to this line in general. It's called for all compound filter operations that contain an array, but this line in particular should never be called because the filter would have failed GQL validation.

Ah okay, I think I understand - in that case I'm probably in favour of removing the line

AndrewSisley · 2024-10-09T14:52:16Z

internal/planner/mapper/targetable.go

@@ -71,6 +72,31 @@ func (k *Operator) Equal(other connor.FilterKey) bool {
 	return false
 }

+// ObjectProperty is a FilterKey that represents a property in an object.


todo: Similar to another comment, I think it is important to document why both this and PropertyIndex exist, as outside of the scope of this PR it will look very strange and confusing.

should be more clear now

Looks good, thanks Keenan

AndrewSisley · 2024-10-09T15:07:13Z

internal/request/graphql/schema/generate.go

+		return fieldType.Name()
+
+	case isList && isNotNull:
+		return "NotNull" + fieldType.Name() + "ListOperatorBlock"


thought: It looks like we have a name clash here - I've created a ticket - would you mind adding a todo comment linking to the ticket, and then amend the issue-description to note taht there is a code-todo?

#3123

AndrewSisley · 2024-10-09T15:11:34Z

tests/integration/query/inline_array/with_filter_all_test.go

+					"custom": [null, false, "second", {"one": 1}, [1, 2]]
+				}`,
+			},
+			testUtils.Request{


todo: I'm sorry to ask because writing these is still a real pain, but would you mind adding some introspection tests? The query tests wont actually validate that the gql types are present and not broken.

I've added a basic introspection test. If there's more you'd like to see let me know.

Oh of course - sorry - there are no types for the input params. The test you added is a good one to have in general, but the thing I was worried about is not as relevant here as I initially thought.

It could be good to test that the json input type is working correctly though, have a look at TestFilterForSimpleSchema.

added another test

AndrewSisley · 2024-10-09T15:14:03Z

tests/integration/query/simple/with_filter/with_nlike_json_test.go

+			testUtils.CreateDoc{
+				DocMap: map[string]any{
+					"name":   "John",
+					"custom": "{\"tree\": \"maple\", \"age\": 250}",


todo: Please either document why this test appears to be storing a string for this test, or change the test so that it looks more obvious that the json value is a string.

Please apply the same to TestQuerySimple_WithNotLikeOpOnJSONFieldAllTypes_ShouldFilter as well.

changed to simple strings

AndrewSisley

Looks really good Keenan, thank you :)

AndrewSisley · 2024-10-11T17:49:58Z

internal/planner/mapper/mapper.go

+			// Object properties can never refer to mapped document fields.
+			// Set the mapping to null for any nested filter values so
+			// that we don't filter any fields outside of this object.
+			innerMapping = nil


praise: This comment is very helpful - thanks Keenan

AndrewSisley · 2024-10-11T17:56:09Z

tests/integration/schema/filter_test.go

+			testUtils.IntrospectionRequest{
+				Request: `
+					query {
+						__schema {


suggestion: You should be able to query __type with a name: "User" filter - it will save the test some processing, and should make it much easier to debug, as only the requested type will be returned from the query (instead of every gql type in the system).

TestSchemaSimple_WithJSONField_CreatesSchemaGivenType does this in your other introspection test.

end of cycle

islamaliev · 2024-10-16T08:43:50Z

Bug bash result: #3142

nasdf added 4 commits October 7, 2024 16:48

basic object filters working

75b75c5

refactor mapper filter code

6217bff

allow json arrays in all and any filters

27a4b12

add tests

60c42d8

nasdf added feature New feature or request area/query Related to the query component labels Oct 8, 2024

nasdf added this to the DefraDB v0.14 milestone Oct 8, 2024

nasdf self-assigned this Oct 8, 2024

nasdf requested a review from a team October 8, 2024 17:57

jsimnz previously requested changes Oct 9, 2024

View reviewed changes

AndrewSisley requested changes Oct 9, 2024

View reviewed changes

nasdf added 4 commits October 9, 2024 09:30

move json tests to subfolder

fdfebac

pr fixes

ab41829

add json explain filter test

48cddd8

add simple introspection test

1384e30

nasdf requested a review from AndrewSisley October 9, 2024 17:26

nasdf added 3 commits October 9, 2024 11:13

add json filter introspection test

1b6f3fa

add filter introspection test.

cd4fe32

fix panic in filter logic

3a4e006

nasdf force-pushed the nasdf/feat/json-type-filter branch from adb52b5 to 3a4e006 Compare October 10, 2024 22:33

AndrewSisley approved these changes Oct 11, 2024

View reviewed changes

Merge branch 'develop' into nasdf/feat/json-type-filter

2975ef9

nasdf merged commit aa6f235 into sourcenetwork:develop Oct 11, 2024
43 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: JSON type filter #3122

feat: JSON type filter #3122

nasdf commented Oct 8, 2024

codecov bot commented Oct 8, 2024 •

edited

Loading

jsimnz left a comment

AndrewSisley left a comment

AndrewSisley Oct 9, 2024

nasdf Oct 9, 2024

AndrewSisley Oct 9, 2024

nasdf Oct 9, 2024

AndrewSisley Oct 9, 2024

nasdf Oct 9, 2024

AndrewSisley Oct 9, 2024

nasdf Oct 9, 2024

AndrewSisley Oct 9, 2024

AndrewSisley Oct 9, 2024

nasdf Oct 9, 2024

AndrewSisley Oct 9, 2024

AndrewSisley Oct 9, 2024

nasdf Oct 9, 2024

AndrewSisley Oct 9, 2024

AndrewSisley Oct 9, 2024

nasdf Oct 9, 2024

AndrewSisley Oct 9, 2024

nasdf Oct 9, 2024

AndrewSisley Oct 9, 2024

nasdf Oct 9, 2024

AndrewSisley left a comment

AndrewSisley Oct 11, 2024

AndrewSisley Oct 11, 2024

islamaliev commented Oct 16, 2024

feat: JSON type filter #3122

feat: JSON type filter #3122

Conversation

nasdf commented Oct 8, 2024

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Oct 8, 2024 • edited Loading

Codecov Report

jsimnz left a comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndrewSisley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

islamaliev commented Oct 16, 2024

codecov bot commented Oct 8, 2024 •

edited

Loading