Repository: spf13/viper
Pull request #986 (2020-09-23 16:07:19)

New encoding layer
This PR adds an encoding layer to Viper, separating direct calls to encoding libraries to different packages and ultimately moving out most of the logic from the main Viper package.

The package implements the proposal from #888 with the exception of moving them out from the module to preserve backwards compatibility. That cannot happen in the v1 branch as it would be a breaking change, but introducing an encoding layer this way allows us to move things out of the module easily in the future.

Although this is a sane proposal and I believe it's the way forward, the abstraction is not perfect and we can already see some shortcomings:

- TOML does not support `map[string]interface{}` well (compared to JSON and YAML anyway), so it contains some conditional logic specific to that type.
- Many supported formats (INI, dotenv, Java properties) don't have a JSON/YAML style "unmarshal" API. In fact, some of them are completely different:
    - In case of dotenv, the env var names are generated by formatting the keys
    - In case of INI sections are created from keys

All this points towards a much higher level abstraction where we pass and return `map[string]interface{}` directly. The current implementation is a low level interface based on the Marshal/Unmarshal style of `encoding/json`.

Something like this would probably be better:

```go
// Decoder decodes the contents of b into a map[string]interface{}.
type Decoder interface {
	Decode(b []byte) (map[string]interface{}, error)
}

// Encoder encodes the contents of v into a raw byte representation.
type Encoder interface {
	Encode(v map[string]interface{}) ([]byte, error)
}
```

