-
Notifications
You must be signed in to change notification settings - Fork 67
enhancement: add filtering to GraphQL API queries #721
enhancement: add filtering to GraphQL API queries #721
Conversation
09d0762
to
88bb290
Compare
94f4aba
to
b6dc578
Compare
b6dc578
to
7605c11
Compare
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.
- Fine to merge into the feature branch
- As mentioned, I don't think we'll be able to really see if it works until we just start using it slash open it up
- Just left comments regarding needing Rust docs and comments, as I think this GraphQL stuff has the most business logic of any part of the codebase (e.g., business logic being something that's not self documenting like everything else).
*elem = format!("{elem} AND {join_condition}") | ||
} | ||
} else { | ||
joins.push(format!( |
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.
Would it be possible to abstract away some of this raw string usage? E.g.,
struct Join {
rk_table: String,
rk_col: String,
pk_table: String,
pk_col: String,
is_the_condition: bool,
}
impl ToString for Join {
fn to_string(&self) -> String {
if self.is_the_condition {
format!("{{}.{} = {}.{}", self.rk_table, self.rk_col, self.pk_table, self.pk_col)
} else {
// So on and so forth
}
}
}
I think making some of this a bit more Rust-y would make it easier to follow without changing any functionality/logic . Totally fine if not, just thinking how to make it a bit more readable
elements_string, self.namespace_identifier, self.entity_name, | ||
); | ||
|
||
if !joins.is_empty() { |
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.
All of this business logic here is gonna need a comment as it's not clear what's going on
} | ||
} | ||
|
||
// Set a nested JSON object as the value for this entity field. |
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.
I don't think this comment helps me understand what this logic is doing
} | ||
} | ||
|
||
fn parse_query_elements(&self, db_type: &DbType) -> Vec<String> { |
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.
It would be helpful when you add rust-doc comments to explain what strategy you're using to parse these query elements. The more info the better
} | ||
|
||
impl UserQuery { | ||
pub fn to_sql(&mut self, db_type: &DbType) -> String { |
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.
When you add rust-docs it'd be nice to know what the overall strategy is here for this to_sql
method. The more info the better.
elements | ||
} | ||
|
||
fn get_topologically_sorted_joins(&mut self) -> Vec<JoinCondition> { |
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.
Could use a helpful high-level rust doc comment here as well. The more info the better
Noted on the comments. Look out for a small refactoring/commenting PR to make this clearer. |
Add filtering functionality to GraphQL API; add E2E tests
Add filtering functionality to GraphQL API; add E2E tests
Add filtering functionality to GraphQL API; add E2E tests
Add filtering functionality to GraphQL API; add E2E tests
#763) * enhancement: add filtering to GraphQL API queries (#721) Add filtering functionality to GraphQL API; add E2E tests * enhancement: add sorting and offset-based pagination to GraphQL API queries (#755) * Add comments; rename to clearer variables * Add sorting and offset-based pagination * enhancement: add `PageInfo` object to paginated GraphQL responses (#761) * Add PageInfo object to queries with a limit * Adjust tests * Change API response to snake_case * docs: add docs for GraphQL filtering and pagination (#768) * Add docs for GraphQL filtering and pagination * Move QueryResponse to models module * Address rvmelkonian feedback * Address ra0x3 feedback * rebase origin/master * Add fuel-indexer-graphql(-parser) packages * Fix index plugins failure * Clippy fixes --------- Co-authored-by: Alexander <alex@deekerno.com> Co-authored-by: Rashad Alston <rashad@Rashads-Air.lan>
Closes #637.
Changelog
arguments
module tofuel-indexer-schema::db
; this is where all functionality for GraphQL query arguments should be locatedpub fn parse_arguments(..)
; this is the entry point for parsing any query args--
todo!()
stubs have been added for sorting and pagination--
id
: select a record by its ID--
and
/or
: combine two filters together--
not
: negate the behavior of a filter--
has
: select records that have non-null values for columns in list--
in
: select records that have a value matching an element in set--
gt
/gte
/lt
/lte
(greater than, etc.): select records whose value satisfies the comparison condition--
between
: select records whose value falls between the bounds of the comparison conditionfuel-indexer-schema/db/lib.rs
tofuel-indexer-schema/db/queries.rs
GraphQLError
Testing Plan
Added several new tests that check each element in a set of results after filtering as well as ensuring that there are no extra elements that do not satisfy the filter conditions. CI should pass.
Manual Testing
http://localhost:29987/api/playground/fuel_examples/explorer_indexer
.Notes
Documentation updates will come in a separate PR so that this one doesn't get even bigger.
This implementation uses a fork of
graphql-parser
. The original version of the crate does not allow for parsing numbers greater thani64::MAX
(as its internal number type is hard-coded to ani64
); we allow for storingu64
andu128
, so we need to figure out a way to parseu64
s andu128
s from valid GraphQL queries. As such, I've forkedgraphql-parser
and added the necessary functionality; you can find the fork here and the diff for the changes here; it should be noted that the original crate has not released a new version in quite some time so there should be minimal burden from upstream changes.