Skip to content
Merged
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
60 changes: 59 additions & 1 deletion alpha/declcfg/declcfg.go
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
package declcfg

import (
"bytes"
"encoding/json"
"errors"
"fmt"
"strings"

"go4.org/bytereplacer"

"github.com/operator-framework/operator-registry/alpha/property"
)
Expand Down Expand Up @@ -84,6 +90,7 @@ type RelatedImage struct {
type Meta struct {
Schema string
Package string
Name string

Blob json.RawMessage
}
Expand All @@ -93,17 +100,68 @@ func (m Meta) MarshalJSON() ([]byte, error) {
}

func (m *Meta) UnmarshalJSON(blob []byte) error {
blob = bytereplacer.New(`\u003c`, "<", `\u003e`, ">", `\u0026`, "&").Replace(blob)
Copy link
Copy Markdown
Member Author

@joelanford joelanford Apr 25, 2023

Choose a reason for hiding this comment

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

I moved the bytes replacement here, which meant I could drop that extra unmarshal call into doc json.RawMessage when we're deciding the reader. Between the allocation improvement of a direct bytes replacer and the removal of the other unmarshal, we shave off ~7% of the runtime of opm render when I run it against the operatorhub catalog.

Not too shabby!

In a follow-up, I might take a look at an alternate mode of action.Render (maybe a new function alongside Run()?), which lets the caller use a walk function. That would save a bunch on memory and might shave off more time as well.


type tmp struct {
Comment thread
stevekuznetsov marked this conversation as resolved.
Schema string `json:"schema"`
Package string `json:"package,omitempty"`
Name string `json:"name,omitempty"`
Properties []property.Property `json:"properties,omitempty"`
}
var t tmp
if err := json.Unmarshal(blob, &t); err != nil {
return err
// TODO: return an error that includes the the full JSON message,
// the offset of the error, and the error message. Let callers
// decide how to format it.
return errors.New(resolveUnmarshalErr(blob, err))
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I moved this here because it was the only place access to the full json message existed after I removed the extra unmarshal to json.RawMessage.

}
m.Schema = t.Schema
m.Package = t.Package
m.Name = t.Name
m.Blob = blob
return nil
}

func resolveUnmarshalErr(data []byte, err error) string {
var te *json.UnmarshalTypeError
if errors.As(err, &te) {
return formatUnmarshallErrorString(data, te.Error(), te.Offset)
}
var se *json.SyntaxError
if errors.As(err, &se) {
return formatUnmarshallErrorString(data, se.Error(), se.Offset)
}
return err.Error()
}

func formatUnmarshallErrorString(data []byte, errmsg string, offset int64) string {
sb := new(strings.Builder)
_, _ = sb.WriteString(fmt.Sprintf("%s at offset %d (indicated by <==)\n ", errmsg, offset))
// attempt to present the erroneous JSON in indented, human-readable format
// errors result in presenting the original, unformatted output
var pretty bytes.Buffer
err := json.Indent(&pretty, data, "", " ")
if err == nil {
pString := pretty.String()
// calc the prettified string offset which correlates to the original string offset
var pOffset, origOffset int64
origOffset = 0
for origOffset = 0; origOffset < offset; {
if pString[pOffset] != '\n' && pString[pOffset] != ' ' {
origOffset++
}
pOffset++
}
_, _ = sb.WriteString(pString[:pOffset])
_, _ = sb.WriteString(" <== ")
_, _ = sb.WriteString(pString[pOffset:])
} else {
for i := int64(0); i < offset; i++ {
_ = sb.WriteByte(data[i])
}
_, _ = sb.WriteString(" <== ")
_, _ = sb.Write(data[offset:])
}

return sb.String()
}
146 changes: 71 additions & 75 deletions alpha/declcfg/load.go
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
package declcfg

import (
"bytes"
"encoding/json"
"errors"
"fmt"
"io"
"io/fs"
"path/filepath"
"strings"

"github.com/joelanford/ignore"
"github.com/operator-framework/api/pkg/operators"
Expand All @@ -22,13 +20,68 @@ const (
indexIgnoreFilename = ".indexignore"
)

type WalkMetasFSFunc func(path string, meta *Meta, err error) error

func WalkMetasFS(root fs.FS, walkFn WalkMetasFSFunc) error {
return walkFiles(root, func(root fs.FS, path string, err error) error {
if err != nil {
return walkFn(path, nil, err)
}

f, err := root.Open(path)
if err != nil {
return walkFn(path, nil, err)
}
defer f.Close()

return WalkMetasReader(f, func(meta *Meta, err error) error {
return walkFn(path, meta, err)
})
})
}

type WalkMetasReaderFunc func(meta *Meta, err error) error

func WalkMetasReader(r io.Reader, walkFn WalkMetasReaderFunc) error {
dec := yaml.NewYAMLOrJSONDecoder(r, 4096)
for {
var in Meta
if err := dec.Decode(&in); err != nil {
if errors.Is(err, io.EOF) {
break
}
return walkFn(nil, err)
Comment thread
joelanford marked this conversation as resolved.
}

if err := walkFn(&in, nil); err != nil {
return err
}
}
return nil
}

type WalkFunc func(path string, cfg *DeclarativeConfig, err error) error

// WalkFS walks root using a gitignore-style filename matcher to skip files
// that match patterns found in .indexignore files found throughout the filesystem.
// It calls walkFn for each declarative config file it finds. If WalkFS encounters
// an error loading or parsing any file, the error will be immediately returned.
func WalkFS(root fs.FS, walkFn WalkFunc) error {
return walkFiles(root, func(root fs.FS, path string, err error) error {
if err != nil {
return walkFn(path, nil, err)
}

cfg, err := LoadFile(root, path)
if err != nil {
return walkFn(path, cfg, err)
}

return walkFn(path, cfg, nil)
})
}

func walkFiles(root fs.FS, fn func(root fs.FS, path string, err error) error) error {
Comment thread
everettraven marked this conversation as resolved.
if root == nil {
return fmt.Errorf("no declarative config filesystem provided")
}
Expand All @@ -40,20 +93,15 @@ func WalkFS(root fs.FS, walkFn WalkFunc) error {

return fs.WalkDir(root, ".", func(path string, info fs.DirEntry, err error) error {
if err != nil {
return walkFn(path, nil, err)
return fn(root, path, err)
}
// avoid validating a directory, an .indexignore file, or any file that matches
// an ignore pattern outlined in a .indexignore file.
if info.IsDir() || info.Name() == indexIgnoreFilename || matcher.Match(path, false) {
return nil
}

cfg, err := LoadFile(root, path)
if err != nil {
return walkFn(path, cfg, err)
}

return walkFn(path, cfg, err)
return fn(root, path, nil)
})
}

Expand Down Expand Up @@ -123,46 +171,38 @@ func extractCSV(objs []string) string {
// Path references will not be de-referenced so callers are responsible for de-referencing if necessary.
func LoadReader(r io.Reader) (*DeclarativeConfig, error) {
cfg := &DeclarativeConfig{}
dec := yaml.NewYAMLOrJSONDecoder(r, 4096)
for {
doc := json.RawMessage{}
if err := dec.Decode(&doc); err != nil {
if errors.Is(err, io.EOF) {
break
}
return nil, err
}
doc = []byte(strings.NewReplacer(`\u003c`, "<", `\u003e`, ">", `\u0026`, "&").Replace(string(doc)))

var in Meta
if err := json.Unmarshal(doc, &in); err != nil {
return nil, fmt.Errorf("unmarshal error: %s", resolveUnmarshalErr(doc, err))
if err := WalkMetasReader(r, func(in *Meta, err error) error {
if err != nil {
return err
}

switch in.Schema {
case SchemaPackage:
var p Package
if err := json.Unmarshal(doc, &p); err != nil {
return nil, fmt.Errorf("parse package: %v", err)
if err := json.Unmarshal(in.Blob, &p); err != nil {
return fmt.Errorf("parse package: %v", err)
}
cfg.Packages = append(cfg.Packages, p)
case SchemaChannel:
var c Channel
if err := json.Unmarshal(doc, &c); err != nil {
return nil, fmt.Errorf("parse channel: %v", err)
if err := json.Unmarshal(in.Blob, &c); err != nil {
return fmt.Errorf("parse channel: %v", err)
}
cfg.Channels = append(cfg.Channels, c)
case SchemaBundle:
var b Bundle
if err := json.Unmarshal(doc, &b); err != nil {
return nil, fmt.Errorf("parse bundle: %v", err)
if err := json.Unmarshal(in.Blob, &b); err != nil {
return fmt.Errorf("parse bundle: %v", err)
}
cfg.Bundles = append(cfg.Bundles, b)
case "":
return nil, fmt.Errorf("object '%s' is missing root schema field", string(doc))
return fmt.Errorf("object '%s' is missing root schema field", string(in.Blob))
default:
cfg.Others = append(cfg.Others, in)
cfg.Others = append(cfg.Others, *in)
}
return nil
}); err != nil {
return nil, err
}
return cfg, nil
}
Expand All @@ -187,47 +227,3 @@ func LoadFile(root fs.FS, path string) (*DeclarativeConfig, error) {

return cfg, nil
}

func resolveUnmarshalErr(data []byte, err error) string {
var te *json.UnmarshalTypeError
if errors.As(err, &te) {
return formatUnmarshallErrorString(data, te.Error(), te.Offset)
}
var se *json.SyntaxError
if errors.As(err, &se) {
return formatUnmarshallErrorString(data, se.Error(), se.Offset)
}
return err.Error()
}

func formatUnmarshallErrorString(data []byte, errmsg string, offset int64) string {
sb := new(strings.Builder)
_, _ = sb.WriteString(fmt.Sprintf("%s at offset %d (indicated by <==)\n ", errmsg, offset))
// attempt to present the erroneous JSON in indented, human-readable format
// errors result in presenting the original, unformatted output
var pretty bytes.Buffer
err := json.Indent(&pretty, data, "", " ")
if err == nil {
pString := pretty.String()
// calc the prettified string offset which correlates to the original string offset
var pOffset, origOffset int64
origOffset = 0
for origOffset = 0; origOffset < offset; {
pOffset++
Comment thread
joelanford marked this conversation as resolved.
if pString[pOffset] != '\n' && pString[pOffset] != ' ' {
origOffset++
}
}
_, _ = sb.WriteString(pString[:pOffset])
_, _ = sb.WriteString(" <== ")
_, _ = sb.WriteString(pString[pOffset:])
} else {
for i := int64(0); i < offset; i++ {
_ = sb.WriteByte(data[i])
}
_, _ = sb.WriteString(" <== ")
_, _ = sb.Write(data[offset:])
}

return sb.String()
}
67 changes: 67 additions & 0 deletions alpha/declcfg/load_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,73 @@ func TestLoadReader(t *testing.T) {
}
}

func TestWalkMetasFS(t *testing.T) {
type spec struct {
name string
fsys fs.FS
assertion require.ErrorAssertionFunc
expectNumPackages int
expectNumChannels int
expectNumBundles int
expectNumOthers int
}
specs := []spec{
{
name: "Error/NilFS",
fsys: nil,
assertion: require.Error,
},
{
name: "Error/NonExistentDir",
fsys: os.DirFS("non/existent/dir/"),
assertion: require.Error,
},
{
name: "Error/Invalid",
fsys: invalidFS,
assertion: require.Error,
},
{
name: "Success/ValidDir",
fsys: validFS,
assertion: require.NoError,
expectNumPackages: 3,
Comment thread
stevekuznetsov marked this conversation as resolved.
expectNumChannels: 0,
expectNumBundles: 12,
expectNumOthers: 1,
},
}

for _, s := range specs {
t.Run(s.name, func(t *testing.T) {
numPackages, numChannels, numBundles, numOthers := 0, 0, 0, 0
err := WalkMetasFS(s.fsys, func(path string, meta *Meta, err error) error {
if err != nil {
return err
}
switch meta.Schema {
case SchemaPackage:
numPackages++
case SchemaChannel:
numChannels++
case SchemaBundle:
numBundles++
default:
numOthers++
}
return nil
})
s.assertion(t, err)
if err == nil {
assert.Equal(t, s.expectNumPackages, numPackages, "unexpected package count")
assert.Equal(t, s.expectNumChannels, numChannels, "unexpected channel count")
assert.Equal(t, s.expectNumBundles, numBundles, "unexpected bundle count")
assert.Equal(t, s.expectNumOthers, numOthers, "unexpected others count")
}
})
}
}

func TestLoadFS(t *testing.T) {
type spec struct {
name string
Expand Down
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ require (
github.com/spf13/cobra v1.6.0
github.com/stretchr/testify v1.8.0
go.etcd.io/bbolt v1.3.6
go4.org v0.0.0-20230225012048-214862532bf5
golang.org/x/mod v0.6.0
golang.org/x/net v0.7.0
golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4
Expand Down
Loading