feat: Filter alias target #3201

nasdf · 2024-11-01T17:48:33Z

Relevant issue(s)

Resolves #3194

Description

This PR adds alias targeting in filters.

Aggregate targets are not included in this PR as they require more changes.

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 and updated integration tests.

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

MacOS

codecov · 2024-11-01T18:06:34Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 77.57%. Comparing base (2b0b862) to head (cfd3ada).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3201      +/-   ##
===========================================
+ Coverage    77.41%   77.57%   +0.15%     
===========================================
  Files          375      375              
  Lines        34756    34790      +34     
===========================================
+ Hits         26906    26986      +80     
+ Misses        6237     6203      -34     
+ Partials      1613     1601      -12

Flag	Coverage Δ
all-tests	`77.57% <100.00%> (+0.15%)`	⬆️

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

Files with missing lines	Coverage Δ
internal/connor/connor.go	`100.00% <100.00%> (ø)`
internal/connor/eq.go	`92.31% <100.00%> (+0.64%)`	⬆️
internal/core/doc.go	`94.62% <100.00%> (+0.26%)`	⬆️
internal/planner/filter/copy_field.go	`96.67% <100.00%> (+0.44%)`	⬆️
internal/planner/mapper/errors.go	`66.67% <100.00%> (+16.67%)`	⬆️
internal/planner/mapper/mapper.go	`87.35% <100.00%> (+0.14%)`	⬆️
internal/planner/mapper/targetable.go	`75.24% <100.00%> (-0.69%)`	⬇️
internal/request/graphql/parser/filter.go	`60.00% <100.00%> (ø)`
internal/request/graphql/schema/generate.go	`86.93% <100.00%> (+0.05%)`	⬆️

... and 17 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 2b0b862...cfd3ada. Read the comment docs.

AndrewSisley

Aggregate targets are not included in this PR as they require more changes.

question: I thought the aggregate filters were of the same type as normal filters, in which case they will now have a (presumably broken) alias property on them. Have you checked what happens when someone tries to use it, and when do you plan on adding support for it?

praise: Looks really good Keenan, just a couple of localised suggestions for you - will be really good to have this in - we've been talking about it for a long time now lol and the JSON-type solution is really nice IMO :)

AndrewSisley · 2024-11-01T19:13:05Z

internal/planner/mapper/targetable.go

