Addressing more comments

matcher/ruleset_test.go

@@ -170,6 +170,7 @@ func (r mockRuleSet) Version() int                            { return r.version
 func (r mockRuleSet) CutoverNanos() int64                     { return r.cutoverNanos }
 func (r mockRuleSet) Tombstoned() bool                        { return r.tombstoned }
 func (r mockRuleSet) ActiveSet(timeNanos int64) rules.Matcher { return r.matcher }
+func (r mockRuleSet) ToMutableRuleSet() rules.MutableRuleSet  { return nil }
 
 func testRuleSet() (kv.Store, Cache, *ruleSet, Options) {
 	store := mem.NewStore()

rules/mapping.go

@@ -62,14 +62,15 @@ func newMappingRuleSnapshot(
 	if err != nil {
 		return nil, err
 	}
-	return &mappingRuleSnapshot{
-		name:         r.Name,
-		tombstoned:   r.Tombstoned,
-		cutoverNanos: r.CutoverTime,
-		filter:       filter,
-		policies:     policies,
-		rawFilters:   r.TagFilters,
-	}, nil
+
+	return newMappingRuleSnapshotFromFields(
+		r.Name,
+		r.Tombstoned,
+		r.CutoverTime,
+		r.TagFilters,
+		policies,
+		filter,
+	), nil
 }
 
 func newMappingRuleSnapshotFromFields(
@@ -78,13 +79,15 @@ func newMappingRuleSnapshotFromFields(
 	cutoverNanos int64,
 	tagFilters map[string]string,
 	policies []policy.Policy,
-) mappingRuleSnapshot {
-	return mappingRuleSnapshot{
+	filter filters.Filter,
+) *mappingRuleSnapshot {
+	return &mappingRuleSnapshot{
 		name:         name,
 		tombstoned:   tombstoned,
 		cutoverNanos: cutoverNanos,
 		policies:     policies,
 		rawFilters:   tagFilters,
+		filter:       filter,
 	}
 }
 
@@ -106,13 +109,14 @@ func newMappingRuleSnapshotJSON(mrs mappingRuleSnapshot) mappingRuleSnapshotJSON
 	}
 }
 
-func (mrsj mappingRuleSnapshotJSON) mappingRuleSnapshot() mappingRuleSnapshot {
+func (mrsj mappingRuleSnapshotJSON) mappingRuleSnapshot() *mappingRuleSnapshot {
 	return newMappingRuleSnapshotFromFields(
 		mrsj.Name,
 		mrsj.Tombstoned,
 		mrsj.CutoverNanos,
 		mrsj.TagFilters,
 		mrsj.Policies,
+		nil,
 	)
 }
 
@@ -199,16 +203,18 @@ func (mc *mappingRule) addSnapshot(
 	rawFilters map[string]string,
 	policies []policy.Policy,
 	cutoverTime int64,
+
 ) error {
 	snapshot := newMappingRuleSnapshotFromFields(
 		name,
 		false,
 		cutoverTime,
 		rawFilters,
 		policies,
+		nil,
 	)
 
-	mc.snapshots = append(mc.snapshots, &snapshot)
+	mc.snapshots = append(mc.snapshots, snapshot)
 	return nil
 }
 
@@ -295,8 +301,7 @@ func newMappingRuleJSON(mc mappingRule) mappingRuleJSON {
 func (mrj mappingRuleJSON) mappingRule() mappingRule {
 	snapshots := make([]*mappingRuleSnapshot, len(mrj.Snapshots))
 	for i, s := range mrj.Snapshots {
-		newSnapshot := s.mappingRuleSnapshot()
-		snapshots[i] = &newSnapshot
+		snapshots[i] = s.mappingRuleSnapshot()
 	}
 	return mappingRule{
 		uuid:      mrj.UUID,

rules/namespace.go

@@ -240,9 +240,9 @@ func NewNamespaces(version int, namespaces *schema.Namespaces) (Namespaces, erro
 	}, nil
 }
 
-// Clone creates a deep copy of this namespace
+// Clone creates a deep copy of this namespace.
 func (nss Namespaces) Clone() (Namespaces, error) {
-	// TODO(dgromov): Do an actual deep copy that doesn't rely on .Schema()
+	// TODO(dgromov): Do an actual deep copy that doesn't rely on .Schema().
 	schema, err := nss.Schema()
 	if err != nil {
 		return emptyNamespaces, err

rules/rollup.go

@@ -68,7 +68,7 @@ func newRollupTarget(target *schema.RollupTarget) (RollupTarget, error) {
 	}, nil
 }
 
-// NewRollupTargetFromFields creates a new rollupTarget from a list of it's component fields.
+// NewRollupTargetFromFields creates a new rollupTarget from a list of its component fields.
 func NewRollupTargetFromFields(name string, tags []string, policies []policy.Policy) RollupTarget {
 	return RollupTarget{Name: []byte(name), Tags: bytesArrayFromStringArray(tags), Policies: policies}
 }
@@ -139,7 +139,7 @@ func newRollupRuleSnapshotJSON(rrs rollupRuleSnapshot) rollupRuleSnapshotJSON {
 	}
 }
 
-func (rrsj rollupRuleSnapshotJSON) rollupRuleSnapshot() rollupRuleSnapshot {
+func (rrsj rollupRuleSnapshotJSON) rollupRuleSnapshot() *rollupRuleSnapshot {
 	targets := make([]RollupTarget, len(rrsj.Targets))
 	for i, t := range rrsj.Targets {
 		targets[i] = t.rollupTarget()
@@ -151,6 +151,7 @@ func (rrsj rollupRuleSnapshotJSON) rollupRuleSnapshot() rollupRuleSnapshot {
 		rrsj.CutoverNanos,
 		rrsj.TagFilters,
 		targets,
+		nil,
 	)
 }
 
@@ -204,14 +205,15 @@ func newRollupRuleSnapshot(
 	if err != nil {
 		return nil, err
 	}
-	return &rollupRuleSnapshot{
-		name:         r.Name,
-		tombstoned:   r.Tombstoned,
-		cutoverNanos: r.CutoverTime,
-		filter:       filter,
-		targets:      targets,
-		rawFilters:   r.TagFilters,
-	}, nil
+
+	return newRollupRuleSnapshotFromFields(
+		r.Name,
+		r.Tombstoned,
+		r.CutoverTime,
+		r.TagFilters,
+		targets,
+		filter,
+	), nil
 }
 
 func newRollupRuleSnapshotFromFields(
@@ -220,13 +222,15 @@ func newRollupRuleSnapshotFromFields(
 	cutoverNanos int64,
 	tagFilters map[string]string,
 	targets []RollupTarget,
-) rollupRuleSnapshot {
-	return rollupRuleSnapshot{
+	filter filters.Filter,
+) *rollupRuleSnapshot {
+	return &rollupRuleSnapshot{
 		name:         name,
 		tombstoned:   tombstoned,
 		cutoverNanos: cutoverNanos,
 		targets:      targets,
 		rawFilters:   tagFilters,
+		filter:       filter,
 	}
 }
 
@@ -349,9 +353,10 @@ func (rc *rollupRule) addSnapshot(
 		cutoverTime,
 		rawFilters,
 		rollupTargets,
+		nil,
 	)
 
-	rc.snapshots = append(rc.snapshots, &snapshot)
+	rc.snapshots = append(rc.snapshots, snapshot)
 	return nil
 }
 
@@ -411,8 +416,7 @@ func newRollupRuleJSON(rc rollupRule) rollupRuleJSON {
 func (rrj rollupRuleJSON) rollupRule() rollupRule {
 	snapshots := make([]*rollupRuleSnapshot, len(rrj.Snapshots))
 	for i, s := range rrj.Snapshots {
-		newSnapshot := s.rollupRuleSnapshot()
-		snapshots[i] = &newSnapshot
+		snapshots[i] = s.rollupRuleSnapshot()
 	}
 	return rollupRule{
 		uuid:      rrj.UUID,

rules/ruleset.go

@@ -454,6 +454,9 @@ type RuleSet interface {
 
 	// ActiveSet returns the active ruleset at a given time.
 	ActiveSet(timeNanos int64) Matcher
+
+	// ToMutableRuleSet returns a mutable version of this ruleset.
+	ToMutableRuleSet() MutableRuleSet
 }
 
 type ruleSet struct {
@@ -471,7 +474,8 @@ type ruleSet struct {
 	isRollupIDFn       metricID.MatchIDFn
 }
 
-func newRuleSetFromSchema(version int, rs *schema.RuleSet, opts Options) (*ruleSet, error) {
+// NewRuleSetFromSchema creates a new RuleSet from a schema object.
+func NewRuleSetFromSchema(version int, rs *schema.RuleSet, opts Options) (RuleSet, error) {
 	if rs == nil {
 		return nil, errNilRuleSetSchema
 	}
@@ -508,17 +512,6 @@ func newRuleSetFromSchema(version int, rs *schema.RuleSet, opts Options) (*ruleS
 	}, nil
 }
 
-// NewRuleSetFromSchema creates a new RuleSet from a schema object.
-func NewRuleSetFromSchema(version int, rs *schema.RuleSet, opts Options) (RuleSet, error) {
-	return newRuleSetFromSchema(version, rs, opts)
-}
-
-// NewMutableRuleSetFromSchema creates a new MutableRuleSet from a schema object.
-func NewMutableRuleSetFromSchema(version int, rs *schema.RuleSet) (MutableRuleSet, error) {
-	// Takes a blank Options stuct because none of the mutation functions need the options.
-	return newRuleSetFromSchema(version, rs, NewOptions())
-}
-
 // NewEmptyRuleSet returns an empty ruleset to be used with a new namespace.
 func NewEmptyRuleSet(namespaceName string, meta UpdateMetadata) MutableRuleSet {
 	return &ruleSet{
@@ -560,6 +553,10 @@ func (rs *ruleSet) ActiveSet(timeNanos int64) Matcher {
 	)
 }
 
+func (rs *ruleSet) ToMutableRuleSet() MutableRuleSet {
+	return MutableRuleSet(rs)
+}
+
 // resolvePolicies resolves the conflicts among policies if any, following the rules below:
 // * If two policies have the same resolution but different retention, the one with longer
 // retention period is chosen.
@@ -689,7 +686,7 @@ type MutableRuleSet interface {
 	// Schema returns the schema.Ruleset representation of this ruleset.
 	Schema() (*schema.RuleSet, error)
 
-	// Clone returns a copy of this MutableRuleSet
+	// Clone returns a copy of this MutableRuleSet.
 	Clone() (MutableRuleSet, error)
 
 	// MarshalJSON serializes this RuleSet into JSON.
@@ -758,17 +755,18 @@ func (rs ruleSet) Schema() (*schema.RuleSet, error) {
 }
 
 func (rs ruleSet) Clone() (MutableRuleSet, error) {
-	// TODO(dgromov): Do an actual deep copy that doesn't rely on .Schema()
+	// TODO(dgromov): Do an actual deep copy that doesn't rely on .Schema().
 	schema, err := rs.Schema()
 	if err != nil {
 		return nil, err
 	}
 
-	newRuleSet, err := NewMutableRuleSetFromSchema(rs.version, schema)
+	newRuleSet, err := NewRuleSetFromSchema(rs.version, schema, NewOptions())
 	if err != nil {
 		return nil, err
 	}
-	return newRuleSet, nil
+
+	return newRuleSet.ToMutableRuleSet(), nil
 }
 
 // MarshalJSON returns the JSON encoding of staged policies.
@@ -1147,8 +1145,8 @@ func (rs ruleSet) getRollupRuleByID(id string) (*rollupRule, error) {
 
 // RuleConflictError is returned when a rule modification is made that would conflict with the current state.
 type RuleConflictError struct {
-	ConflictUUID string
-	msg          string
+	ConflictRuleUUID string
+	msg              string
 }
 
 func (e RuleConflictError) Error() string { return e.msg }
@@ -1161,7 +1159,7 @@ func (rs ruleSet) validateMappingRuleUpdate(mrd MappingRuleData) error {
 		if n, err := m.Name(); err != nil {
 			continue
 		} else if n == mrd.Name {
-			return RuleConflictError{msg: fmt.Sprintf("Rule with name: %s already exists", n), ConflictUUID: m.uuid}
+			return RuleConflictError{msg: fmt.Sprintf("Rule with name: %s already exists", n), ConflictRuleUUID: m.uuid}
 		}
 	}
 
@@ -1177,7 +1175,7 @@ func (rs ruleSet) validateRollupRuleUpdate(rrd RollupRuleData) error {
 		if n, err := r.Name(); err != nil {
 			continue
 		} else if n == rrd.Name {
-			return RuleConflictError{msg: fmt.Sprintf("Rule with name: %s already exists", n), ConflictUUID: r.uuid}
+			return RuleConflictError{msg: fmt.Sprintf("Rule with name: %s already exists", n), ConflictRuleUUID: r.uuid}
 		}
 
 		if len(r.snapshots) == 0 {
@@ -1187,7 +1185,7 @@ func (rs ruleSet) validateRollupRuleUpdate(rrd RollupRuleData) error {
 		for _, t1 := range latestSnapshot.targets {
 			for _, t2 := range rrd.Targets {
 				if t1.sameTransform(t2) {
-					return RuleConflictError{msg: fmt.Sprintf("Same rollup transformation: %s: %v already exists", t1.Name, t1.Tags), ConflictUUID: r.uuid}
+					return RuleConflictError{msg: fmt.Sprintf("Same rollup transformation: %s: %v already exists", t1.Name, t1.Tags), ConflictRuleUUID: r.uuid}
 				}
 			}
 		}

rules/ruleset_test.go

@@ -646,7 +646,7 @@ func TestRuleSetMarshalJSON(t *testing.T) {
 
 func TestRuleSetUnmarshalJSON(t *testing.T) {
 	version := 1
-	newRuleSet, err := NewMutableRuleSetFromSchema(version, marshalTestSchema)
+	newRuleSet, err := newMutableRuleSetFromSchema(version, marshalTestSchema)
 	require.NoError(t, err)
 
 	data, err := json.Marshal(newRuleSet)
@@ -678,7 +678,7 @@ func TestRuleSetSchema(t *testing.T) {
 		RollupRules:   testRollupRulesConfig(),
 	}
 
-	rs, err := NewMutableRuleSetFromSchema(version, expectedRs)
+	rs, err := newMutableRuleSetFromSchema(version, expectedRs)
 	require.NoError(t, err)
 	res, err := rs.Schema()
 	require.NoError(t, err)
@@ -2199,11 +2199,22 @@ func initMutableTest() (MutableRuleSet, *ruleSet, RuleSetUpdateHelper, error) {
 		RollupRules:   testRollupRulesConfig(),
 	}
 
-	mutable, err := NewMutableRuleSetFromSchema(version, expectedRs)
+	mutable, err := newMutableRuleSetFromSchema(version, expectedRs)
 	rs := mutable.(*ruleSet)
 	return mutable, rs, NewRuleSetUpdateHelper(10), err
 }
 
+// NewMutableRuleSetFromSchema creates a new MutableRuleSet from a schema object.
+func newMutableRuleSetFromSchema(version int, rs *schema.RuleSet) (MutableRuleSet, error) {
+	// Takes a blank Options stuct because none of the mutation functions need the options.
+	roRuleSet, err := NewRuleSetFromSchema(version, rs, NewOptions())
+	if err != nil {
+		return nil, err
+	}
+	rawRs := roRuleSet.(*ruleSet)
+	return MutableRuleSet(rawRs), nil
+}
+
 func TestAddMappingRule(t *testing.T) {
 	mutable, rs, helper, err := initMutableTest()
 	require.NoError(t, err)

rules/store.go

@@ -45,7 +45,7 @@ type Store interface {
 	ReadNamespaces() (*Namespaces, error)
 
 	// ReadRuleSet returns the version and the persisted ruleset data in kv store.
-	ReadRuleSet(namespaceName string) (MutableRuleSet, error)
+	ReadRuleSet(namespaceName string) (RuleSet, error)
 }
 
 // StoreOptions is a configuration for the rules r/w store.
@@ -88,7 +88,7 @@ func (s store) ReadNamespaces() (*Namespaces, error) {
 	return &nss, err
 }
 
-func (s store) ReadRuleSet(nsName string) (MutableRuleSet, error) {
+func (s store) ReadRuleSet(nsName string) (RuleSet, error) {
 	ruleSetKey := s.ruleSetKey(nsName)
 	value, err := s.kvStore.Get(ruleSetKey)
 
@@ -101,7 +101,8 @@ func (s store) ReadRuleSet(nsName string) (MutableRuleSet, error) {
 	if err := value.Unmarshal(&ruleSet); err != nil {
 		return nil, fmt.Errorf("Could not fetch RuleSet %s: %v", nsName, err.Error())
 	}
-	rs, err := NewMutableRuleSetFromSchema(version, &ruleSet)
+
+	rs, err := NewRuleSetFromSchema(version, &ruleSet, NewOptions())
 	if err != nil {
 		return nil, fmt.Errorf("Could not fetch RuleSet %s: %v", nsName, err.Error())
 	}