#Erik Sipsma3294 So it s working out

1 messages · Page 1 of 1 (latest)

waxen quiver
#

Yeah that's a good question, I haven't tried any of them with extend yet. I hope they do work with it, otherwise we will have to resort to mangling again or to customizing them further (which we are gonna do anyways but was hoping to do that more incrementally over time)

vernal matrix
#

for the next phase we'll need sub-resolvers

#

and for extend to work

#

(e.g. extend Query { alpine: Alpine! })

waxen quiver
#

Yes for sure, I'm thinking about some of the architecture and terminology stuff right now, but can see what happens w/ the various generators when you give them extend early next week if you haven't gotten there yet.

#

(I also gotta do another pass to add more labels+priority, just got the creation of all of them out of the way now)

vernal matrix
#

Oh, btw, arguably that's the difference between SDL and introspection

#

with introspection you don't get the extend etc, only the end result

#

panic: conflict on type "Core": field "image" re-defined <-- went with simple "fail on conflict" merging strategy for "stitching", for now

vernal matrix
#

Pretty happy with the API so far ...

#

pretty close to have converted Core over this

waxen quiver
waxen quiver
#

I'm just curious what the extend looks like

#

And can then see how it'd fit (or not fit) with the code generation for user actions

vernal matrix
#

introspection uses a some queries like __type to list the types -- there's no way to actually see the "extend" from there, it's listing objects and their fields

#

(as they're loaded in the executable schema)

#

SDLs ... well in the CURRENT implementation they're concatenated, I don't think there's anything preventing from merging the extend ahead of time and building up a new SDL

#

I think that's why apollo is using _service { sdl } instead of using the introspection query -- it'd lose the extend field, probably other things

waxen quiver
#

Right I guess what I was wondering is if we only had a bunch of instrospection JSONs and merged them (like if they have the same keys, merge all the values underneath), would that create the same result as if you had taken a bunch of SDLs that use extend, parse the schema and then introspect it?

Not sure if that makes sense, if not, no worries, it's mostly for my own curiosity

vernal matrix
#

I think we'd lose sanity checking

#

e.g. we'd have to "assume" we want to extend a type

#

(they'll just show up as duplicates through introspection)

#

whereas with the SDLs, we know the difference between type Filesystem and extend type Filesystem

waxen quiver
#

Ah okay, yeah that makes sense

#

Yeah I don't think we should switch to introspection format, I was just curious

vernal matrix
#

Yep

#

well I was the one pitching introspection yesterday

#

this changed my mind

#

I think we should use "introspection", but return the SDLs rather than introspection query

#

(and even that, that's a maybe)

waxen quiver
waxen quiver
vernal matrix
#

oh, the other option is to continue doing what we're doing today

#

read from file

waxen quiver
#

Ah I see

vernal matrix
#

instead of execop'ing an "introspection"

#

(and the SDK could respond to the introspection request, so it's not like the user needs to do anything)

#

the advantage I guess is freedom of schema. You could split it up in a bunch of files, it doesn't need a magical path, etc

#

downside is it's not hermetic. Right now we know that for a given "stable FS" (like, from a git commit or whatever), we generate the stubs and they're not going to change so long as it's the same FS

#

with introspection we don't know what's happening

#

anyway, it's low priority, either way, IMO

waxen quiver
#

Actually this gets back to what we were talking about before a little bit. Maybe import should not just take a Filesystem, but instead take both a Filesystem and a separate arg for the SDL.

We could still implement the current approach where the schema is read from the Filesystem (the client can just read it using the other core apis), but we'll also have the flexibility to do more in the future

vernal matrix
# 
var _ router.ExecutableSchema = &FilesystemSchema{}

type FilesystemSchema struct {
}

func (s *FilesystemSchema) Schema() string {
    return `
    scalar FSID

    type Filesystem {
        id: FSID!
    }

    extend type Core {
        filesystem(id: FSID!): Filesystem!
    }
    `
}

func (r *FilesystemSchema) Resolvers() router.Resolvers {
    return router.Resolvers{
        "FSID": fsIDResolver,
        "Core": router.ObjectResolver{
            "filesystem": r.Filesystem,
        },
    }
}

func (r *FilesystemSchema) Filesystem(p graphql.ResolveParams) (interface{}, error) {
    return &model.Filesystem{
        ID: p.Args["id"].(model.FSID),
    }, nil
}
# 
    schema, err := router.Stitch([]router.ExecutableSchema{
        &router.RootSchema{},
        &core.CoreSchema{},
        &core.FilesystemSchema{},
    })
#

it's almost as pleasant as gqlgen

#

(sorry, that's a different topic, got excited for a minute)

waxen quiver
#

We don't have to switch right now to the different model of import where SDL and the implementation Filesystem are separate args, but that does feel right somehow, and if it simplifies the updates you're making then maybe we should do it

vernal matrix
#

yep

#

btw, are you making any schema changes?

#

I'm starting to port them over

waxen quiver
#

No i have pushed everything, not changing that locally

vernal matrix
#

perfect

vernal matrix
#

hey, I'm just going to assume that arguments are there with the correct type, since graphql is already doing type checking

#

removes a ton of boilerplate

#

any concerns?

waxen quiver
#

graphql doesn't do all the checking

vernal matrix
#

before:

 "image": &tools.FieldResolve{
    Resolve: func(p graphql.ResolveParams) (interface{}, error) {
        rawRef, ok := p.Args["ref"]
        if !ok {
            return nil, fmt.Errorf("missing ref")
        }
        ref, ok := rawRef.(string)
        if !ok {
            return nil, fmt.Errorf("ref is not a string")
        }
        llbdef, err := llb.Image(ref).Marshal(p.Context, llb.Platform(getPlatform(p)))
        if err != nil {
            return nil, err
        }
        gw, err := getGatewayClient(p)
        if err != nil {
            return nil, err
        }
        _, err = gw.Solve(context.Background(), bkgw.SolveRequest{
            Evaluate:   true,
            Definition: llbdef.ToPB(),
        })
        if err != nil {
            return nil, err
        }

        return newFilesystem(llbdef), nil
    },
}

now:

func (r *coreSchema) Image(p graphql.ResolveParams) (interface{}, error) {
    ref := p.Args["ref"].(string)
    st := llb.Image(ref)
    return r.Solve(p.Context, st)
}
vernal matrix
#

i realized that if it happens we should panic, something went terribly wrong

waxen quiver
#

Yeah I think the problem is that you don't see the entire panic, including stack trace because the panic gets caught by graphql-go, so you only see an error saying the cast was wrong but not the line number

#

I could be misremembering but I think that's what happened

#

and why I started letting copilot fill all that in

#

either way, not a big deal

vernal matrix
#

yeah, but for it to happen, it means the schema got "bypassed" somehow

#

can only happen with a graphql-go bug

#

except for non-mandatory fields, for those we do need the check

waxen quiver
# vernal matrix can only happen with a graphql-go bug

No it can happen for custom types. I.e. the action resolver returns what it unmarshals into the interface{} object here: https://github.com/dagger/cloak/blob/e60aede62917e4e2711cca499d019c857f0b14ed/api/graphql.go#L190-L193

None of those types will be cast to, i.e. *Filesystem, even though in other methods they may actually already be converted to that type before being passed to the resolver. That's why I temporarily added these if statements: https://github.com/dagger/cloak/blob/e60aede62917e4e2711cca499d019c857f0b14ed/api/chaining.go#L218-L232