Interface Issues

[swalker2m]

The query:

query {
  observation(observationId: "o-151") {
    execution {
      config(futureLimit: 100) {
        ... on GmosNorthExecutionConfig {
          static {
            stageMode
            detector
          }
        }
        ... on GmosSouthExecutionConfig {
          static {
            stageMode
            detector
          }
        }
      }
    }
  }
}

Produces:

[swalker2m]

In Grackle version 0.18.1 but not prior versions.

[swalker2m]

Check #1024 to see this problem in action.

[swalker2m]

Discussion with Miles indicates that the query / schema violate the spec and Grackle is now conforming to it whereas before it was not. The issue as I understand it is that two concrete implementations of an interface cannot ultimately have leaf fields with the same name and yet with different types. This despite the fact that the fields are not part of the interface itself. So in this case we have stageMode: GmosNorthStageMode! in GmosNorthStatic and stageMode: GmosSouthStageMode! in GmosSouthStatic, which don't even share an interface but appear in GmosNorthExecutionConfig and GmosSouthExecutionConfig both of which implement ExecutionConfig.

interface ExecutionConfig {
  instrument: Instrument!
}

type GmosNorthExecutionConfig implements ExecutionConfig {
  static: GmosNorthStatic!
  ...
}

type GmosSouthExecutionConfig implements ExecutionConfig {
  static: GmosSouthStatic!
  ...
}

type GmosNorthStatic {
  stageMode: GmosNorthStageMode!
  ...
}

type GmosSouthStatic {
  stageMode: GmosSouthStageMode!
  ...
}

[swalker2m]

To me this seems to render interfaces dangerously unusable. When you make a concrete interface implementation you have to check all other implementations and ensure that you've invented unique names for the paths which lead to leaves of differing types. (e.g., execution / config / static / stageMode).

[swalker2m]

So one solution would be to ban interfaces, at least where this is likely to be a problem as in the case of ExecutionConfig. If we do this, ExecutionConfig would become a type with optional fields, all but one of which will always be null.

type ExecutionConfig {
  gmosNorth: GmosNorthExecutionConfig
  gmosSouth: GmosSouthExecutionConfig
  ghost: GhostExecutionConfig
 ....
}

[toddburnside]

I agree that the spec seems to wrong in this case. There seems no good reason to require fields of the same name that aren't in the interface to be of the same type. Seems to violate normal expectations of interfaces.

[swalker2m]

In this particular case we could instead rename all the implementation-specific fields at the highest level and work around the issue, I think:

type GmosNorthExecutionConfig implements ExecutionConfig {
  gmosNorthStatic: GmosNorthStatic!
  gmosNorthAcquisition: GmosNorthExecutionSequence
  gmosNorthSequence: GmosNorthExecutionSequence
  instrument: Instrument!
}

instead of the original

type GmosNorthExecutionConfig implements ExecutionConfig {
  static: GmosNorthStatic!
  acquisition: GmosNorthExecutionSequence
  science: GmosNorthExecutionSequence
  instrument: Instrument!
}

that would make the paths down to stageMode distinct: execution / config / gmosNorthStatic / stageMode vs execution / config / gmosSouthStatic / stageMode.

[swalker2m]

Another solution, I think is to dump it on the client and require aliasing the field names. But that seems like it would lead naive API users (such as myself) to write what looks like a perfectly valid query as in the issue description. From the error it might be hard to see what is wrong.

[toddburnside]

In this particular case we could instead rename all the implementation-specific fields at the highest level and work around the issue, I think:

IMO, making unique names for all of the fields is uglier and hides some common intent. I don't think the interface provides enough usefulness in this case to warrant that.

[toddburnside]

Another solution, I think is to dump it on the client and require aliasing the field names. But that seems like it would lead naive API users (such as myself) to write what looks like a perfectly valid query as in the issue description. From the error it might be hard to see what is wrong.

This would be fairly easy to implement in our clients since the relevant query (for this instance) is in lucuma-schemas. Just need to adjust the queries and the decoders. But I agree that API users may find the error less than helpful.

query { observation(observationId: "o-106") { execution { config(futureLimit: 100) { ... on GmosNorthExecutionConfig { staticSouth: static { stageMode detector } } ... on GmosSouthExecutionConfig { staticNorth: static { stageMode detector } } } } } }

[rpiaggio]

