Possible restrictions needed related to NULLABLE(DESC)

[potatosalad]

When running some property based tests, I came across some interesting counterexamples related to NULLABLE(DESC):

DESC?

{
  "type": "NULLABLE",
  "of": {
    "type": "DESC"
  }
}

If an implementation happens to have the value represented like:

NON_NULL(DESC(NULL))

The resulting encoding could be either 0D 00 01 (inline-everything) or 0F 01 (inline-everything, self-describing).

When decoding the first example, the resulting value would still be:

NON_NULL(DESC(NULL))

However, when decoding the second example, the resulting value would be:

NULL

Similar issues occur with nested NULLABLE(NULLABLE(... DESC)).

Possible Solutions

1. Since DESC can represent NULL internally, should NULLABLE(DESC) be illegal?
2. Similarly, should NULLABLE(NULLABLE(...)) (nested NULLABLE) also be illegal according to the spec?

I realize this is an edge case 😄 Never would have found it without the fuzz-testing aspects of the property based tests.

[msolomon]

That's a bit annoying. Fortunately, I don't believe these any of these types can actually appear in types derived from a GraphQL schema.

I think there are only 3 ways a self-describing value may appear:

1. In SelfDescribing Mode, Core is written as a self-describing value
2. In SelfDescribingErrors Mode, every Error value is written as a self-describing value
3. The extensions field of each Error value is written as a self-describing value. This field is nullable, and must be an object (due to the GraphQL spec)

Even the @ArgoCodec annotation does not allow self-describing values.

It's also true that the Wire types themselves are unsound, and allow cases like NULLABLE(NULLABLE(...)) which it cannot actually encode. It might be neat to fix that, but today it relies on the soundness of the GraphQL type system.

Do you know of any counterexamples, such that this can happen from a real schema (and any combination of Modes)?

[potatosalad]

Do you know of any counterexamples, such that this can happen from a real schema (and any combination of Modes)?

Yes, but it's unrelated to the original NULLABLE issue described here. There's an issue with the argo-js reference implementation (and possibly the spec needs to be updated) when it relates to field selection merging.

I'll file a quick PR to demonstrate the issue in the tests.

[potatosalad]

I filed #19 which demonstrates the field selection merging issue.

[jbourassa]

Even the @ArgoCodec annotation does not allow self-describing values.

[...]

Do you know of any counterexamples, such that this can happen from a real schema (and any combination of Modes)?

It is not yet a real-world example, but I'm planning to suggest a change for adding DESC to the ArgoCodecType enum. We'd want to use self-describing types as the Argo codec for Shopify's JSON scalar.

[msolomon]

Ok, good to know about wanting DESC in @ArgoCodec. Unfortunately, I suspect this will require a breaking change to self-describing types. Is that likely to be a problem for either of you at this juncture?

(Will tackle field merging issues in #19)

[potatosalad]

Ok, good to know about wanting DESC in @ArgoCodec. Unfortunately, I suspect this will require a breaking change to self-describing types.

What breaking changes were you thinking might be needed?

Is that likely to be a problem for either of you at this juncture?

Soon, it'll be more a problem if there's major breaking changes, but right now it won't hurt anything major for us.

[jbourassa]

Is that likely to be a problem for either of you at this juncture?

Not yet a problem for us 👍.

[msolomon]

Ok, thanks.

What breaking changes were you thinking might be needed?

I haven't worked out the details yet, but I believe there are ambiguities in how self-describing values are represented in the binary when mixed with regular values. This is fine if they don't mix in user-specified ways, but this would change that.

[msolomon]

I added DESC (self-describing values) to @ArgoCodec for version 1.2 (just released). It worked fine, I added a couple of test cases.

It does have the strange quirk that jsonField: JSON! permits null as a value, but I believe that is correct behavior--null is a valid JSON value). It doesn't give you a way to distinguish the source of null in a nullable field like jsonField: JSON, but I think that is ok. I think the same reasoning applies to NON_NULL(DESC(null)) yielding null as a value--null is a valid self-describing value.

As for the other issue described here, is there any way to get Argo to generate NULLABLE(NULLABLE(...)) from a GraphQL schema+query? I haven't been able to come up with one.

[jbourassa]

I added DESC (self-describing values) to @ArgoCodec for version 1.2 (just released)

That is awesome Mike, thank you so much! 🙇