From 3fda875eee63a21be4c1265ee5e54eed0a1805fe Mon Sep 17 00:00:00 2001
From: Patrick East <east.patrick@gmail.com>
Date: Wed, 18 Sep 2019 19:26:51 -0700
Subject: [PATCH] bundle: Fix for overwriting data file keys

This corrects an issue where data.json (or yaml) files in a bundle
would overwrite data if the file was at the root of the bundle
directory and was loaded after other data files.

This also helps avoid any issues with merging data files that
could have overlapped without any problems.

Fixes: #1763
Signed-off-by: Patrick East <east.patrick@gmail.com>
---
 bundle/bundle.go        | 52 +++++++++++++++++++++++------------------
 bundle/bundle_test.go   | 14 ++++++++---
 internal/merge/merge.go |  2 ++
 3 files changed, 42 insertions(+), 26 deletions(-)

diff --git a/bundle/bundle.go b/bundle/bundle.go
index 1270d97340..0d4a91d118 100644
--- a/bundle/bundle.go
+++ b/bundle/bundle.go
@@ -18,11 +18,13 @@ import (
 	"strings"
 
 	"github.com/open-policy-agent/opa/internal/file/archive"
+	"github.com/open-policy-agent/opa/internal/merge"
+
+	"github.com/pkg/errors"
 
 	"github.com/open-policy-agent/opa/ast"
 	"github.com/open-policy-agent/opa/internal/file"
 	"github.com/open-policy-agent/opa/util"
-	"github.com/pkg/errors"
 )
 
 // Common file extensions and file names.
@@ -327,38 +329,42 @@ func (b Bundle) Equal(other Bundle) bool {
 }
 
 func (b *Bundle) insert(key []string, value interface{}) error {
-	if len(key) == 0 {
-		obj, ok := value.(map[string]interface{})
-		if !ok {
-			return fmt.Errorf("root value must be object")
-		}
-		b.Data = obj
-		return nil
-	}
-
-	obj, err := b.mkdir(key[:len(key)-1])
+	// Build an object with the full structure for the value
+	obj, err := mktree(key, value)
 	if err != nil {
 		return err
 	}
 
-	obj[key[len(key)-1]] = value
+	// Merge the new data in with the current bundle data object
+	merged, ok := merge.InterfaceMaps(b.Data, obj)
+	if !ok {
+		return fmt.Errorf("failed to insert data file from path %s", filepath.Join(key...))
+	}
+
+	b.Data = merged
+
 	return nil
 }
 
-func (b *Bundle) mkdir(key []string) (map[string]interface{}, error) {
-	obj := b.Data
-	for i := 0; i < len(key); i++ {
-		node, ok := obj[key[i]]
+func mktree(path []string, value interface{}) (map[string]interface{}, error) {
+	if len(path) == 0 {
+		// For 0 length path the value is the full tree.
+		obj, ok := value.(map[string]interface{})
 		if !ok {
-			node = map[string]interface{}{}
-			obj[key[i]] = node
-		}
-		obj, ok = node.(map[string]interface{})
-		if !ok {
-			return nil, fmt.Errorf("non-leaf value must be object")
+			return nil, fmt.Errorf("root value must be object")
 		}
+		return obj, nil
 	}
-	return obj, nil
+
+	dir := map[string]interface{}{}
+	for i := len(path) - 1; i > 0; i-- {
+		dir[path[i]] = value
+		value = dir
+		dir = map[string]interface{}{}
+	}
+	dir[path[0]] = value
+
+	return dir, nil
 }
 
 // RootPathsOverlap takes in two bundle root paths and returns
diff --git a/bundle/bundle_test.go b/bundle/bundle_test.go
index 6b1a80450a..64c587985c 100644
--- a/bundle/bundle_test.go
+++ b/bundle/bundle_test.go
@@ -11,9 +11,8 @@ import (
 	"strings"
 	"testing"
 
-	"github.com/open-policy-agent/opa/internal/file/archive"
-
 	"github.com/open-policy-agent/opa/ast"
+	"github.com/open-policy-agent/opa/internal/file/archive"
 )
 
 func TestRead(t *testing.T) {
@@ -23,6 +22,7 @@ func TestRead(t *testing.T) {
 		{"/a/b/d/data.json", "true"},
 		{"/a/b/y/data.yaml", `foo: 1`},
 		{"/example/example.rego", `package example`},
+		{"/data.json", `{"x": {"y": true}, "a": {"b": {"z": true}}}}`},
 	}
 
 	buf := archive.MustWriteTarGz(files)
@@ -42,8 +42,12 @@ func TestRead(t *testing.T) {
 					"y": map[string]interface{}{
 						"foo": json.Number("1"),
 					},
+					"z": true,
 				},
 			},
+			"x": map[string]interface{}{
+				"y": true,
+			},
 		},
 		Modules: []ModuleFile{
 			{
@@ -55,7 +59,7 @@ func TestRead(t *testing.T) {
 	}
 
 	if !exp.Equal(bundle) {
-		t.Fatal("Exp:", exp, "\n\nGot:", bundle)
+		t.Fatal("\nExp:", exp, "\n\nGot:", bundle)
 	}
 }
 
@@ -216,6 +220,10 @@ func TestReadErrorBadContents(t *testing.T) {
 			{"a/b/c/data.json", "true"},
 		}},
 		{[][2]string{{"/test.rego", ""}}},
+		{[][2]string{
+			{"/a/b/data.json", `{"c": "foo"}`},
+			{"/data.json", `{"a": {"b": {"c": [123]}}}`},
+		}},
 	}
 	for _, test := range tests {
 		buf := archive.MustWriteTarGz(test.files)
diff --git a/internal/merge/merge.go b/internal/merge/merge.go
index af0ba94076..fa53236d01 100644
--- a/internal/merge/merge.go
+++ b/internal/merge/merge.go
@@ -2,6 +2,8 @@
 // Use of this source code is governed by an Apache2
 // license that can be found in the LICENSE file.
 
+// Package merge contains helpers to merge data structures
+// frequently encountered in OPA.
 package merge
 
 // InterfaceMaps returns the result of merging a and b. If a and b cannot be