graphql-platform icon indicating copy to clipboard operation
graphql-platform copied to clipboard

Subscriptions: subscriber and resolver parameters mismatch fails silently

Open cts-tradeit opened this issue 3 years ago • 4 comments

Is there an existing issue for this?

  • [X] I have searched the existing issues

Describe the bug

This example

public async IAsyncEnumerable<Item> SubItems(long id, [Service] IEventTarget eventTarget) {
       await foreach(Event<Item> @event in eventTarget.PullEvents<Event<Item>>()) { 
            if (@event.Entity.Id != id) continue; 
            yield return @event.Entitiy;
       }
}

[Subscribe(With = nameof(SubItems))]
public Item  OnItem([Message] Item item) {
       return item;
}

produces this schema

type Subscriptions {
    onItem: Item!;
}

As we can see, signature for node onItem is reived from method OnItem. As this method is parameterless so is the node. However subscribe function SubItems requires a parameter. If we run the code (breakpoint on await foreach line), as soon as someone open a channel we see that we did not break there. In this case HC silently fails.

If we adjust second method to look like

[Subscribe(With = nameof(SubItems))]
public Item  OnItem(long id, [Message] Item item) {
       return item;
}

schema becomes

type Subscriptions {
    onItem(id: Long!): Item!;
}

and as soon as channel opens we break on breakpoint from above. If we passed for example 8, we can observer that id in boh methods holds an 8. In this case HC works

If we change name of the parameter id of one to notId, we no longer break there and again HC silently fails. So do mismatches in parameter types, but only and only if the parameters types (and values) are not compatible (for example int and long work together but only if the value provided is not outside int range so value 1844674407370955161 fails silently).

Steps to reproduce

  1. Above

Relevant log output

No response

Additional Context?

No response

Product

Hot Chocolate

Version

12.9.0

cts-tradeit avatar Aug 10 '22 11:08 cts-tradeit

Other question is whether requirement to duplicate parameters on each method is not bug as well. Resolver method does not do anything with the id, so it should not be necessary to pass it there. I think that node's signature should be created by merging both methods' signatures and distributing values accordingly.

cts-tradeit avatar Aug 10 '22 11:08 cts-tradeit

Its actually not a bug ... since the resolver is what defines the field. the factory can just also access the arguments. But we are changing this with 13.

michaelstaib avatar Aug 10 '22 15:08 michaelstaib

Ok but I do think that it should not fail silently eve in 12. Either it should throw at startup or during invocation.

cts-tradeit avatar Aug 10 '22 20:08 cts-tradeit

It's not so simple to achieve this since if we could do that we could infer the and merge. So, for now it is as it is and works in its limitations. For V13 this will change this. For V12 there will be no change... the gap between 12 and 13 is huge on subscriptions.

But if you want you can add a note to the docs outlining this limitation.

michaelstaib avatar Aug 10 '22 21:08 michaelstaib