Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Preserve extension fields when marshalling/unmarshalling #511

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
86 changes: 72 additions & 14 deletions load.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import (
"strings"

"github.com/goccy/go-yaml"
"github.com/goccy/go-yaml/parser"
"github.com/moby/buildkit/frontend/dockerfile/shell"
"github.com/pkg/errors"
"golang.org/x/exp/maps"
Expand Down Expand Up @@ -210,36 +211,93 @@ func (s *Spec) SubstituteArgs(env map[string]string, opts ...SubstituteOpt) erro
func LoadSpec(dt []byte) (*Spec, error) {
var spec Spec

dt, err := stripXFields(dt)
if err != nil {
return nil, fmt.Errorf("error stripping x-fields: %w", err)
}

if err := yaml.UnmarshalWithOptions(dt, &spec, yaml.Strict()); err != nil {
return nil, fmt.Errorf("error unmarshalling spec: %w", err)
return nil, errors.Wrap(err, "error unmarshalling spec")
}

if err := spec.Validate(); err != nil {
return nil, err
}
spec.FillDefaults()

spec.FillDefaults()
return &spec, nil
}

func stripXFields(dt []byte) ([]byte, error) {
func (s *Spec) UnmarshalYAML(f func(interface{}) error) error {
var obj map[string]interface{}
if err := yaml.Unmarshal(dt, &obj); err != nil {
return nil, fmt.Errorf("error unmarshalling spec: %w", err)

if err := f(&obj); err != nil {
return err
}

var ext map[string]interface{}

addExt := func(k string, v interface{}) {
if ext == nil {
ext = make(map[string]interface{})
}
ext[k] = v
}

for k := range obj {
if strings.HasPrefix(k, "x-") || strings.HasPrefix(k, "X-") {
delete(obj, k)
for k, v := range obj {
if !strings.HasPrefix(k, "x-") && !strings.HasPrefix(k, "X-") {
continue
}
addExt(k, v)
delete(obj, k)
}

type internalSpec Spec
var s2 internalSpec

dt, err := yaml.Marshal(obj)
if err != nil {
return err
}

parsed, err := parser.ParseBytes(dt, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit

Suggested change
parsed, err := parser.ParseBytes(dt, 0)
const noFlags = 0
parsed, err := parser.ParseBytes(dt, noFlags)

if err != nil {
return err
}

if err := yaml.UnmarshalWithOptions([]byte(parsed.Docs[0].String()), &s2, yaml.Strict()); err != nil {
return err
}

dt, err = yaml.Marshal(ext)
if err != nil {
return err
}

parsedExt, err := parser.ParseBytes(dt, 0)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto

if err != nil {
return errors.Wrap(err, "error parsing extension")
}

*s = Spec(s2)
s.ext = ext
s.extRaw = parsedExt

return nil
}

func (s Spec) MarshalYAML() ([]byte, error) {
// We need to define a new type to avoid infinite recursion of MarshalYAML.
type internalSpec Spec

if s.ext == nil {
return yaml.Marshal(internalSpec(s))
}

type specExt struct {
internalSpec `yaml:",inline"`
Ext map[string]interface{} `yaml:",omitempty,inline"`
}

return yaml.Marshal(obj)
return yaml.Marshal(specExt{
internalSpec: internalSpec(s),
Ext: s.ext,
})
}

func (s *BuildStep) processBuildArgs(lex *shell.Lex, args map[string]string, allowArg func(string) bool) error {
Expand Down
50 changes: 50 additions & 0 deletions load_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import (
"reflect"
"testing"

"github.com/goccy/go-yaml"
"github.com/moby/buildkit/frontend/dockerui"
"gotest.tools/v3/assert"
"gotest.tools/v3/assert/cmp"
Expand Down Expand Up @@ -1160,3 +1161,52 @@ func TestImage_validate(t *testing.T) {
})
}
}

func checkExt[T any](t *testing.T, spec Spec, key string, expect T) {
t.Helper()

var actual T
err := spec.Ext(key, &actual)
assert.NilError(t, err)
assert.Check(t, cmp.DeepEqual(actual, expect))
}

func TestExtensionFieldMarshalUnmarshal(t *testing.T) {
dt := []byte(`
name: test
x-hello: world
x-foo:
- bar
- baz
X-capitalized: world2
`)

var spec Spec
err := yaml.Unmarshal(dt, &spec)
assert.NilError(t, err)

assert.Check(t, cmp.Equal(spec.Name, "test"), spec)
checkExt(t, spec, "hello", "world")
checkExt(t, spec, "x-hello", "world")
checkExt(t, spec, "foo", []string{"bar", "baz"})
checkExt(t, spec, "x-foo", []string{"bar", "baz"})
checkExt(t, spec, "capitalized", "world2")
checkExt(t, spec, "X-capitalized", "world2")

// marshal and unmarshal to ensure the extension fields are preserved

dt, err = yaml.Marshal(spec)
assert.NilError(t, err)

var spec2 Spec
err = yaml.Unmarshal(dt, &spec2)
assert.NilError(t, err)

assert.Check(t, cmp.Equal(spec2.Name, "test"), spec2)
checkExt(t, spec2, "hello", "world")
checkExt(t, spec, "x-hello", "world")
checkExt(t, spec2, "foo", []string{"bar", "baz"})
checkExt(t, spec, "x-foo", []string{"bar", "baz"})
checkExt(t, spec, "capitalized", "world2")
checkExt(t, spec, "X-capitalized", "world2")
}
47 changes: 47 additions & 0 deletions spec.go
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,14 @@ package dalec

import (
"io/fs"
"strings"
"time"

"github.com/goccy/go-yaml"
"github.com/goccy/go-yaml/ast"
"github.com/moby/buildkit/client/llb"
"github.com/opencontainers/go-digest"
"github.com/pkg/errors"
)

// Spec is the specification for a package build.
Expand Down Expand Up @@ -94,6 +98,14 @@ type Spec struct {
// Each item in this list is run with a separate rootfs and cannot interact with other tests.
// Each [TestSpec] is run with a separate rootfs, asynchronously from other [TestSpec].
Tests []*TestSpec `yaml:"tests,omitempty" json:"tests,omitempty"`

// extRaw is the raw AST of the extension fields in the spec.
// This is used to extract the ext fields in [Spec.Ext]
extRaw *ast.File
// ext is all the ext fields in the spec.
// This gets used when marshalling the spec back to yaml.
// It is used to avoid having to re-parse the raw AST.
ext map[string]interface{}
Comment on lines +101 to +108
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like adding private fields to the spec struct, especially abstract stuff like *ast.File. We worked hard early on to ensure that the spec remained simple data, rather than data plus functionality.

For example, we decided not to implement the Source type as a group of interfaces because we wanted to maintain the spec as simple data. This also means that anyone importing the spec object will be importing goccy/go-yaml/ast.

I recognize that it will be a pain to wrap the spec and change function signatures, but a lot of that could be done with a find/replace operation and having the first member of the wrapper type be the spec:

type specExtensions struct {
        extRaw *ast.File
        ext map[string]interface{}
}

type SpecWithExtensions struct {
        *Spec
        ext specExtensions
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Spec is just a data type.
Currently it is unable to represent all the data that can go into a spec since we can't dynamically generate fields for extension fields.

These changes make it so Spec can represent that data and make it accessible to those who want it.
Its not changing the actual dalec spec in any, is not exposed outside of this package.

What concerns do you have for this?

}

// PatchSpec is used to apply a patch to a source with a given set of options.
Expand Down Expand Up @@ -427,3 +439,38 @@ func (s *SystemdConfiguration) EnabledUnits() map[string]SystemdUnitConfig {

return units
}

// Ext reads the extension field from the spec and unmarshals it into the target
// value.
func (s Spec) Ext(key string, target interface{}) error {
if !strings.HasPrefix(key, "x-") && !strings.HasPrefix(key, "X-") {
_, ok := s.ext["x-"+key]
if ok {
key = "x-" + key
} else {
_, ok = s.ext["X-"+key]
if !ok {
return errors.Errorf("extension field %q not found", key)
}
key = "X-" + key
}
}

p, err := yaml.PathString("$." + key)
if err != nil {
return err
}

node, err := p.FilterFile(s.extRaw)
if err != nil {
return errors.Wrap(err, "error filtering node")
}

dt := node.String()
err = yaml.Unmarshal([]byte(dt), target)
if err != nil {
return errors.Wrapf(err, "error unmarshalling extension field %q into target", key)
}

return nil
}
Loading