🐞 cli: raise error when a function return a list of containers

wingyplus commented 4 weeks ago

What is the issue?

From this snippet:

$ cat main.go
package main

import (
        "dagger/reproduce-bug/internal/dagger"
)

type ReproduceBug struct{}

func (m *ReproduceBug) Containers() []*dagger.Container {
        return []*dagger.Container{
                dag.Container(dagger.ContainerOpts{Platform: "linux/arm64"}).From("alpine:latest"),
                dag.Container(dagger.ContainerOpts{Platform: "linux/amd64"}).From("alpine:latest"),
        }
}

When calling dagger call containers, the cli raise an error:

Setup tracing at https://dagger.cloud/traces/setup. To hide: export STOPIT=1

✔ connect 1.2s
✔ initialize 0.3s
✔ prepare 0.0s
✔ reproduceBug: ReproduceBug! 0.0s
✔ ReproduceBug.containers: [Container!]! 1.2s
✔ Container.sync: ContainerID! 0.0s
✔ Container.sync: ContainerID! 0.0s

Error: response from query: json: cannot unmarshal array into Go value of type string
Run 'dagger call containers --help' for usage.

Expected that it will return a list of container information or more descriptive error if it's not supported.

Dagger version

dagger v0.12.5 (registry.dagger.io/engine:v0.12.5) darwin/arm64

Steps to reproduce

No response

Log output

No response

helderco commented 3 weeks ago

This is a regression. As you can see, there's a "List of containers" example in:

https://github.com/dagger/dagger/pull/7479

It may have broken in the following PR, since there's an implicit sync that only happens in Container objects:

https://github.com/dagger/dagger/pull/7829

You can see there's test coverage for []*dagger.File, but not for []*dagger.Container, that's why it wasn't caught:

https://github.com/dagger/dagger/blob/cbf9efa9b7cf687dd566149e6761c0634b7e4c7e/core/integration/module_call_test.go#L1602-L1607

I'll investigate, thanks for reporting!

helderco commented 3 weeks ago

The problem is that on a single Container, the CLI produces these queries:

query {
    test {
        sync
    }
}

query {
    loadContainerFromID(id: "xxx") {
        defaultArgs
        mounts
        user
        workdir
        entrypoint
        platform
    }
}

The same happens only for Terminal, btw. The end result is that you get an evaluated Container before printing those values to the screen. The sync is there because by removing stdout and stderr, it would just show the metadata without evaluating the pipeline. The Directory and File objects don't have the same problem.

If it's a list of Container objects, the following will instead return multiple values:

query {
    test {
        sync
    }
}

Since the request with sync expects a single string here:

https://github.com/dagger/dagger/blob/86351c98d9d3b0d373cbb06d33977cea528e25fe/cmd/dagger/functions.go#L642-L646

It's now getting a []string when parsing the response from multiple Container objects (even though there's no difference in the request, only in the response), which explains this error message:

json: cannot unmarshal array into Go value of type string

There's three possible solutions here:

Don't do sync when it's a list of objects, i.e., just print the metadata, without evaluating.
- Caveat: introduces inconsistency by only evaluating when it's not a list.

Produce a query with multiple sub-queries, based on the list of IDs from sync.

Caveat: our Go query builder doesn't support this as it was custom built for a single chain.

Example:

query  {
cont1: loadContainerFromID(id: "xxx") {
    defaultArgs
    mounts
    user
    workdir
    entrypoint
    platform
}
cont2: loadContainerFromID(id: "yyy") {
    defaultArgs
    mounts
    user
    workdir
    entrypoint
    platform
}
cont3: loadContainerFromID(id: "zzz") {
    defaultArgs
    mounts
    user
    workdir
    entrypoint
    platform
}
}

Make separate concurrent requests for each Container, and append the results afterwards.

The 2nd option is more performant but I'm leaning on the 3rd option because I think it's easier to adapt to while the performance penalty should be negligible.

Of course we could also just return an error but I don't think that's necessary.

wingyplus commented 3 weeks ago

Do we need to keep the list ordered? I'm not sure if the option 3 can cause the list shuffle because concurrent processing.

helderco commented 3 weeks ago

It's not a problem because we can ensure the index in the list is preserved, based on the indices from the list of IDs after { containers { sync } }.

dagger / dagger