mickhansen / graphql-sequelize

GraphQL & Relay for MySQL & Postgres via Sequelize
MIT License
1.9k stars 172 forks source link

Fix issue in argsToFindOptions #627

Closed joewoodhouse closed 5 years ago

joewoodhouse commented 5 years ago

In my database it's common for us to have a column called "order" for specifying the relative order of things.

When doing a query such as

{
  objects(order: "id"){
    id,
    order
  }
}

when using defaultListArgs(), the resulting findOptions are currently generated as

{
  where: { order: 'id' },
  order: [ [ 'id', 'ASC' ] ]
}

this results in the wrong query (I did not ask to filter by order: 'id') and will most likely generate an SQL syntax error e.g. invalid input syntax for integer: "id"

The fix changes argsToFindOptions to handle the special cases of offset,limit and order first, and then assessing the targetAttributes.

codecov-io commented 5 years ago

Codecov Report

Merging #627 into master will decrease coverage by 0.24%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #627      +/-   ##
==========================================
- Coverage   91.48%   91.24%   -0.25%     
==========================================
  Files          13       13              
  Lines         411      411              
==========================================
- Hits          376      375       -1     
- Misses         35       36       +1
Impacted Files Coverage Δ
src/argsToFindOptions.js 94.73% <100%> (ø) :arrow_up:
src/relay.js 96.37% <0%> (-0.73%) :arrow_down:

Continue to review full report at Codecov.

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

joewoodhouse commented 5 years ago

@janmeier agreed, just pushed new tests up for that.

Any ideas what's going on with the travis-ci tests? I seem to have made them fail but I've no idea if it's due to my change. Failures like:

AssertionError: expected '0.7163867175254655' to equal '0.6084338268942417'

don't seem to relate at all to my change. I see they fail quite regularly on other branches too, are they reliable?

xsbchen commented 5 years ago

@mickhansen can merge this pr?

mickhansen commented 5 years ago

v9.0.3