chore: fix reviewed interface and TypeScript codegen regressions#417
Open
grouville wants to merge 5 commits into
Open
chore: fix reviewed interface and TypeScript codegen regressions#417grouville wants to merge 5 commits into
grouville wants to merge 5 commits into
Conversation
Scale-out has to rebuild the module in the scaled-out query before it can run the selected function or check remotely. For a normal CLI checkout, a command such as dagger --scale-out check usually starts from moduleSource(refString: "."), which is a local or git module source. This bug is in the other user-facing shape: the module was created from a Directory object first, for example through host.directory(...).asModule or Directory.asModuleSource(...), and then the module was used by the scale-out path.
In that Directory-source case, ModuleSourceKindDir stores the original Directory object ID in DirSrc.OriginalContextDir and stores the source root path separately. The old scale-out query tried to rebuild the module source like this:
{
node(id: "...") {
asModuleSource(sourceRootPath: "src") {
asModule {
id
}
}
}
}
That query is invalid GraphQL. node(id:) returns the Node interface, and Node only exposes id. asModuleSource is a Directory field, so GraphQL validation rejects asModuleSource when it is selected directly from node(id:).
The valid query first narrows the Node result to Directory:
{
node(id: "...") {
... on Directory {
asModuleSource(sourceRootPath: "src") {
asModule {
id
}
}
}
}
}
Fix buildScaleOutModuleQuery by adding the Directory inline fragment before selecting asModuleSource when the stored module source came from a Directory.
The regression test builds the scale-out query for a module whose source was created from a Directory, then validates the generated GraphQL against a tiny schema containing only node, Directory.asModuleSource, ModuleSource.asModule, and Module.id. Without the fix, validation fails because asModuleSource is selected from Node. With the fix, the query validates through the Directory fragment.
Focused repro:
dagger call engine-dev test --pkg ./core --run '^TestModTreeNode/TestBuildScaleOutModuleQueryForModuleLoadedFromDirectory$' --test-verbose
Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>
DagQL checks whether a field can implement an interface field before publishing that relationship in the GraphQL schema. The old check verified that every argument declared by the interface existed on the implementation with a compatible type. It did not check for required arguments that exist only on the implementation.
A module author can hit this with an interface method that takes no argument and an object method that requires one:
type Runnable interface {
DaggerObject
Run(ctx context.Context) (string, error)
}
type Job struct{}
func (j *Job) Run(ctx context.Context, target string) (string, error) {
return target, nil
}
If Job is accepted as Runnable, callers that only know they have a Runnable can call Run, but they cannot pass target because target is not part of the interface method. In GraphQL terms, the invalid contract is:
interface Runnable {
run: String!
}
type Job {
run(target: String!): String!
}
That required target argument is implementation-only. A query through Runnable cannot provide it, so the implementation can fail when the Job resolver is called without a required value.
Extra implementation arguments are still valid when callers are not forced to provide them:
type Job {
run(optionalTarget: String): String!
runWithDefault(target: String! = "default"): String!
}
Fix argsCompatible by scanning implementation arguments after the interface arguments have been matched. Extra arguments are allowed when they are nullable or have a default value. An extra non-null argument without a default now makes the implementation incompatible.
The tests cover both object and interface cases. Query.run(target:) no longer satisfies an interface field shaped as run(). TransformC, which adds required z, no longer satisfies TransformA. TransformCDefault documents the allowed case where the extra z argument has a default.
Focused repro:
dagger call engine-dev test --pkg ./dagql --run '^TestInterfaces$' --test-verbose
Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>
DagQL compares field types before allowing one field to implement an interface field. For list return types, the old comparison checked the element type before checking whether the list value itself could be null. That accepted a field returning a nullable list for an interface field that promised a list would always be present.
This invalid return shape was accepted:
interface NonNullList {
names: [String!]!
}
type Query {
names: [String!]
}
Both fields agree that every element is a non-null String. They disagree about the list itself. Callers through NonNullList rely on names never returning null, but the implementation can return null for the whole list.
List arguments need the same outer-list check in the opposite direction. It is valid for an implementation to accept null when the interface always passes a list:
interface NonNullListArg {
acceptsNames(names: [String!]!): String!
}
type Query {
acceptsNames(names: [String!]): String!
}
The reverse is invalid because a caller using the interface may omit the list or pass null, while the implementation requires the list:
interface NullableListArg {
acceptsNames(names: [String!]): String!
}
type Query {
acceptsNames(names: [String!]!): String!
}
Fix typeCompatible by checking the list value's NonNull flag before recursing into the element type. Fix argTypeCompatible with the matching argument rule, where the implementation must accept every value the interface allows callers to pass.
The tests keep the cases separate. namesOrNull cannot satisfy an interface that promises names is always a list. acceptsNames cannot satisfy an interface that allows names to be null. The valid directions are tested too: a non-null list result can satisfy a nullable-list interface field, and a method accepting names or null can satisfy an interface that always passes names.
Focused repro:
dagger call engine-dev test --pkg ./dagql --run '^TestInterfaces$' --test-verbose
Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>
Some GraphQL arguments are encoded as the ID scalar but are still object or interface inputs. The schema marks those cases with @expectedType so SDK generators can expose the generated client type instead of asking users to pass a raw ID.
For example, this schema field should accept a File client object in TypeScript:
type Query {
file(id: ID @expectedType(name: "File")): File!
}
The TypeScript generator also has a special case for arguments literally named id. That branch ran before @expectedType handling, so an id-named object input was generated as a plain ID:
file = (id: ID): File
That makes generated TypeScript clients reject the natural call shape where the user passes a File object. The same schema worked when the argument had any name other than id, because those arguments reached the @expectedType path.
Fix formatInputType by handling @expectedType before the id-name special case. Modern TypeScript generation now emits the generated object or interface type first:
file = (id: File): File
Legacy SDK compatibility is preserved for old FooID aliases on id arguments when appropriate. Plain ID arguments without @expectedType still generate as ID.
The regression test is dedicated to this edge case: Query.file(id: ID @expectedType(name: "File")) must generate file = (id: File): File and must not generate file = (id: ID): File.
Focused repro:
dagger call engine-dev test --pkg ./cmd/codegen/generator/typescript/templates/src --run '^TestModernTypeScriptFileInputNamedIDUsesFileType$' --test-verbose
Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>
TypeScript method generation maps GraphQL object IDs into generated client instances. For a concrete object list, that is correct:
containers: [Container!]!
results.map((r) => new Container(ctx.copy().selectNode(r.id, "Container")))
After TypeRef.IsObject started treating GraphQL interfaces as object-like, list fields returning interfaces also entered that concrete-object branch. For a schema like this:
interface Syncer {
id: ID!
}
type Query {
syncers: [Syncer!]!
}
the generated TypeScript tried to compile this:
results.map((r) => new Syncer(ctx.copy().selectNode(r.id, "Syncer")))
That is invalid because Syncer is emitted as a TypeScript interface, not a runtime class. The constructible client wrapper is _SyncerClient, which is already what single interface-returning methods use.
Add an IsListOfInterface template helper that unwraps NonNull and List wrappers and detects an interface element type. Use it inside the existing list-of-object branch to construct _<Interface>Client for interface lists while preserving the existing class construction for concrete object lists:
results.map((r) => new _SyncerClient(ctx.copy().selectNode(r.id, "Syncer")))
The regression test renders a schema with Query.syncers returning [Syncer!]! and asserts that the generated method returns Promise<Syncer[]> while mapping results through new _SyncerClient(...). It also asserts that new Syncer(...) is not emitted.
Focused repro for this commit:
dagger call engine-dev test --pkg ./cmd/codegen/generator/typescript/templates/src --run '^TestInterfaceListReturnUsesClientWrapper$' --test-verbose
Focused repro for both TypeScript generator regressions in this stack:
dagger call engine-dev test --pkg ./cmd/codegen/generator/typescript/templates/src --run '^(TestModernTypeScriptFileInputNamedIDUsesFileType|TestInterfaceListReturnUsesClientWrapper)$' --test-verbose
Signed-off-by: Guillaume de Rouville <guillaume@dagger.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
While reviewing the interface/codegen PR, I seem to have found five edge cases where the new behavior could produce invalid GraphQL, accept an invalid interface contract, or generate TypeScript clients that do not compile.
This PR keeps the fixes split by failure mode. Each commit contains:
The elements tested are:
A module loaded from a
Directory, for example withhost.directory(...).asModule, could fail when used through the scale-out path. The generated GraphQL tried to callasModuleSourceonnode(id:), butnode(id:)only promises a genericNode. The query now first narrows that node toDirectory.An object could incorrectly satisfy an interface even when its method required an extra argument that the interface did not declare. For example,
run(target:)was allowed to implementrun(), even though callers using the interface have no way to passtarget.A field returning “maybe a list” could incorrectly satisfy an interface field promising “always a list”. The previous check compared the list element type but missed whether the list itself could be null. The same outer-list nullability rule is now also checked for method arguments.
A TypeScript generated method with an object input named
idcould ask callers for rawIDinstead of the generated object type. For example,file(id: File)could be generated asfile(id: ID), causing type errors for callers passing aFileclient object.A TypeScript generated method returning a list of interfaces could emit invalid runtime code. The public type should stay
Promise<Syncer[]>, but internally the generator must construct_SyncerClientwrappers for each returned ID. The broken code tried to donew Syncer(...), which cannot compile becauseSynceris a TypeScript interface.