@@ -85,6 +85,9 @@ func (k *ObjectProperty) GetProp(data any) any {
 	if data == nil {
 		return nil
 	}
+	if _, ok := data.(core.Doc); ok {


question: Why are you checking for this particular type?

suggestion: Depending on the answer to the above question, it might be better to instead check the success of the cast below:

object, ok := data.(map[string]any) if !ok { return nil // this can happen when an alias target does not exist }

In past reviews you had a preference for specific type checks instead of a single type assertion. I thought I could anticipate this one before review.

My preferences probably vary by context. Here you are casting to the type you actually want the line below, it doesn't seem to make sense to care about some other type as well.

AndrewSisley · 2024-11-01T19:13:50Z

internal/planner/mapper/mapper.go

@@ -992,6 +992,12 @@ func resolveInnerFilterDependencies(
 	newFields := []Requestable{}

 	for key, value := range source {
+		// alias fields are guarenteed to be resolved
+		// because they refer to existing fields


praise: Thanks for this explanation :)

AndrewSisley · 2024-11-01T19:15:27Z

internal/request/graphql/parser/filter.go

@@ -93,20 +93,20 @@ func parseFilterFieldsForDescriptionMap(
 	fields := make([]client.FieldDefinition, 0)
 	for k, v := range conditions {
 		switch k {
-		case "_or", "_and":
+		case request.FilterOpOr, request.FilterOpAnd:


praise: Thanks for these cleanups :)

AndrewSisley · 2024-11-01T19:16:38Z

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

+			},
+			testUtils.Request{
+				Request: `query {
+					Users(filter: {_alias: {}}) {


question: Is this consistent with the behaviour of other 'empty' filter properties?

Yes, there's no existing tests but I double checked

Nice, thanks Keenan :)

AndrewSisley · 2024-11-01T19:18:21Z

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

+			},
+			testUtils.Request{
+				Request: `query {
+					Users(filter: {_alias: null}) {


question: I thought we could make input params not-null without making them required?

suggestion: If so, please make the alias param not-null, and change this test so that it expects an error :)

I think we can have not null inputs but we need a default value. There will probably be a lot of other inputs that need a similar update.

Ah okay, must be a memory of the variable stuff then - thanks :)

AndrewSisley · 2024-11-01T19:19:23Z

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

+
+func TestQuerySimple_WithNonAliasedField_Succeeds(t *testing.T) {
+	test := testUtils.TestCase{
+		Description: "Simple query with non aliased filter",


praise: Thank you for adding this test, and the ones for the compound filters!

AndrewSisley · 2024-11-01T19:21:08Z

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

+			},
+			testUtils.Request{
+				Request: `query {
+					Users(filter: {_alias: {UserAge: {_eq: 21}}}) {


suggestion: Would it not be better to error in this case? This looks like it would mainly come about through a user-programming error, in which case they would likely much prefer an error over a silent failure.

I have a preference to not return errors from otherwise validly typed queries. I also don't think the added complexity of checking aliases would be worth the benefits.

The added complexity for us should pretty minimal no (might even be just be converting a continue to a return NewErrorFoo)? And the potential time/money savings for our users could be very large.

If you see a solution that simple I'd definitely consider it.

It would be really difficult to tell whether a filter property is targeting a relationship field or a nested JSON property without adding a good amount of new validation logic.

I found a kind of simple solution for this. Let me know what you think.

AndrewSisley · 2024-11-01T19:22:46Z

tests/integration/query/one_to_many/with_count_test.go

+				Doc: `{
+					"name": "Painted House",
+					"rating": 4.9,
+					"author_id": "bae-e1ea288f-09fa-55fa-b0b5-0ac8941ea35b"


todo: Please use DocIndex for these IDs instead of strings, it reduces the maintenance and readability overhead quite a bit IMO, it will also force the test to create the docs in a 'correct' order :)

islamaliev

It's a nice feature. Good job Keenan.
I left some minor suggestions/concerns.

internal/connor/connor.go

internal/planner/filter/copy_field.go

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

fredcarle · 2024-11-05T16:34:30Z

internal/connor/connor.go

@@ -62,7 +66,7 @@ func matchWith(op string, conditions, data any) (bool, error) {
 		return anyOp(conditions, data)
 	case AllOp:
 		return all(conditions, data)
-	case EqualOp:
+	case EqualOp, request.FilterOpAlias:


suggestion: I think it's fine to duplicate the const for _alias here and avoid the added dependency.

islamaliev

LGTM! Good work, Keenan.

AndrewSisley

LGTM - thanks Keenan :)

Remaining suggestion from me is very optional.

AndrewSisley · 2024-11-06T18:27:06Z

internal/planner/mapper/targetable.go

-
-func (k *ObjectProperty) GetOperatorOrDefault(defaultOp string) string {
-	return defaultOp
+	_, ok := data.(core.Doc)


suggestion: Made me smile a little to see you do this again in this PR - and I'm curious as to why/where you think I prefer this 😁 (are you thinking back to me preferring nil checks vs failed casting?)

My suggestion is the same as the last, code is simpler IMO, and avoids having to care about an extra, somewhat irrelevant type:

docMap, ok := data.(map[string]any) if !ok { return nil, defaultOp, NewErrFieldOrAliasNotFound(k.Name) } return docMap[k.Name], defaultOp, nil

I think I burned that one into my brain. Should be fixed now.

islamaliev · 2024-12-10T12:03:02Z

bug bash result: ran all tests from integration tests and many other related tests. Everything is good.

implement filter alias target

c4077d3

nasdf added the feature New feature or request label Nov 1, 2024

nasdf added this to the DefraDB v0.15 milestone Nov 1, 2024

nasdf self-assigned this Nov 1, 2024

nasdf requested a review from a team November 1, 2024 18:11

AndrewSisley requested changes Nov 1, 2024

View reviewed changes

islamaliev reviewed Nov 4, 2024

View reviewed changes

internal/connor/connor.go Outdated Show resolved Hide resolved

internal/planner/filter/copy_field.go Show resolved Hide resolved

tests/integration/query/simple/with_filter/with_alias_test.go Outdated Show resolved Hide resolved

fredcarle reviewed Nov 5, 2024

View reviewed changes

pr fixes

d71d1b1

islamaliev approved these changes Nov 5, 2024

View reviewed changes

refactor filter key prop logic

60de91b

nasdf requested a review from AndrewSisley November 6, 2024 17:06

AndrewSisley approved these changes Nov 6, 2024

View reviewed changes

nasdf and others added 2 commits November 6, 2024 11:35

pr fix

17702a6

Merge branch 'develop' into nasdf/feat/filter-alias-target

cfd3ada

nasdf merged commit c8fd3b1 into sourcenetwork:develop Nov 6, 2024
42 of 43 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Filter alias target #3201

feat: Filter alias target #3201

nasdf commented Nov 1, 2024

codecov bot commented Nov 1, 2024 •

edited

Loading

AndrewSisley left a comment

AndrewSisley Nov 1, 2024

nasdf Nov 5, 2024

AndrewSisley Nov 5, 2024

AndrewSisley Nov 1, 2024

AndrewSisley Nov 1, 2024

AndrewSisley Nov 1, 2024

nasdf Nov 5, 2024

AndrewSisley Nov 5, 2024

AndrewSisley Nov 1, 2024

nasdf Nov 4, 2024 •

edited

Loading

AndrewSisley Nov 4, 2024

AndrewSisley Nov 1, 2024

AndrewSisley Nov 1, 2024

nasdf Nov 5, 2024

AndrewSisley Nov 5, 2024

nasdf Nov 5, 2024

nasdf Nov 6, 2024

AndrewSisley Nov 1, 2024

nasdf Nov 5, 2024

islamaliev left a comment

fredcarle Nov 5, 2024

nasdf Nov 5, 2024

islamaliev left a comment

AndrewSisley left a comment

AndrewSisley Nov 6, 2024

nasdf Nov 6, 2024

islamaliev commented Dec 10, 2024

feat: Filter alias target #3201

feat: Filter alias target #3201

Conversation

nasdf commented Nov 1, 2024

Relevant issue(s)

Description

Tasks

How has this been tested?

codecov bot commented Nov 1, 2024 • edited Loading

Codecov Report

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

nasdf Nov 4, 2024 • edited Loading

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

islamaliev 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 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

islamaliev commented Dec 10, 2024

codecov bot commented Nov 1, 2024 •

edited

Loading

nasdf Nov 4, 2024 •

edited

Loading