Skip to content

distinct sql and ignore field in filters and sorting #95

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

Merged
merged 6 commits into from
Mar 17, 2019

Conversation

miklemv
Copy link
Contributor

@miklemv miklemv commented Mar 11, 2019

Now by default, the sql request with the distinct. I think this is not very good, better distict = false.
I added argument distict to request and set default distict = false. The default mode can be changed static setter. I'm not sure that distinct is needed in arguments.
Example request "{ Books(distinct: true) { select { genre } }}" return 5 record. This is misleading.

Added @GraphQLIgnoreFilter - prohibiting the use of field in filters.
Added @GraphQLIgnoreOrder - prohibiting the use of the field for sorting.
Using all fields in filters and sorting is dangerous, because this can create an unexpected strong load on the database if the field does not have an index. It seems to me that it is better to list the fields explicitly for sorting and filtering, but this will change the current behavior.

@codecov
Copy link

codecov bot commented Mar 11, 2019

Codecov Report

Merging #95 into master will increase coverage by 0.24%.
The diff coverage is 71.42%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #95      +/-   ##
============================================
+ Coverage     65.44%   65.68%   +0.24%     
- Complexity      349      366      +17     
============================================
  Files            37       37              
  Lines          1962     2011      +49     
  Branches        291      296       +5     
============================================
+ Hits           1284     1321      +37     
- Misses          552      562      +10     
- Partials        126      128       +2
Impacted Files Coverage Δ Complexity Δ
...ventures/graphql/jpa/query/example/books/Book.java 0% <ø> (ø) 0 <0> (ø) ⬇️
.../query/example/books/BooksSchemaConfiguration.java 100% <100%> (ø) 4 <0> (ø) ⬇️
.../query/schema/impl/GraphQLJpaQueryDataFetcher.java 93.25% <62.5%> (-1.87%) 21 <1> (+1)
...query/autoconfigure/GraphQLJpaQueryProperties.java 80% <66.66%> (-5.72%) 9 <2> (+2)
...jpa/query/schema/impl/GraphQLJpaSchemaBuilder.java 86.43% <70.27%> (-1.32%) 103 <10> (+12)
...a/query/schema/impl/QraphQLJpaBaseDataFetcher.java 72.05% <0%> (+0.88%) 111% <0%> (+2%) ⬆️

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 4c8bf31...ac2ef24. Read the comment docs.

Copy link
Collaborator

@igdianov igdianov left a comment

Choose a reason for hiding this comment

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

Let's configure default distinct with GraphJPASchemaBuilder method using Spring Boot configuration properties in starter, so folks can customize it via application properties or Java configuration.

@igdianov
Copy link
Collaborator

@miklemv There is a rather big change in #96 that will cause conflicts with this PR. Please, wait making any changes until I merge it into master tomorrow. Then, you can rebase your PR to resolve conflicts.

@miklemv
Copy link
Contributor Author

miklemv commented Mar 12, 2019

path: graphql - will clean
graphql-jpa-query-example-merge/pom.xml - dublicate dependency )

I create options in GraphQLJpaQueryProperties and GraphQLSchemaBuilder config methods.

@miklemv miklemv force-pushed the filter_and_distinct branch from aecc2b9 to ac2ef24 Compare March 14, 2019 17:51
@miklemv
Copy link
Contributor Author

miklemv commented Mar 14, 2019

rebase branch and fix ignore field in sub filterers.

Copy link
Collaborator

@igdianov igdianov left a comment

Choose a reason for hiding this comment

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

@miklemv Looks very good, but I think it makes more sense to put distinct argument inside select field. It is more logical place. I also wonder if the distinct argument will support variable bindings?

@miklemv
Copy link
Contributor Author

miklemv commented Mar 15, 2019

@igdianov
I think that this parameter is not needed at the graphQL schea level. The
{ Books(distinct: true) { select { genre } }}
will not return 2 and 5 records, since the id field is involved in the sql query. Function SetDistinctFetcher at the developer level is enough.
Now it's difficult to make a distinct for the graphQL fields and I think this is not very useful functionality.

@igdianov
Copy link
Collaborator

@miklemv Sounds good! We can always refactor later if needed. Thank you!

@igdianov igdianov merged commit bde04d5 into introproventures:master Mar 17, 2019
@miklemv miklemv deleted the filter_and_distinct branch April 21, 2019 13:09
@miklemv miklemv restored the filter_and_distinct branch April 21, 2019 13:09
@chanhengseang3
Copy link
Contributor

chanhengseang3 commented Apr 23, 2019

I think distinct parameter does not work correctly:
image
Is there any known issues about this?

@chanhengseang3
Copy link
Contributor

image

@miklemv
Copy link
Contributor Author

miklemv commented Apr 24, 2019

@chanhengseang3
Yes. I I talked about it
#95 (comment)

Thish problem is hibernate level. I don'k know how distinc operation with models by fields.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants