Skip to content
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

fix(GraphQL): Added error for case when multiple filter functions are used in filter. #7368

Merged
merged 3 commits into from
Jan 27, 2021

Conversation

JatinDev543
Copy link
Contributor

@JatinDev543 JatinDev543 commented Jan 27, 2021

we were having indeterministic behavior in the filter when we have multiple filter functions as below .

query {
  queryFoo(filter:{value:{eq:2 le:2}})
     {
      value
    }
  }
  

It was because we were selecting the first filter function from the list of functions whose order is not guaranteed.
Now we are returning an error for this.


This change is Reviewable

@github-actions github-actions bot added the area/graphql Issues related to GraphQL support on Dgraph. label Jan 27, 2021
Copy link
Contributor

@abhimanyusinghgaur abhimanyusinghgaur left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @jatindevdg and @pawanrawal)


graphql/schema/validation_rules.go, line 28 at r1 (raw file):

var allowedFilters = []string{"StringHashFilter", "StringExactFilter", "StringFullTextFilter",
	"StringRegExpFilter", "StringTermFilter", "DateTimeFilter", "FloatFilter", "Int64Filter", "IntFilter"}

we should also add these filters to this list:

PointGeoFilter, ContainsFilter, IntersectsFilter, PolygonGeoFilter

graphql/schema/validation_rules.go, line 59 at r1 (raw file):

 value.ExpectedType == nil

don't see this being used, can we remove this check?


graphql/schema/validation_rules.go, line 64 at r1 (raw file):

Multiple filter functions for %s not allowed
%s filter expects only one filter function, got: %d

graphql/schema/validation_rules.go, line 183 at r1 (raw file):

Quoted 8 lines of code…
func contains(s []string, str string) bool {
	for _, v := range s {
		if v == str {
			return true
		}
	}
	return false
}

we can use x.HasString() instead of defining this

Copy link
Contributor Author

@JatinDev543 JatinDev543 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 0 of 3 files reviewed, 4 unresolved discussions (waiting on @abhimanyusinghgaur and @pawanrawal)


graphql/schema/validation_rules.go, line 28 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…

we should also add these filters to this list:

PointGeoFilter, ContainsFilter, IntersectsFilter, PolygonGeoFilter

done.


graphql/schema/validation_rules.go, line 59 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
 value.ExpectedType == nil

don't see this being used, can we remove this check?

removed.


graphql/schema/validation_rules.go, line 64 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
Multiple filter functions for %s not allowed
%s filter expects only one filter function, got: %d

changed.


graphql/schema/validation_rules.go, line 183 at r1 (raw file):

Previously, abhimanyusinghgaur (Abhimanyu Singh Gaur) wrote…
func contains(s []string, str string) bool {
	for _, v := range s {
		if v == str {
			return true
		}
	}
	return false
}

we can use x.HasString() instead of defining this

changed.

@JatinDev543 JatinDev543 merged commit de2f4ab into master Jan 27, 2021
@JatinDev543 JatinDev543 deleted the jatin/GRAPHQL-954 branch January 27, 2021 16:36
JatinDev543 added a commit that referenced this pull request Feb 2, 2021
… used in filter. (#7368)

we were having indeterministic behavior in the filter when we have multiple filter functions as below .

query {
  queryFoo(filter:{value:{eq:2 le:2}})
     {
      value
    }
  }

It was because we were selecting the first filter function from the list of functions whose order is not guaranteed.
Now we are returning an error for this.

(cherry picked from commit de2f4ab)
JatinDev543 added a commit that referenced this pull request Feb 2, 2021
… used in filter. (#7368) (#7384)

* fix(GraphQL): Added error for case when multiple filter functions are used in filter. (#7368)

we were having indeterministic behavior in the filter when we have multiple filter functions as below .

query {
  queryFoo(filter:{value:{eq:2 le:2}})
     {
      value
    }
  }

It was because we were selecting the first filter function from the list of functions whose order is not guaranteed.
Now we are returning an error for this.

(cherry picked from commit de2f4ab)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/graphql Issues related to GraphQL support on Dgraph.
Development

Successfully merging this pull request may close these issues.

2 participants