Skip to content

Implementation calculations fields #83

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

Closed
wants to merge 4 commits into from

Conversation

miklemv
Copy link
Contributor

@miklemv miklemv commented Mar 4, 2019

Good day.
Our team needs to make computable fields in graphQL queries: calculations that are not implemented with the Formula instruction. I propose a revision to implement this functionality:

  • Annotation GraphQLCalcField for a computable field.
  • The CashGraphQLCalcFields cache class contains a list of computable jpa fields of objects.
  • CalcEntity is an example of an entity.
  • CalcEntityTests class of tests for the current refinement.

Also our team is working on the implementation of mutations for jpa, but it is still in testing.

@codecov
Copy link

codecov bot commented Mar 4, 2019

Codecov Report

Merging #83 into master will increase coverage by 1.11%.
The diff coverage is 93.84%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #83      +/-   ##
============================================
+ Coverage      60.4%   61.52%   +1.11%     
- Complexity      283      313      +30     
============================================
  Files            31       32       +1     
  Lines          1816     1879      +63     
  Branches        282      287       +5     
============================================
+ Hits           1097     1156      +59     
- Misses          593      597       +4     
  Partials        126      126
Impacted Files Coverage Δ Complexity Δ
...a/query/schema/impl/QraphQLJpaBaseDataFetcher.java 69.43% <100%> (ø) 95 <0> (+2) ⬆️
...jpa/query/schema/impl/GraphQLJpaSchemaBuilder.java 86.95% <100%> (+1%) 80 <10> (+10) ⬆️
...l/jpa/query/schema/impl/CashGraphQLCalcFields.java 86.66% <86.66%> (ø) 18 <18> (?)

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 ac75192...f261aed. Read the comment docs.

@@ -146,7 +137,7 @@ public Object get(DataFetchingEnvironment environment) {
Field selectedField = (Field) selection;

// "__typename" is part of the graphql introspection spec and has to be ignored by jpa
if(!TYPENAME.equals(selectedField.getName())) {
if(!TYPENAME.equals(selectedField.getName()) && !CashGraphQLCalcFields.isCalcField(from.getJavaType(), selectedField.getName())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You could use JPA @Transient annotation to ignore calculated fields in queries.

@igdianov
Copy link
Collaborator

igdianov commented Mar 4, 2019

@miklemv Thank you for contribution! The library is definitely missing transient calculated fields support. Aside from suggestions to improve class names, I wonder why not use JPA's @Transient annotation to implement calculated fields support without introducing extra @GraphQLCalcField annotation?

@miklemv
Copy link
Contributor Author

miklemv commented Mar 5, 2019

I thought that specifying GraphQLCalculatedField would be more understandable. But this solution contains more code. I think we can use @transient while the logic of the work will remain the same. If @transient is more preferable - I can modify the code.

@igdianov
Copy link
Collaborator

igdianov commented Mar 5, 2019

@miklemv Less code sounds good! 👍 I think it makes sense to use existing JPA @Transient to implicitly create calculated field unless, you want to disable it using @GraphQLDisabled annotation.

@miklemv
Copy link
Contributor Author

miklemv commented Mar 6, 2019

I changed the logic, now we look at @transient.

@igdianov
Copy link
Collaborator

igdianov commented Mar 6, 2019

@miklemv Can you rebase your PR on top of master to resolve merge conflicts and trigger Travis CI? Thank you!

@miklemv miklemv closed this Mar 7, 2019
@miklemv miklemv deleted the calc_fields branch March 7, 2019 08:55
@miklemv
Copy link
Contributor Author

miklemv commented Mar 7, 2019

I recreate branch.
New pull request #85

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.

2 participants