I was going to suggest using union types, but then found out that they have the same restriction as interfaces.

Another solution, I think is to dump it on the client and require aliasing the field names. But that seems like it would lead naive API users (such as myself) to write what looks like a perfectly valid query as in the issue description. From the error it might be hard to see what is wrong.

I lean towards this solution. Can we provide better error messages?

[toddburnside]

I lean towards this solution. Can we provide better error messages?

I assume Miles could at least add to the message. Maybe suggest aliasing fields of the same name with different types? Maybe that would be sufficient for API users.

[swalker2m]

There are a number of other cases that will need to be adjusted similarly. I see at least Atom, Step, StepRecord, and Visit but there are other interfaces as well that we might want to either do away with or change the field names. ProposalClass seems like it could inadvertently evolve to have this problem.

[toddburnside]

I lean towards this solution. Can we provide better error messages?

I think with a better error message, putting the burden on the API users to provide unique names seems like it might be the best approach.

[milessabin]

Is there any possibility of aligning the field types?

[swalker2m]

Is there any possibility of aligning the field types?

Unfortunately no. There will be quite a few instruments with distinct filter enumerations for example. Or do you mean introduce a union of all the possibilities in each case? For this example:

union GmosStageMode = GmosNorthStageMode | GmosSouthStageMode

type GmosNorthStatic {
  stageMode: GmosStageMode!
  ...
}

type GmosSouthStatic {
  stageMode: GmosStageMode!
  ...
}

Still I think that's going to be cumbersome.

[rpiaggio]

There are a number of other cases that will need to be adjusted similarly. I see at least Atom, Step, StepRecord, and Visit but there are other interfaces as well that we might want to either do away with or change the field names. ProposalClass seems like it could inadvertently evolve to have this problem.

Most of the queries for these types are also in a single choke point in lucuma-schemas. For UI development, it just means fixing the queries and subqueries there and the decoders accordingly.

[milessabin]

Both GmosNorthStageMode and GmosSouthStageMode are scalars, right?

How did you choose which decoder to use in the response to the query at the top of this issue before the fix for the Grackle bug landed? I'm puzzled because whichever of GmosNorthExecutionConfig or GmosSouthExecutionConfig is returned from config the shape of the response json would be the same, so presumably you're able to detect the type based on details of the serialized form of the scalar? But if that's case then it seems that you ought to be able to construct an unambiguous scalar representation which encompasses both?

[milessabin]

Where I'm heading with the previous question is that I'm thinking that maybe the client in this context doesn't care about the precise type, and just needs a uniform String representation to display. In which case why not leave things as they are and just add,

type GmosNorthStatic {
  stageMode: GmosNorthStageMode!
  displayStageMode: String!
  ...
}

type GmosSouthStatic {
  stageMode: GmosSouthStageMode!
  displayStageMode: String!
  ...
}

As a added bonus that could live in the interface, so you might not need the inline fragments at all.

[milessabin]

If String is too horrible you could have,

scalar GmosNorthStageMode
scalar GmosSouthStageMode

scalar GmosDisplayStageMode

type GmosNorthStatic {
  stageMode: GmosNorthStageMode!
  displayStageMode: GmosDisplayStageMode!
  ...
}

type GmosSouthStatic {
  stageMode: GmosSouthStageMode!
  displayStageMode: GmosDisplayStageMode!
  ...
}

[toddburnside]

As a added bonus that could live in the interface, so you might not need the inline fragments at all.

Some instruments will have a stageMode, etc., others will not.

[swalker2m]