There are also some other missing pieces from this PR:
- Proper tests for the different codecs (currently we rely on Viper's tests which are...not necessarily good)
- Some codecs might have additional config options

I'm not sure whether this change should make its way to Viper as it is. On one hand it would be nice to start experimenting, on the other hand this is not a perfect solution and will require some changes for the long-term. I'll leave this PR here for now, wait for some potential feedback and merge it if it still feels right after a few days/weeks.


sagikazarmark: I'm going to merge this PR for now. I've created a note in the Viper 2 board with the next steps (will probably convert it to an issue).

Patch:
diff --git a/internal/encoding/decoder.go b/internal/encoding/decoder.go
new file mode 100644
index 0000000..08b1bb6
--- /dev/null
+++ b/internal/encoding/decoder.go
@@ -0,0 +1,61 @@
+package encoding
+
+import (
+	"sync"
+)
+
+// Decoder decodes the contents of b into a v representation.
+// It's primarily used for decoding contents of a file into a map[string]interface{}.
+type Decoder interface {
+	Decode(b []byte, v interface{}) error
+}
+
+const (
+	// ErrDecoderNotFound is returned when there is no decoder registered for a format.
+	ErrDecoderNotFound = encodingError("decoder not found for this format")
+
+	// ErrDecoderFormatAlreadyRegistered is returned when an decoder is already registered for a format.
+	ErrDecoderFormatAlreadyRegistered = encodingError("decoder already registered for this format")
+)
+
+// DecoderRegistry can choose an appropriate Decoder based on the provided format.
+type DecoderRegistry struct {
+	decoders map[string]Decoder
+
+	mu sync.RWMutex
+}
+
+// NewDecoderRegistry returns a new, initialized DecoderRegistry.
+func NewDecoderRegistry() *DecoderRegistry {
+	return &DecoderRegistry{
+		decoders: make(map[string]Decoder),
+	}
+}
+
+// RegisterDecoder registers a Decoder for a format.
+// Registering a Decoder for an already existing format is not supported.
+func (e *DecoderRegistry) RegisterDecoder(format string, enc Decoder) error {
+	e.mu.Lock()
+	defer e.mu.Unlock()
+
+	if _, ok := e.decoders[format]; ok {
+		return ErrDecoderFormatAlreadyRegistered
+	}
+
+	e.decoders[format] = enc
+
+	return nil
+}
+
+// Decode calls the underlying Decoder based on the format.
+func (e *DecoderRegistry) Decode(format string, b []byte, v interface{}) error {
+	e.mu.RLock()
+	decoder, ok := e.decoders[format]
+	e.mu.RUnlock()
+
+	if !ok {
+		return ErrDecoderNotFound
+	}
+
+	return decoder.Decode(b, v)
+}
diff --git a/internal/encoding/encoder.go b/internal/encoding/encoder.go
new file mode 100644
index 0000000..82c7996
--- /dev/null
+++ b/internal/encoding/encoder.go
@@ -0,0 +1,60 @@
+package encoding
+
+import (
+	"sync"
+)
+
+// Encoder encodes the contents of v into a byte representation.
+// It's primarily used for encoding a map[string]interface{} into a file format.
+type Encoder interface {
+	Encode(v interface{}) ([]byte, error)
+}
+
+const (
+	// ErrEncoderNotFound is returned when there is no encoder registered for a format.
+	ErrEncoderNotFound = encodingError("encoder not found for this format")
+
+	// ErrEncoderFormatAlreadyRegistered is returned when an encoder is already registered for a format.
+	ErrEncoderFormatAlreadyRegistered = encodingError("encoder already registered for this format")
+)
+
+// EncoderRegistry can choose an appropriate Encoder based on the provided format.
+type EncoderRegistry struct {
+	encoders map[string]Encoder
+
+	mu sync.RWMutex
+}
+
+// NewEncoderRegistry returns a new, initialized EncoderRegistry.
+func NewEncoderRegistry() *EncoderRegistry {
+	return &EncoderRegistry{
+		encoders: make(map[string]Encoder),
+	}
+}
+
+// RegisterEncoder registers an Encoder for a format.
+// Registering a Encoder for an already existing format is not supported.
+func (e *EncoderRegistry) RegisterEncoder(format string, enc Encoder) error {
+	e.mu.Lock()
+	defer e.mu.Unlock()
+
+	if _, ok := e.encoders[format]; ok {
+		return ErrEncoderFormatAlreadyRegistered
+	}
+
+	e.encoders[format] = enc
+
+	return nil
+}
+
+func (e *EncoderRegistry) Encode(format string, v interface{}) ([]byte, error) {
+	e.mu.RLock()
+	encoder, ok := e.encoders[format]
+	e.mu.RUnlock()
+
+	if !ok {
+		return nil, ErrEncoderNotFound
+	}
+
+	return encoder.Encode(v)
+}
diff --git a/internal/encoding/error.go b/internal/encoding/error.go
new file mode 100644
index 0000000..e4cde02
--- /dev/null
+++ b/internal/encoding/error.go
@@ -0,0 +1,7 @@
+package encoding
+
+type encodingError string
+
+func (e encodingError) Error() string {
+	return string(e)
+}
diff --git a/internal/encoding/hcl/codec.go b/internal/encoding/hcl/codec.go
new file mode 100644
index 0000000..f3e4ab1
--- /dev/null
+++ b/internal/encoding/hcl/codec.go
@@ -0,0 +1,40 @@
+package hcl
+
+import (
+	"bytes"
+	"encoding/json"
+
+	"github.com/hashicorp/hcl"
+	"github.com/hashicorp/hcl/hcl/printer"
+)
+
+// Codec implements the encoding.Encoder and encoding.Decoder interfaces for HCL encoding.
+// TODO: add printer config to the codec?
+type Codec struct{}
+
+func (Codec) Encode(v interface{}) ([]byte, error) {
+	b, err := json.Marshal(v)
+	if err != nil {
+		return nil, err
+	}
+
+	// TODO: use printer.Format? Is the trailing newline an issue?
+
+	ast, err := hcl.Parse(string(b))
+	if err != nil {
+		return nil, err
+	}
+
+	var buf bytes.Buffer
+
+	err = printer.Fprint(&buf, ast.Node)
+	if err != nil {
+		return nil, err
+	}
+
+	return buf.Bytes(), nil
+}
+
+func (Codec) Decode(b []byte, v interface{}) error {
+	return hcl.Unmarshal(b, v)
+}
diff --git a/internal/encoding/json/codec.go b/internal/encoding/json/codec.go
new file mode 100644
index 0000000..dff9ec9
--- /dev/null
+++ b/internal/encoding/json/codec.go
@@ -0,0 +1,17 @@
+package json
+
+import (
+	"encoding/json"
+)
+
+// Codec implements the encoding.Encoder and encoding.Decoder interfaces for JSON encoding.
+type Codec struct{}
+
+func (Codec) Encode(v interface{}) ([]byte, error) {
+	// TODO: expose prefix and indent in the Codec as setting?
+	return json.MarshalIndent(v, "", "  ")
+}
+
+func (Codec) Decode(b []byte, v interface{}) error {
+	return json.Unmarshal(b, v)
+}
diff --git a/internal/encoding/toml/codec.go b/internal/encoding/toml/codec.go
new file mode 100644
index 0000000..c043802
--- /dev/null
+++ b/internal/encoding/toml/codec.go
@@ -0,0 +1,45 @@
+package toml
+
+import (
+	"github.com/pelletier/go-toml"
+)
+
+// Codec implements the encoding.Encoder and encoding.Decoder interfaces for TOML encoding.
+type Codec struct{}
+
+func (Codec) Encode(v interface{}) ([]byte, error) {
+	if m, ok := v.(map[string]interface{}); ok {
+		t, err := toml.TreeFromMap(m)
+		if err != nil {
+			return nil, err
+		}
+
+		s, err := t.ToTomlString()
+		if err != nil {
+			return nil, err
+		}
+
+		return []byte(s), nil
+	}
+
+	return toml.Marshal(v)
+}
+
+func (Codec) Decode(b []byte, v interface{}) error {
+	tree, err := toml.LoadBytes(b)
+	if err != nil {
+		return err
+	}
+
+	if m, ok := v.(*map[string]interface{}); ok {
+		vmap := *m
+		tmap := tree.ToMap()
+		for k, v := range tmap {
+			vmap[k] = v
+		}
+
+		return nil
+	}
+
+	return tree.Unmarshal(v)
+}
diff --git a/internal/encoding/yaml/codec.go b/internal/encoding/yaml/codec.go
new file mode 100644
index 0000000..f94b269
--- /dev/null
+++ b/internal/encoding/yaml/codec.go
@@ -0,0 +1,14 @@
+package yaml
+
+import "gopkg.in/yaml.v2"
+
+// Codec implements the encoding.Encoder and encoding.Decoder interfaces for YAML encoding.
+type Codec struct{}
+
+func (Codec) Encode(v interface{}) ([]byte, error) {
+	return yaml.Marshal(v)
+}
+
+func (Codec) Decode(b []byte, v interface{}) error {
+	return yaml.Unmarshal(b, v)
+}
diff --git a/viper.go b/viper.go
index e8c0462..46b1a85 100644
--- a/viper.go
+++ b/viper.go
@@ -22,7 +22,6 @@ package viper
 import (
 	"bytes"
 	"encoding/csv"
-	"encoding/json"
 	"errors"
 	"fmt"
 	"io"
@@ -36,18 +35,20 @@ import (
 	"time"
 
 	"github.com/fsnotify/fsnotify"
-	"github.com/hashicorp/hcl"
-	"github.com/hashicorp/hcl/hcl/printer"
 	"github.com/magiconair/properties"
 	"github.com/mitchellh/mapstructure"
-	"github.com/pelletier/go-toml"
 	"github.com/spf13/afero"
 	"github.com/spf13/cast"
 	jww "github.com/spf13/jwalterweatherman"
 	"github.com/spf13/pflag"
 	"github.com/subosito/gotenv"
 	"gopkg.in/ini.v1"
-	"gopkg.in/yaml.v2"
+
+	"github.com/spf13/viper/internal/encoding"
+	"github.com/spf13/viper/internal/encoding/hcl"
+	"github.com/spf13/viper/internal/encoding/json"
+	"github.com/spf13/viper/internal/encoding/toml"
+	"github.com/spf13/viper/internal/encoding/yaml"
 )
 
 // ConfigMarshalError happens when failing to marshal the configuration.
@@ -67,8 +68,44 @@ type RemoteResponse struct {
 	Error error
 }
 
+var (
+	encoderRegistry = encoding.NewEncoderRegistry()
+	decoderRegistry = encoding.NewDecoderRegistry()
+)
+
 func init() {
 	v = New()
+
+	{
+		codec := yaml.Codec{}
+
+		encoderRegistry.RegisterEncoder("yaml", codec)
+		decoderRegistry.RegisterDecoder("yaml", codec)
+
+		encoderRegistry.RegisterEncoder("yml", codec)
+		decoderRegistry.RegisterDecoder("yml", codec)
+	}
+
+	{
+		codec := json.Codec{}
+
+		encoderRegistry.RegisterEncoder("json", codec)
+		decoderRegistry.RegisterDecoder("json", codec)
+	}
+
+	{
+		codec := toml.Codec{}
+
+		encoderRegistry.RegisterEncoder("toml", codec)
+		decoderRegistry.RegisterDecoder("toml", codec)
+	}
+
+	{
+		codec := hcl.Codec{}
+
+		encoderRegistry.RegisterEncoder("hcl", codec)
+		decoderRegistry.RegisterDecoder("hcl", codec)
+	}
 }
 
 type remoteConfigFactory interface {
@@ -1584,35 +1621,12 @@ func (v *Viper) unmarshalReader(in io.Reader, c map[string]interface{}) error {
 	buf := new(bytes.Buffer)
 	buf.ReadFrom(in)
 
-	switch strings.ToLower(v.getConfigType()) {
-	case "yaml", "yml":
-		if err := yaml.Unmarshal(buf.Bytes(), &c); err != nil {
-			return ConfigParseError{err}
-		}
-
-	case "json":
-		if err := json.Unmarshal(buf.Bytes(), &c); err != nil {
-			return ConfigParseError{err}
-		}
-
-	case "hcl":
-		obj, err := hcl.Parse(buf.String())
-		if err != nil {
-			return ConfigParseError{err}
-		}
-		if err = hcl.DecodeObject(&c, obj); err != nil {
-			return ConfigParseError{err}
-		}
-
-	case "toml":
-		tree, err := toml.LoadReader(buf)
+	switch format := strings.ToLower(v.getConfigType()); format {
+	case "yaml", "yml", "json", "toml", "hcl":
+		err := decoderRegistry.Decode(format, buf.Bytes(), &c)
 		if err != nil {
 			return ConfigParseError{err}
 		}
-		tmap := tree.ToMap()
-		for k, v := range tmap {
-			c[k] = v
-		}
 
 	case "dotenv", "env":
 		env, err := gotenv.StrictParse(buf)
@@ -1665,26 +1679,13 @@ func (v *Viper) unmarshalReader(in io.Reader, c map[string]interface{}) error {
 func (v *Viper) marshalWriter(f afero.File, configType string) error {
 	c := v.AllSettings()
 	switch configType {
-	case "json":
-		b, err := json.MarshalIndent(c, "", "  ")
-		if err != nil {
-			return ConfigMarshalError{err}
-		}
-		_, err = f.WriteString(string(b))
+	case "yaml", "yml", "json", "toml", "hcl":
+		b, err := encoderRegistry.Encode(configType, c)
 		if err != nil {
 			return ConfigMarshalError{err}
 		}
 
-	case "hcl":
-		b, err := json.Marshal(c)
-		if err != nil {
-			return ConfigMarshalError{err}
-		}
-		ast, err := hcl.Parse(string(b))
-		if err != nil {
-			return ConfigMarshalError{err}
-		}
-		err = printer.Fprint(f, ast.Node)
+		_, err = f.WriteString(string(b))
 		if err != nil {
 			return ConfigMarshalError{err}
 		}
@@ -1717,25 +1718,6 @@ func (v *Viper) marshalWriter(f afero.File, configType string) error {
 			return ConfigMarshalError{err}
 		}
 
-	case "toml":
-		t, err := toml.TreeFromMap(c)
-		if err != nil {
-			return ConfigMarshalError{err}
-		}
-		s := t.String()
-		if _, err := f.WriteString(s); err != nil {
-			return ConfigMarshalError{err}
-		}
-
-	case "yaml", "yml":
-		b, err := yaml.Marshal(c)
-		if err != nil {
-			return ConfigMarshalError{err}
-		}
-		if _, err = f.WriteString(string(b)); err != nil {
-			return ConfigMarshalError{err}
-		}
-
 	case "ini":
 		keys := v.AllKeys()
 		cfg := ini.Empty()


Test patch:
diff --git a/internal/encoding/decoder_test.go b/internal/encoding/decoder_test.go
new file mode 100644
index 0000000..80e6688
--- /dev/null
+++ b/internal/encoding/decoder_test.go
@@ -0,0 +1,77 @@
+package encoding
+
+import (
+	"testing"
+)
+
+type decoder struct {
+	v interface{}
+}
+
+func (d decoder) Decode(_ []byte, v interface{}) error {
+	rv := v.(*string)
+	*rv = d.v.(string)
+
+	return nil
+}
+
+func TestDecoderRegistry_RegisterDecoder(t *testing.T) {
+	t.Run("OK", func(t *testing.T) {
+		registry := NewDecoderRegistry()
+
+		err := registry.RegisterDecoder("myformat", decoder{})
+		if err != nil {
+			t.Fatal(err)
+		}
+	})
+
+	t.Run("AlreadyRegistered", func(t *testing.T) {
+		registry := NewDecoderRegistry()
+
+		err := registry.RegisterDecoder("myformat", decoder{})
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		err = registry.RegisterDecoder("myformat", decoder{})
+		if err != ErrDecoderFormatAlreadyRegistered {
+			t.Fatalf("expected ErrDecoderFormatAlreadyRegistered, got: %v", err)
+		}
+	})
+}
+
+func TestDecoderRegistry_Decode(t *testing.T) {
+	t.Run("OK", func(t *testing.T) {
+		registry := NewDecoderRegistry()
+		decoder := decoder{
+			v: "decoded value",
+		}
+
+		err := registry.RegisterDecoder("myformat", decoder)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		var v string
+
+		err = registry.Decode("myformat", []byte("some value"), &v)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if v != "decoded value" {
+			t.Fatalf("expected 'decoded value', got: %#v", v)
+		}
+	})
+
+	t.Run("DecoderNotFound", func(t *testing.T) {
+		registry := NewDecoderRegistry()
+
+		var v string
+
+		err := registry.Decode("myformat", []byte("some value"), &v)
+		if err != ErrDecoderNotFound {
+			t.Fatalf("expected ErrDecoderNotFound, got: %v", err)
+		}
+	})
+}
diff --git a/internal/encoding/encoder_test.go b/internal/encoding/encoder_test.go
new file mode 100644
index 0000000..e2472ad
--- /dev/null
+++ b/internal/encoding/encoder_test.go
@@ -0,0 +1,70 @@
+package encoding
+
+import (
+	"testing"
+)
+
+type encoder struct {
+	b []byte
+}
+
+func (e encoder) Encode(_ interface{}) ([]byte, error) {
+	return e.b, nil
+}
+
+func TestEncoderRegistry_RegisterEncoder(t *testing.T) {
+	t.Run("OK", func(t *testing.T) {
+		registry := NewEncoderRegistry()
+
+		err := registry.RegisterEncoder("myformat", encoder{})
+		if err != nil {
+			t.Fatal(err)
+		}
+	})
+
+	t.Run("AlreadyRegistered", func(t *testing.T) {
+		registry := NewEncoderRegistry()
+
+		err := registry.RegisterEncoder("myformat", encoder{})
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		err = registry.RegisterEncoder("myformat", encoder{})
+		if err != ErrEncoderFormatAlreadyRegistered {
+			t.Fatalf("expected ErrEncoderFormatAlreadyRegistered, got: %v", err)
+		}
+	})
+}
+
+func TestEncoderRegistry_Decode(t *testing.T) {
+	t.Run("OK", func(t *testing.T) {
+		registry := NewEncoderRegistry()
+		encoder := encoder{
+			b: []byte("encoded value"),
+		}
+
+		err := registry.RegisterEncoder("myformat", encoder)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		b, err := registry.Encode("myformat", "some value")
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		if string(b) != "encoded value" {
+			t.Fatalf("expected 'encoded value', got: %#v", string(b))
+		}
+	})
+
+	t.Run("EncoderNotFound", func(t *testing.T) {
+		registry := NewEncoderRegistry()
+
+		_, err := registry.Encode("myformat", "some value")
+		if err != ErrEncoderNotFound {
+			t.Fatalf("expected ErrEncoderNotFound, got: %v", err)
+		}
+	})
+}