Skip to content

Conversation

@kquick
Copy link
Contributor

@kquick kquick commented Oct 8, 2019

Prior to this change, the graphql-api internals could generate errors
but the user-supplied Handler could not. This adds the ability for
the user-supplied Handler to also return an error.

  1. Added a HandlerError constructor to the ResolverError to represent
    errors generated by the handler.

  2. Updated the resolve to return a 'HandlerResult a' which is now a
    typedef for 'Either Text a'. This could not be done as a 'Union'
    type because the 'Left err' value should have a different effect on
    the results (null in 'data' and adding to 'errors').

  3. Updated all the HasResolver class instances for the change in WIP: Basic servant like api #2.

  4. Added 'returns' and 'handlerError' convenience functions for
    handlers to abstract the internals of the success/failure
    implementation.

  5. Enhanced the tests (particularly ResolverSpec) for testing the
    generation of success and error results for different handler
    types.

This change is not backward compatible---and cannot be because the
handlers now need the ability to signal errors. However, all existing
handlers which perforce return successful values can be updated rather
simply using the new 'returns' helper for each handler field instead
of the previous 'pure' function:

miniCat name = pure (pure name :<> pure 32)

becomes:

miniCat name = pure (returns name :<> returns 32)

And:

handler :: Handler IO Query
handler = pure $ \fooId -> do
   foo <- lookupFoo fooId
   -- note that fmap maps over the Maybe, so we still need
   -- have to wrap the result in a pure.
   sequence $ fmap (pure . viewFoo) foo

becomes:

handler :: Handler IO Query
handler = pure $ \fooId -> do
   foo <- lookupFoo fooId
   returns $ viewFoo <$> foo

The current reported package version of 0.3.0 should be changed to at
least 0.4.0 to indicate a breaking, non-backward-compatible change.

Prior to this change, the graphql-api internals could generate errors
but the user-supplied Handler could not.  This adds the ability for
the user-supplied Handler to also return an error.

1. Added a HandlerError constructor to the ResolverError to represent
   errors generated by the handler.

2. Updated the resolve to return a 'HandlerResult a' which is now a
   typedef for 'Either Text a'.  This could not be done as a 'Union'
   type because the 'Left err' value should have a different effect on
   the results (null in 'data' and adding to 'errors').

3. Updated all the HasResolver class instances for the change in haskell-graphql#2.

4. Added 'returns' and 'handlerError' convenience functions for
   handlers to abstract the internals of the success/failure
   implementation.

5. Enhanced the tests (particularly ResolverSpec) for testing the
   generation of success and error results for different handler
   types.

This change is *not* backward compatible---and cannot be because the
handlers now need the ability to signal errors.  However, all existing
handlers which perforce return successful values can be updated rather
simply using the new 'returns' helper for each handler field instead
of the previous 'pure' function:

    miniCat name = pure (pure name :<> pure 32)

becomes:

    miniCat name = pure (returns name :<> returns 32)

And:

    handler :: Handler IO Query
    handler = pure $ \fooId -> do
       foo <- lookupFoo fooId
       -- note that fmap maps over the Maybe, so we still need
       -- have to wrap the result in a pure.
       sequence $ fmap (pure . viewFoo) foo

becomes:

    handler :: Handler IO Query
    handler = pure $ \fooId -> do
       foo <- lookupFoo fooId
       returns $ viewFoo <$> foo

The current reported package version of 0.3.0 should be changed to at
least 0.4.0 to indicate a breaking, non-backward-compatible change.
@kquick
Copy link
Contributor Author

kquick commented Oct 8, 2019

Addresses issue #172, largely using the method recommended by @harendra-kumar.

@kquick
Copy link
Contributor Author

kquick commented Oct 8, 2019

Also note that this is still not completely compliant with the GraphQL specification, which states:

If the field which experienced an error was declared as Non-Null, the null result will bubble up to the next nullable field. In that case, the path for the error should include the full path to the result field where the error occurred, even if that field is not present in the response.

This is not currently supported by the existed per-resolver error handling (e.g. throwE); this pull request does not change the current behavior (it's no better than the previous behavior, but no worse either); this pull request should not conflict with whatever approach is chosen to resolve this discrepancy in the future.

@teh
Copy link
Collaborator

teh commented Oct 13, 2019

Thank you, I think this is a great addition!

The test failure is a simple test ratchet change in this file: https://github.com/haskell-graphql/graphql-api/blob/master/scripts/hpc-ratchet#L37

alternatives: WORSE (150 missing => 151 missing)
booleans: OK
expressions: WORSE (1347 missing => 1351 missing)

@teh teh merged commit ecad3ef into haskell-graphql:master Oct 14, 2019
@kquick
Copy link
Contributor Author

kquick commented Oct 14, 2019

Thanks, @teh! Are there any plans for a hackage release in the near future?

@teh
Copy link
Collaborator

teh commented Oct 15, 2019

Hey, I pushed http://hackage.haskell.org/package/graphql-api-0.4.0 - please test and let me know if you have any issues!

@kquick
Copy link
Contributor Author

kquick commented Oct 15, 2019

@teh, the hackage release works great, thanks for pushing that out!

You might want to close #172 as well.

cjduncana pushed a commit to cjduncana/graphql-api that referenced this pull request Nov 23, 2019
With the merged pull request haskell-graphql#224, version 0.4.0 allows handlers to return errors. This change requires the use of a different function in the handler. This file change updates the README.md file to reflect this updated version.
teh added a commit that referenced this pull request Nov 23, 2019
Update README.md with changes from #224
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