The stage modes are enumerations. Looking at them, they actually match (though they shouldn't) so this isn't the best example. We might be able to unify the static fields for GMOS but it won't solve the general problem.

[toddburnside]

How did you choose which decoder to use in the response to the query at the top of this issue before the fix for the Grackle bug landed?

Currently, we are decoding the Instrument first, then using that to determine how to decode the rest of it. The instrument is part of the interface. (There is more to the actual query we use than the snippet above.)

[toddburnside]

I presume for the GMOS north vs south sequences, with their FPUs, Filters etc. we must include a __typename? Or do we just try decoding it as GMOS north and then south, until one matches?

It's all part of the same query, so we base it on the instrument in the config.

[swalker2m]

I think I may be coming back to the conclusion that interfaces are just too dangerous to be used, unless they are fairly small and limited. An interface per instrument execution config, atom, step, etc. is too hard to employ because of the need to guarantee that you never introduce a path from the root of a query down to a leaf for which the type may differ depending upon the concrete implementation involved.

• Aliases make it difficult to supply JSON encoders / decoders for clients in the service's schema module as Todd discovered.
• Adding instrument prefixes only at the highest level would work if we could guarantee that there is no and will never be a query that offers an atom, step, etc. directly without going through the execution config. We already cannot meet that requirement.
• Adding instrument prefixes everywhere is cumbersome and really offers no guarantee of safety anyway, unless every concrete field has the prefix I suppose. Our API is huge and the 50th time you have to type gmosNorthSomething starts to grate.

So maybe the best idea is to convert the main interfaces into concrete types with a field for each possible instrument, all but one of which is always null.

[milessabin]

To clarify things a little for me, is this an accurate summary of the situation?

You have many types which are,

1. structurally very similar (hence have multi-step common paths)
2. can be meaningfully returned via reference to a common interface type or as components of a union (hence need to be distinguished using an inline or other fragment)
3. despite (1) and (2), have differing leaf types, or non-leaf terminal subtrees, which mean that when following common paths, at some point the types differ.

I'm guessing that to represent these types in Scala you would use parametric polymorphism to capture the variation in a common interface for these?

[toddburnside]

@milessabin Yes, that is the case, at least currently. For example, the first instruments we are supporting are almost identical instruments at the north and south telescopes, with a few differences. So, we are using type parameters to encode the differences. In the future, though, we'll have other instruments. They may have some very different fields. But, many instruments have things like filters, which serve the same purpose but are different for different instruments.

The differences in the instruments carry down into the sequences (steps needed to acquire data) and the data that is produced. So, we have a lot of places where the trees are very similar and things have the same "purpose", but different types.

[milessabin]

That helps, thanks.

A direct translation from an interface with type parameters to an interface without is always going to be tricky.

GraphQL will allow you to merge structurally similar types, so long as they agree on all the same-named leaf types. That gives you one other option that you might not have considered yet. You could replace all your conflicting scalar types with object types which are mergeable. Instead of,

type Query {
  instrument: Common
}

scalar Foo
scalar Bar

interface Common

type FooInstrument extends Common {
  thing: Foo
}

type BarInstrument extends Common {
  thing: Bar
}

query {
  instrument {
    ... on FooInstrument {
      thing  // Not mergeable
    }
    ... on BarInstrument {
      thing  // Not mergeable
    }
}

You could instead have,

type Query {
  instrument: Common
}

scalar Foo
scalar Bar
scalar CommonLeaf // scalar that can represent both Foo and Bar

type FooWrapper {
  foo: Foo
  common: CommonLeaf
}
type BarWrapper {
  bar: Bar
  common: CommonLeaf
}

interface Common

type FooInstrument extends Common {
  thing: Foo
  wrappedThing: FooWrapper
}

type BarInstrument extends Common {
  thing: Bar
  wrappedThing: BarWapper
}

query {
  instrument {
    ... on FooInstrument {
      wrappedThing {
        common // Mergeable
      }
    }
    ... on BarInstrument {
      wrappedThing {
        common // Mergeable
      }
    }
}

What's happening here is that you're trading opaque scalars for structured types which have fields which support unification across the family. I've preserved the original unwrapped fields, but if that isn't necessary you could simplify things by leaving them out.

I don't know if that's helpful in your scenario.

[toddburnside]

I don't know if that's helpful in your scenario.

Thanks for the suggestion, but that would add a lot of layers to the already deep trees. I think we'll just skip the interface part.

[milessabin]

but that would add a lot of layers to the already deep trees

It would only add one more layer, immediately above scalars at the leaves.

Anyhow, I'll stop flogging this dead horse now ...

[rpiaggio]

@milessabin would your suggestion still work if:

• We defined instad union CommonLeaf = Foo | Bar?
• We omitted the *Wrapper types and defined directly

  
  type FooInstrument extends Common {
  thing: Foo
  unitedThing: CommonLeaf
  }

type BarInstrument extends Common { thing: Bar unitedThing: CommonLeaf }

?

[milessabin]

@rpiaggio GraphQL doesn't support unions of scalars ... that's the root of your problem (rather than interfaces, which are blameless IMO).

[swalker2m]

https://github.com/gemini-hlsw/lucuma-odb/pull/1032