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.

Sign up for GitHub

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

Jump to bottom

Allow data and errors to be returned with DataFetcherResult #342

Merged
merged 4 commits into from Sep 13, 2019
Merged

Allow data and errors to be returned with DataFetcherResult #342

merged 4 commits into from Sep 13, 2019

Conversation

smyrick
Copy link
Contributor

@smyrick smyrick commented Sep 11, 2019 •
edited

📝 Description

Kotlin functions can now return a DataFetcherResult instead of just their return type which allows you to modify the errors field with any extra data you need

Running the spring example with new function
Screen Shot 2019-09-11 at 12 49 33 PM

🔗 Related Issues

Fixes #244
Rebase of #245

Allow data and errors to be returned with DataFetcherResult
c5148b0 
Fixes ExpediaGroup#244
Rebase of ExpediaGroup#245

Kotlin functions can now return a DataFetcherResult instead of just their return type which allows you to modify the errors field with any extra data you need
@smyrick smyrick added type: enhancement New feature or request changes: minor Changes require a minor version labels Sep 11, 2019
@codecov-io
Copy link

codecov-io commented Sep 11, 2019 •
edited

Codecov Report

Merging #342 into master will decrease coverage by 0.08%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #342      +/-   ##
============================================
- Coverage     96.82%   96.73%   -0.09%     
+ Complexity      278      275       -3     
============================================
  Files            81       82       +1     
  Lines           976      980       +4     
  Branches        180      182       +2     
============================================
+ Hits            945      948       +3     
  Misses            6        6              
- Partials         25       26       +1
Impacted Files Coverage Δ Complexity Δ
...up/graphql/generator/extensions/kTypeExtensions.kt 100% <100%> (ø) 0 <0> (ø) ⬇️
...aphql/generator/types/utils/functionReturnTypes.kt 100% <100%> (ø) 0 <0> (?)
...p/graphql/generator/extensions/kClassExtensions.kt 100% <100%> (ø) 0 <0> (ø) ⬇️
...iagroup/graphql/generator/types/FunctionBuilder.kt 100% <100%> (ø) 4 <0> (-3) ⬇️
...oup/graphql/federation/execution/EntityResolver.kt 82.6% <0%> (-4.35%) 2% <0%> (ø)

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 370d650...5c7a907. Read the comment docs.

@dariuszkuc
Copy link
Collaborator

Do we need to support monads?

@smyrick
Copy link
Contributor Author

smyrick commented Sep 11, 2019 •
edited

Do we need to support monads?

@dkuc84 Do you mean DataFetcherResult<CompletableFuture<String>>?

@dariuszkuc
Copy link
Collaborator

Do we need to support monads?

@dkuc84 Do you mean DataFetcherResult<CompletableFuture<String>>?

Yes + custom monads that can be configured (e.g. Rx/Reactor)

@smyrick
Copy link
Contributor Author

smyrick commented Sep 11, 2019

It does not work right now with DataFetcherResult<CompletableFuture<String>> so I will work on fixing that first

@smyrick smyrick added the status: do not merge Do not merge until this is removed label Sep 11, 2019
@dariuszkuc
Copy link
Collaborator

Just to clarify - we should support CompletableFuture<DataFetcherResult<String>> and not the other way around. We need monad wrapper to ensure asynchronous processing, if we do it the other way around it will be blocking.

@smyrick smyrick added status: do not merge Do not merge until this is removed and removed status: do not merge Do not merge until this is removed labels Sep 12, 2019
@smyrick smyrick removed the status: do not merge Do not merge until this is removed label Sep 12, 2019
@smyrick
Copy link
Contributor Author

smyrick commented Sep 12, 2019

New example with CompletableFuture<DataFetcherResult> and unit tests that validate the other way is not valid

Screen Shot 2019-09-12 at 3 02 18 PM

@dariuszkuc dariuszkuc merged commit c7d9ca3 into ExpediaGroup:master Sep 13, 2019
@smyrick smyrick deleted the fix-244 branch September 13, 2019 20:26
dariuszkuc pushed a commit to dariuszkuc/graphql-kotlin that referenced this pull request Aug 5, 2022
@smyrick @dariuszkuc
Allow data and errors to be returned with DataFetcherResult (ExpediaG…
2db374d 
…roup#342)

* Allow data and errors to be returned with DataFetcherResult

Fixes ExpediaGroup#244
Rebase of ExpediaGroup#245

Kotlin functions can now return a DataFetcherResult instead of just their return type which allows you to modify the errors field with any extra data you need

* Move output type monad code into the generator

* Move unwrapping logic back to function builder

* Add unit test for publisher implementation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dariuszkuc dariuszkuc dariuszkuc approved these changes

Assignees
No one assigned
Labels
changes: minor Changes require a minor version type: enhancement New feature or request
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Return both data and errors/partial response
3 participants
@smyrick @codecov-io @dariuszkuc