Skip to content

Commit

Permalink
implement new conventions for IDable objects (#5881)
Browse files Browse the repository at this point in the history
* implement new conventions for IDable objects

This is an assorted set of sweeping changes that fix the longstanding
bug involved with traversing lists of ID-able objects in the SDK. We've
worked around this issue about 10 times in the past two weeks, and now I
understand why!

In short, we need a reliable way to go from foos: [Foo!]!, to selecting
the Nth Foo, and querying against it.

In Go this looks like the following, which actually panics prior to
this commit:

    funs, _ := mod.Functions(ctx)
    for _, fun := range funs {
      fun.Name(ctx) // BOOM
    }

This was panicking because the implementation for returning lists of
objects only dealt with scalars, leaving the rest of the object "hollow"
(nil q or c fields). It could have populated c, but q was impossible:
how do you chain queries from the Nth element in a list? If you had a
list of ID-able objects you could at least query them by ID and chain
from there, but how do you know how to query by ID?

This commit adds a new rule: all ID-able objects implement a top-level
`loadFooFromID(id: FooID!): Foo!` resolver. That means when an SDK sees
a list of [Foo!]! objects, it knows it can just fetch IDs and then
create objects that query from loadFooFromID(id: $id).

This new rule is leveraged by a new `core/schema/` helper for
registering resolvers that follow this pattern, to help cut down on a
ton of boilerplate. (I think more code can be deleted than what's in
this commit, may look after.)

Now that there's a dedicated constructor for this, we can start removing
the optional ID arg from other constructors, which also means that
ID-able objects can now be constructed with required args. I applied
this change to GeneratedCode, Module, and Function, and deprecated the
id args everywhere else.

Along the way, I had to rename CacheID to CacheVolumeID. This has long
been a thorn in every SDK developer's side, since it's the _only ID
type_ that doesn't match its corresponding object type. We're about to
make breaking changes for Services v2 so I figure this is worth doing
now.

Also, bumps graphql-go-tools to a version that doesn't just swallow
schema errors and result in cryptic second-order runtime errors.

Signed-off-by: Alex Suraci <[email protected]>

* sdk/rust: remove CacheVolumeID special-case, regen

Signed-off-by: Alex Suraci <[email protected]>

* regen node, python

Signed-off-by: Alex Suraci <[email protected]>

* python: remove noqa now that deprecations are back

Signed-off-by: Alex Suraci <[email protected]>

* sdk/go: refactor away high-maintenance type maps

* One was of all custom scalars (IDs + Platform + JSON) and was
  ultimately used to detect enums during marshaling. This is now handled
  using an interface instead that all enum values conform to.
* The other was, and had to be, only IDs. This was used for detecting if
  FooID! types should be converted to object counterparts. Now we just
  rely on type names always being Type+"ID", which we can do now that
  they're all consistent.

Signed-off-by: Alex Suraci <[email protected]>

* re-bootstrap go sdk

Signed-off-by: Alex Suraci <[email protected]>

* add changie

Signed-off-by: Alex Suraci <[email protected]>

* fix up deprecation syntax + typeDef doc

Signed-off-by: Alex Suraci <[email protected]>

* regen SDKs

Signed-off-by: Alex Suraci <[email protected]>

* add missing doc + regen

Signed-off-by: Alex Suraci <[email protected]>

---------

Signed-off-by: Alex Suraci <[email protected]>
  • Loading branch information
vito authored Oct 12, 2023
1 parent ee68d51 commit 17bd90a
Show file tree
Hide file tree
Showing 17 changed files with 580 additions and 351 deletions.
79 changes: 48 additions & 31 deletions codegen/generator/functions.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,6 @@ const (
QueryStructClientName = "Client"
)

// CustomScalar registers custom Dagger type.
// TODO: This may done it dynamically later instead of a static
// map.
var CustomScalar = map[string]string{
"ContainerID": "Container",
"FileID": "File",
"DirectoryID": "Directory",
"SecretID": "Secret",
"SocketID": "Socket",
"CacheID": "CacheVolume",
"ModuleID": "Module",
"FunctionID": "Function",
"TypeDefID": "TypeDef",
"GeneratedCodeID": "GeneratedCode",
}

// FormatTypeFuncs is an interface to format any GraphQL type.
// Each generator has to implement this interface.
type FormatTypeFuncs interface {
Expand Down Expand Up @@ -65,6 +49,50 @@ func (c *CommonFunctions) IsSelfChainable(t introspection.Type) bool {
return false
}

func (c *CommonFunctions) InnerType(t *introspection.TypeRef) *introspection.TypeRef {
switch t.Kind {
case introspection.TypeKindNonNull:
return c.InnerType(t.OfType)
case introspection.TypeKindList:
return c.InnerType(t.OfType)
default:
return t
}
}

func (c *CommonFunctions) ObjectName(t *introspection.TypeRef) string {
switch t.Kind {
case introspection.TypeKindNonNull:
return c.ObjectName(t.OfType)
case introspection.TypeKindObject:
return t.Name
default:
panic(fmt.Sprintf("unexpected type kind %s", t.Kind))
}
}

func (c *CommonFunctions) IsIDableObject(t *introspection.TypeRef) bool {
schema := GetSchema()
switch t.Kind {
case introspection.TypeKindNonNull:
return c.IsIDableObject(t.OfType)
case introspection.TypeKindObject:
schemaType := schema.Types.Get(t.Name)
if schemaType == nil {
panic(fmt.Sprintf("schema type %s is nil", t.Name))
}

for _, f := range schemaType.Fields {
if f.Name == "id" {
return true
}
}
return false
default:
return false
}
}

// FormatReturnType formats a GraphQL type into the SDK language output,
// unless it's an ID that will be converted which needs to be formatted
// as an input (for chaining).
Expand Down Expand Up @@ -137,21 +165,10 @@ func (c *CommonFunctions) ConvertID(f introspection.Field) bool {
if ref.Kind != introspection.TypeKindScalar {
return false
}

// FIXME: As of now all custom scalars are IDs. If that changes we
// need to make sure we can tell the difference.
alias, ok := CustomScalar[ref.Name]

// FIXME: We don't have a simple way to convert any ID to its
// corresponding object (in codegen) so for now just return the
// current instance. Currently, `sync` is the only field where
// the error is what we care about but more could be added later.
// To avoid wasting a result, we return the ID which is a leaf value
// and triggers execution, but then convert to object in the SDK to
// allow continued chaining. For this, we're assuming the returned
// ID represents the exact same object but if that changes, we'll
// need to adjust.
return ok && alias == f.ParentObject.Name
// NB: we only concern ourselves with the ID of the parent class, since this
// is really only meant for ID and Sync, the only cases where we
// intentionally return an ID (leaf node) instead of an object.
return ref.Name == f.ParentObject.Name+"ID"
}

// FormatInputType formats a GraphQL type into the SDK language input
Expand Down
6 changes: 3 additions & 3 deletions codegen/generator/go/templates/format.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
package templates

import "dagger.io/dagger/codegen/generator"
import "strings"

// FormatTypeFunc is an implementation of generator.FormatTypeFuncs interface
// to format GraphQL type into Golang.
Expand Down Expand Up @@ -32,8 +32,8 @@ func (f *FormatTypeFunc) FormatKindScalarBoolean(representation string) string {
}

func (f *FormatTypeFunc) FormatKindScalarDefault(representation string, refName string, input bool) string {
if alias, ok := generator.CustomScalar[refName]; ok && input {
representation += "*" + alias
if obj, rest, ok := strings.Cut(refName, "ID"); input && ok && rest == "" {
representation += "*" + obj
} else {
representation += refName
}
Expand Down
3 changes: 3 additions & 0 deletions codegen/generator/go/templates/functions.go
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,9 @@ func (funcs goTemplateFuncs) FuncMap() template.FuncMap {
"ToUpperCase": funcs.ToUpperCase,
"ConvertID": funcs.ConvertID,
"IsSelfChainable": funcs.IsSelfChainable,
"IsIDableObject": funcs.IsIDableObject,
"InnerType": funcs.InnerType,
"ObjectName": funcs.ObjectName,

// go specific
"Comment": funcs.comment,
Expand Down
2 changes: 1 addition & 1 deletion codegen/generator/go/templates/modules.go
Original file line number Diff line number Diff line change
Expand Up @@ -626,7 +626,7 @@ func (ps *parseState) goMethodToAPIFunctionDef(typeName string, fn *types.Func,
return nil, fmt.Errorf("method %s has too many return values", fn.Name())
}

fnDef := Qual("dag", "NewFunction").Call(Lit(fn.Name()), Add(Line(), fnReturnType))
fnDef := Qual("dag", "Function").Call(Lit(fn.Name()), Add(Line(), fnReturnType))

funcDecl, err := ps.declForFunc(fn)
if err != nil {
Expand Down
3 changes: 2 additions & 1 deletion codegen/generator/go/templates/src/enum.go.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,12 @@
{{- $enumName := .Name }}
type {{ $enumName }} string

func ({{ $enumName }}) IsEnum() {}

const (
{{- range $index, $field := .EnumValues | SortEnumFields }}
{{ $field.Name | FormatEnum}} {{ $enumName }} = "{{ $field.Name }}"
{{- end }}
)

{{- end }}
{{- end }}
51 changes: 13 additions & 38 deletions codegen/generator/go/templates/src/object.go.tmpl
Original file line number Diff line number Diff line change
Expand Up @@ -97,29 +97,33 @@ type {{ $field | FieldOptionsStructName }} struct {
q = q.Select("{{ range $i, $v := $field | GetArrayField }}{{ if $i }} {{ end }}{{ $v.Name }}{{ end }}")

type {{ $field.Name | ToLowerCase }} struct {
{{ range $v := $field | GetArrayField }}
{{ range $v := $field | GetArrayField }}
{{ $v.Name | ToUpperCase }} {{ $v.TypeRef | FormatOutputType }}
{{- end }}
{{- end }}
}

{{$eleType := $field.TypeRef | InnerType}}
convert := func(fields []{{ $field.Name | ToLowerCase }}) {{ $field.TypeRef | FormatOutputType }} {
out := {{ $field.TypeRef | FormatOutputType }}{}

for i := range fields {
out = append(out, {{ $field.TypeRef | FormatOutputType | FormatArrayToSingleType }}{{"{"}}{{ $field | GetArrayField | FormatArrayField }}{{"}"}})
val := {{ $field.TypeRef | FormatOutputType | FormatArrayToSingleType }}{{"{"}}{{ $field | GetArrayField | FormatArrayField }}{{"}"}}
{{- if $eleType | IsIDableObject }}
val.q = querybuilder.Query().Select("load{{$eleType | ObjectName}}FromID").Arg("id", fields[i].Id)
val.c = r.c
{{- end }}
out = append(out, val)
}

return out
}

{{- end }}

{{- end }}

{{- if and $field.TypeRef.IsList (IsListOfObject $field.TypeRef) }}
var response []{{ $field.Name | ToLowerCase }}
{{- else }}
{{- else }}
var response {{ $field.TypeRef | FormatOutputType }}
{{- end }}
{{- end }}

q = q.Bind(&response)
{{- $typeName := $field.TypeRef | FormatOutputType }}
Expand Down Expand Up @@ -176,36 +180,7 @@ func (r *{{ $.Name | FormatName }}) UnmarshalJSON(bs []byte) error {
if err != nil {
return err
}
{{ if eq ($.Name | FormatName) "Container" }}
*r = *dag.Container(ContainerOpts{
ID: ContainerID(id),
})
{{ else if eq ($.Name | FormatName) "Directory" }}
*r = *dag.Directory(DirectoryOpts{
ID: DirectoryID(id),
})
{{ else if eq ($.Name | FormatName) "Socket" }}
*r = *dag.Socket(SocketOpts{
ID: SocketID(id),
})
{{ else if eq ($.Name | FormatName) "Module" }}
*r = *dag.Module(ModuleOpts{
ID: ModuleID(id),
})
{{ else if eq ($.Name | FormatName) "Function" }}
*r = *dag.Function(FunctionID(id))
{{ else if eq ($.Name | FormatName) "File" }}
*r = *dag.File(FileID(id))
{{ else if eq ($.Name | FormatName) "Secret" }}
*r = *dag.Secret(SecretID(id))
{{ else if eq ($.Name | FormatName) "Cache" }}
cacheID := CacheID(id)
*r = CacheVolume{
q: dag.q,
c: dag.c,
id: &cacheID,
}
{{ end }}
*r = *dag.Load{{ $.Name | FormatName }}FromID({{$.Name | FormatName}}ID(id))
return nil
}
{{- end }}
Expand Down
6 changes: 4 additions & 2 deletions codegen/generator/nodejs/templates/format.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
package templates

import (
"strings"

"dagger.io/dagger/codegen/generator"
)

Expand Down Expand Up @@ -34,9 +36,9 @@ func (f *FormatTypeFunc) FormatKindScalarBoolean(representation string) string {
}

func (f *FormatTypeFunc) FormatKindScalarDefault(representation string, refName string, input bool) string {
if alias, ok := generator.CustomScalar[refName]; ok && input {
if obj, rest, ok := strings.Cut(refName, "ID"); input && ok && rest == "" {
// map e.g. FooID to Foo
representation += formatName(alias)
representation += formatName(obj)
} else {
representation += refName
}
Expand Down
2 changes: 1 addition & 1 deletion codegen/generator/nodejs/templates/src/objects_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ var objectsJSON = `
"name": null,
"ofType": {
"kind": "SCALAR",
"name": "CacheID",
"name": "CacheVolumeID",
"ofType": null
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,25 +3,25 @@
* A directory whose contents persist across runs
*/
export class CacheVolume extends BaseClient {
private readonly _id?: CacheID = undefined
private readonly _id?: CacheVolumeID = undefined

/**
* Constructor is used for internal usage only, do not create object from it.
*/
constructor(
parent?: { queryTree?: QueryTree[], host?: string, sessionToken?: string },
_id?: CacheID,
_id?: CacheVolumeID,
) {
super(parent)

this._id = _id
}
async id(): Promise<CacheID> {
async id(): Promise<CacheVolumeID> {
if (this._id) {
return this._id
}

const response: Awaited<CacheID> = await computeQuery(
const response: Awaited<CacheVolumeID> = await computeQuery(
[
...this._queryTree,
{
Expand Down
Loading

0 comments on commit 17bd90a

Please sign in to comment.