Repository: casbin/casbin
Pull request #340 (2019-12-07 16:01:48)

LoadPolicyLine Required Section Check
For #333 this PR checks for the existence of r,p,e and m model sections, returning an error when loading if any of those is not present and indicates which required sections are missing.

This also has a minor typo correction in a comment and a move of the empty string check in model.AddDef().


Patch:
diff --git a/config/config.go b/config/config.go
index f188dfc..5398edf 100644
--- a/config/config.go
+++ b/config/config.go
@@ -37,7 +37,7 @@ var (
 	DEFAULT_MULTI_LINE_SEPARATOR = []byte{'\\'}
 )
 
-// ConfigInterface defines the behavior of a Config implemenation
+// ConfigInterface defines the behavior of a Config implementation
 type ConfigInterface interface {
 	String(key string) string
 	Strings(key string) []string
diff --git a/model/model.go b/model/model.go
index 753830f..bdf1cc0 100644
--- a/model/model.go
+++ b/model/model.go
@@ -15,6 +15,7 @@
 package model
 
 import (
+	"fmt"
 	"strconv"
 	"strings"
 
@@ -37,6 +38,9 @@ var sectionNameMap = map[string]string{
 	"m": "matchers",
 }
 
+// Minimal required sections for a model to be valid
+var requiredSections = []string{"r", "p", "e", "m"}
+
 func loadAssertion(model Model, cfg config.ConfigInterface, sec string, key string) bool {
 	value := cfg.String(sectionNameMap[sec] + "::" + key)
 	return model.AddDef(sec, key, value)
@@ -44,14 +48,14 @@ func loadAssertion(model Model, cfg config.ConfigInterface, sec string, key stri
 
 // AddDef adds an assertion to the model.
 func (model Model) AddDef(sec string, key string, value string) bool {
+	if value == "" {
+		return false
+	}
+
 	ast := Assertion{}
 	ast.Key = key
 	ast.Value = value
 
-	if ast.Value == "" {
-		return false
-	}
-
 	if sec == "r" || sec == "p" {
 		ast.Tokens = strings.Split(ast.Value, ", ")
 		for i := range ast.Tokens {
@@ -140,16 +144,26 @@ func (model Model) LoadModelFromText(text string) error {
 }
 
 func (model Model) loadModelFromConfig(cfg config.ConfigInterface) error {
-	loadSection(model, cfg, "r")
-	loadSection(model, cfg, "p")
-	loadSection(model, cfg, "e")
-	loadSection(model, cfg, "m")
-
-	loadSection(model, cfg, "g")
-
+	for s := range sectionNameMap {
+		loadSection(model, cfg, s)
+	}
+	ms := make([]string, 0)
+	for _, rs := range requiredSections {
+		if !model.hasSection(rs) {
+			ms = append(ms, sectionNameMap[rs])
+		}
+	}
+	if len(ms) > 0 {
+		return fmt.Errorf("missing required sections: %s", strings.Join(ms, ","))
+	}
 	return nil
 }
 
+func (model Model) hasSection(sec string) bool {
+	section := model[sec]
+	return section != nil
+}
+
 // PrintModel prints the model to the log.
 func (model Model) PrintModel() {
 	log.LogPrint("Model:")


Test patch:
diff --git a/model/model_test.go b/model/model_test.go
index ebc8bf3..bfce615 100644
--- a/model/model_test.go
+++ b/model/model_test.go
@@ -1,9 +1,124 @@
+// Copyright 2019 The casbin Authors. All Rights Reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
 package model
 
 import (
+	"github.com/casbin/casbin/v2/config"
+	"io/ioutil"
+	"path/filepath"
+	"strings"
 	"testing"
 )
 
-func TestModel(t *testing.T) {
-	//No tests yet
+var (
+	basicExample = filepath.Join("..", "examples", "basic_model.conf")
+	basicConfig  = &MockConfig{
+		data: map[string]string{
+			"request_definition::r":"sub, obj, act",
+			"policy_definition::p":"sub, obj, act",
+			"policy_effect::e":"some(where (p.eft == allow))",
+			"matchers::m":"r.sub == p.sub && r.obj == p.obj && r.act == p.act",
+		},
+	}
+)
+
+type MockConfig struct {
+	data map[string]string
+	config.ConfigInterface
+}
+
+func (mc *MockConfig) String(key string) string {
+	return mc.data[key]
+}
+
+func TestNewModel(t *testing.T) {
+	m := NewModel()
+	if m == nil {
+		t.Error("new model should not be nil")
+	}
+}
+
+func TestNewModelFromFile(t *testing.T) {
+	m, err := NewModelFromFile(basicExample)
+	if err != nil {
+		t.Errorf("model failed to load from file: %s", err)
+	}
+	if m == nil {
+		t.Error("model should not be nil")
+	}
+}
+
+func TestNewModelFromString(t *testing.T) {
+	modelBytes, _ := ioutil.ReadFile(basicExample)
+	modelString := string(modelBytes)
+	m, err := NewModelFromString(modelString)
+	if err != nil {
+		t.Errorf("model faild to load from string: %s", err)
+	}
+	if m == nil {
+		t.Error("model should not be nil")
+	}
+}
+
+func TestLoadModelFromConfig(t *testing.T) {
+	m := NewModel()
+	err := m.loadModelFromConfig(basicConfig)
+	if err != nil {
+		t.Error("basic config should not return an error")
+	}
+	m = NewModel()
+	err = m.loadModelFromConfig(&MockConfig{})
+	if err == nil {
+		t.Error("empty config should return error")
+	} else {
+		// check for missing sections in message
+		for _, rs := range requiredSections {
+			if !strings.Contains(err.Error(), sectionNameMap[rs]) {
+				t.Errorf("section name: %s should be in message", sectionNameMap[rs])
+			}
+		}
+	}
+}
+
+func TestHasSection(t *testing.T) {
+	m := NewModel()
+	_ = m.loadModelFromConfig(basicConfig)
+	for _, sec := range requiredSections {
+		if !m.hasSection(sec) {
+			t.Errorf("%s section was expected in model", sec)
+		}
+	}
+	m = NewModel()
+	_ = m.loadModelFromConfig(&MockConfig{})
+	for _, sec := range requiredSections {
+		if m.hasSection(sec) {
+			t.Errorf("%s section was not expected in model", sec)
+		}
+	}
+}
+
+func TestModel_AddDef(t *testing.T) {
+	m := NewModel()
+	s := "r"
+	v := "sub, obj, act"
+	ok := m.AddDef(s, s, v)
+	if !ok {
+		t.Errorf("non empty assertion should be added")
+	}
+	ok = m.AddDef(s, s, "")
+	if ok {
+		t.Errorf("empty assertion value should not be added")
+	}
 }