fix: RecursiveTypeMapper returns exception which can't be handled by StandardServer

[fogrye]

RecursiveTypeMapper throws CannotMapTypeException when fails to find class by name. This exception goes through the GraphQl executor instead of catching and wrapping into ExecutionResult. Non wrapped exception does not trigger HttpCodeDecider. Schema test trust more to input than configuration which is not optimal, great possibility to improve configuration validation.

Fixes: #336

[oojacoboo]

Thanks for this @fogrye. That skipped test - can we get that resolved? What's the issue with it? I'm not seeing where it's using a request in that test either.

[fogrye]

@oojacoboo it meant to fail when test query request entity which isn't presented because of bad config but after fix stoped. Now doTestSchema throws unexpected exception because $result->toArray() returns array without data key but with errors.

I see there only option for rewrite: check that with bad config (like not existing folder) nothing happens or new exception been thrown.

[oojacoboo]

Can we still use CannotMapTypeException and set the Error from the createLocatedError as the previous exception value? Maybe by adding a new method like CannotMapTypeException::createFromLocatedError?

[fogrye]

@oojacoboo unfortunately no, GraphQL\Server\Helper is too strict about types.

[oojacoboo]

Why can't/shouldn't CannotMapTypeException just extend GraphQL\Error\Error?

[fogrye]

@oojacoboo I see the point in keeping everything clean and consistent but here is some problems:

1. If CannotMapTypeException becomes a child of Error all errors it constructs becomes "soft" and handled which is a big change.
2. All exceptions in CannotMapTypeException some way referred to dev's mistake while in case of RecursiveTypeMapper it's end of execution which deserves some "special attention".
3. Also CannotMapTypeException has nothing to take from it's possible parent and this extend just a bad inheritance.

Of course it's up to you to decide about such things as I'm newbie here but I would wish to clean up mappings a little including rethinking about how they trigger errors. Like, as an example, currently there is no way to throw error with reference to place in gql request because mapper which triggers them know nothing about which node it's processing.

[oojacoboo]

Good points @fogrye. Let's find a way to preserve that test. Maybe it makes sense to create a new Exception class for this scenario, then we can expect that Exception in that test commented out?

I agree that some improvements could be made on the mappings. If you want to submit a PR that takes a shot at this, that'd certainly be appreciated.

[oojacoboo]

@fogrye just a reminder on this. It'd be nice to get this merged in before it gets stale. We need tests though.

[fogrye]

@oojacoboo had a busy time lately, will try to make it for tests soon.

[oojacoboo]

@fogrye thanks for this - couple small code style changes and I think it's ready to merge :)