adsfasdf

ovn-org · Oct 14, 2022 · bb4fc64 · bb4fc64
1 parent 0c672cf
commit bb4fc64
Show file tree

Hide file tree

Showing 9 changed files with 198 additions and 80 deletions.
diff --git a/mapper/info.go b/mapper/info.go
@@ -71,20 +71,11 @@ func (i *Info) SetField(column string, value interface{}) error {
 	if colSchema == nil {
 		return fmt.Errorf("SetField: column %s schema not found", column)
 	}
-
-	// Validate set length requirements
-	newVal := reflect.ValueOf(value)
-	if colSchema.Type == ovsdb.TypeSet || colSchema.Type == ovsdb.TypeEnum {
-		maxVal := colSchema.TypeObj.Max()
-		minVal := colSchema.TypeObj.Min()
-		if maxVal > 1 && newVal.Len() > maxVal {
-			return fmt.Errorf("SetField: column %s overflow: %d new elements but max is %d", column, newVal.Len(), maxVal)
-		} else if minVal > 0 && newVal.Len() < minVal {
-			return fmt.Errorf("SetField: column %s underflow: %d new elements but min is %d", column, newVal.Len(), minVal)
-		}
+	if err := ovsdb.ValidateColumnConstraints(colSchema, value); err != nil {
+		return fmt.Errorf("SetField: column %s failed validation: %v", column, err)
 	}
 
-	fieldValue.Set(newVal)
+	fieldValue.Set(reflect.ValueOf(value))
 	return nil
 }
 

diff --git a/mapper/mapper.go b/mapper/mapper.go
@@ -118,6 +118,9 @@ func (m Mapper) NewRow(data *Info, fields ...interface{}) (ovsdb.Row, error) {
 		if len(fields) == 0 && ovsdb.IsDefaultValue(column, nativeElem) {
 			continue
 		}
+		if err := ovsdb.ValidateColumnConstraints(column, nativeElem); err != nil {
+			return nil, fmt.Errorf("column %s assignment failed: %w", column, err)
+		}
 		ovsElem, err := ovsdb.NativeToOvs(column, nativeElem)
 		if err != nil {
 			return nil, fmt.Errorf("table %s, column %s: failed to generate ovs element. %s", data.Metadata.TableName, name, err.Error())

diff --git a/mapper/mapper_test.go b/mapper/mapper_test.go
@@ -40,6 +40,8 @@ var (
 		42.0,
 	}
 
+	aFloatSetTooBig = []float64{1.0, 2.0, 3.0, 4.0, 5.0, 6.0}
+
 	aMap = map[string]string{
 		"key1": "value1",
 		"key2": "value2",
@@ -115,7 +117,7 @@ var testSchema = []byte(`{
               "type": "real"
             },
             "min": 0,
-            "max": 10
+            "max": 5
           }
         },
         "aEmptySet": {
@@ -216,28 +218,49 @@ func TestMapperGetData(t *testing.T) {
 		NonTagged:           "something",
 	}
 
-	ovsRow := getOvsTestRow(t)
 	/* Code under test */
 	var schema ovsdb.DatabaseSchema
 	if err := json.Unmarshal(testSchema, &schema); err != nil {
 		t.Error(err)
 	}
 
-	mapper := NewMapper(schema)
-	test := ormTestType{
-		NonTagged: "something",
-	}
-	testInfo, err := NewInfo("TestTable", schema.Table("TestTable"), &test)
-	assert.NoError(t, err)
-
-	err = mapper.GetRowData(&ovsRow, testInfo)
-	assert.NoError(t, err)
-	/*End code under test*/
+	tests := []struct {
+		name        string
+		setup       func() ovsdb.Row
+		expectErr   bool
+	}{{
+		name:  "basic",
+		setup: func() ovsdb.Row {
+			return getOvsTestRow(t)
+		},
+	}, {
+		name:  "too big array",
+		setup: func() ovsdb.Row {
+			testRow := getOvsTestRow(t)
+			testRow["aFloatSet"] = test.MakeOvsSet(t, ovsdb.TypeReal, aFloatSetTooBig)
+			return testRow
+		},
+		expectErr: true,
+	}}
+	for _, test := range tests {
+		t.Run(fmt.Sprintf("GetData: %s", test.name), func(t *testing.T) {
+			mapper := NewMapper(schema)
+			tt := ormTestType{
+				NonTagged: "something",
+			}
+			testInfo, err := NewInfo("TestTable", schema.Table("TestTable"), &tt)
+			assert.NoError(t, err)
 
-	if err != nil {
-		t.Error(err)
+			ovsRow := test.setup()
+			err = mapper.GetRowData(&ovsRow, testInfo)
+			if test.expectErr {
+				assert.Error(t, err)
+			} else {
+				assert.NoError(t, err)
+				assert.Equal(t, expected, tt)
+			}
+		})
 	}
-	assert.Equal(t, expected, test)
 }
 
 func TestMapperNewRow(t *testing.T) {
@@ -315,6 +338,14 @@ func TestMapperNewRow(t *testing.T) {
 			MyFloatSet: aFloatSet,
 		},
 		expectedRow: ovsdb.Row(map[string]interface{}{"aFloatSet": test.MakeOvsSet(t, ovsdb.TypeReal, aFloatSet)}),
+	}, {
+		name: "aFloatSet too big",
+		objInput: &struct {
+			MyFloatSet []float64 `ovsdb:"aFloatSet"`
+		}{
+			MyFloatSet: aFloatSetTooBig,
+		},
+		shoulderr: true,
 	}, {
 		name: "Enum",
 		objInput: &struct {

diff --git a/ovsdb/bindings.go b/ovsdb/bindings.go
@@ -399,3 +399,21 @@ func isDefaultBaseValue(elem interface{}, etype ExtendedType) bool {
 		return false
 	}
 }
+
+// ValidateColumnConstraints validates the native value against any constraints
+// of a given column.
+func ValidateColumnConstraints(column *ColumnSchema, nativeValue interface{}) error {
+	switch column.Type {
+	case TypeSet, TypeEnum:
+		// Validate set length requirements
+		newVal := reflect.ValueOf(nativeValue)
+		maxVal := column.TypeObj.Max()
+		minVal := column.TypeObj.Min()
+		if maxVal > 1 && newVal.Len() > maxVal {
+			return fmt.Errorf("slice would overflow (%d elements but %d allowed)", newVal.Len(), maxVal)
+		} else if minVal > 0 && newVal.Len() < minVal {
+			return fmt.Errorf("slice would underflow (%d elements but %d required)", newVal.Len(), minVal)
+		}
+	}
+	return nil
+}
diff --git a/ovsdb/error.go b/ovsdb/error.go
@@ -117,7 +117,7 @@ type ConstraintViolation struct {
 }
 
 // Error implements the error interface
-func (e *ConstraintViolation) Error() string {
+func (e ConstraintViolation) Error() string {
 	msg := constraintViolation
 	if e.details != "" {
 		msg += ": " + e.details

diff --git a/server/server_integration_test.go b/server/server_integration_test.go
@@ -29,6 +29,7 @@ type bridgeType struct {
 	ExternalIds  map[string]string `ovsdb:"external_ids"`
 	Ports        []string          `ovsdb:"ports"`
 	Status       map[string]string `ovsdb:"status"`
+	FloodVLANs   []int             `ovsdb:"flood_vlans"`
 }
 
 // ovsType is the simplified ORM model of the Bridge table

diff --git a/server/testdata/ovslite.json b/server/testdata/ovslite.json
@@ -69,6 +69,17 @@
                         "min": 0,
                         "max": "unlimited"
                     }
+                },
+                "flood_vlans": {
+                    "type": {
+                        "key": {
+                            "type": "integer",
+                            "minInteger": 0,
+                            "maxInteger": 4095
+                        },
+                        "min": 0,
+                        "max": 3
+                    }
                 }
             },
             "indexes": [
@@ -78,4 +89,4 @@
             ]
         }
     }
-}
+}
diff --git a/server/transact.go b/server/transact.go
@@ -48,7 +48,10 @@ func (o *OvsdbServer) transact(name string, operations []ovsdb.Operation) ([]ovs
 			r := transaction.Select(op.Table, op.Where, op.Columns)
 			results = append(results, r)
 		case ovsdb.OperationUpdate:
-			r, tu := transaction.Update(name, op.Table, op.Where, op.Row)
+			r, tu, err := transaction.Update(name, op.Table, op.Where, op.Row)
+			if err != nil {
+				panic(err)
+			}
 			results = append(results, r)
 			if tu != nil {
 				if err := updates.Merge(tu); err != nil {
@@ -241,68 +244,70 @@ func (t *Transaction) Select(table string, where []ovsdb.Condition, columns []st
 	}
 }
 
-func (t *Transaction) Update(database, table string, where []ovsdb.Condition, row ovsdb.Row) (ovsdb.OperationResult, ovsdb.TableUpdates2) {
+func opResultError(err error, table, detailFmt string, args ...interface{}) ovsdb.OperationResult {
+	detail := fmt.Sprintf(detailFmt, args...)
+	return ovsdb.OperationResult{
+		Error:   err.Error(),
+		Details: fmt.Sprintf("table %q: %s", table, detail),
+	}
+}
+
+// Update updates the cache with the new row and returns the result of the operation
+// and the updates, or an error
+func (t *Transaction) Update(database, table string, where []ovsdb.Condition, row ovsdb.Row) (ovsdb.OperationResult, ovsdb.TableUpdates2, error) {
 	dbModel := t.Model
 	m := dbModel.Mapper
 	schema := dbModel.Schema.Table(table)
 	tableUpdate := make(ovsdb.TableUpdate2)
 
 	rows, err := t.rowsFromTransactionCacheAndDatabase(table, where)
 	if err != nil {
-		return ovsdb.OperationResult{
-			Error: err.Error(),
-		}, nil
+		return opResultError(err, table, "failed to get rows from database"), nil, nil
 	}
 
 	for uuid, old := range rows {
 		oldInfo, _ := dbModel.NewModelInfo(old)
 
 		oldRow, err := m.NewRow(oldInfo)
 		if err != nil {
-			panic(err)
+			return ovsdb.OperationResult{}, nil, err
 		}
 		new, err := dbModel.NewModel(table)
 		if err != nil {
-			panic(err)
+			return ovsdb.OperationResult{}, nil, err
 		}
 		newInfo, err := dbModel.NewModelInfo(new)
 		if err != nil {
-			panic(err)
+			return ovsdb.OperationResult{}, nil, err
 		}
 		err = m.GetRowData(&oldRow, newInfo)
 		if err != nil {
-			panic(err)
+			return ovsdb.OperationResult{}, nil, err
 		}
 		err = newInfo.SetField("_uuid", uuid)
 		if err != nil {
-			panic(err)
+			return ovsdb.OperationResult{}, nil, err
 		}
 
 		rowDelta := ovsdb.NewRow()
 		for column, value := range row {
 			colSchema := schema.Column(column)
 			if colSchema == nil {
 				e := ovsdb.ConstraintViolation{}
-				return ovsdb.OperationResult{
-					Error:   e.Error(),
-					Details: fmt.Sprintf("%s is not a valid column in the %s table", column, table),
-				}, nil
+				return opResultError(e, table, "%q is not a valid column", column), nil, nil
 			}
 			if !colSchema.Mutable() {
 				e := ovsdb.ConstraintViolation{}
-				return ovsdb.OperationResult{
-					Error:   e.Error(),
-					Details: fmt.Sprintf("column %s is of table %s not mutable", column, table),
-				}, nil
+				return opResultError(e, table, "column %q is not mutable", column), nil, nil
 			}
 			old, err := newInfo.FieldByColumn(column)
 			if err != nil {
-				panic(err)
+				return ovsdb.OperationResult{}, nil, err
 			}
 
 			native, err := ovsdb.OvsToNative(colSchema, value)
 			if err != nil {
-				panic(err)
+				return ovsdb.OperationResult{}, nil, err
 			}
 
 			if reflect.DeepEqual(old, native) {
@@ -316,17 +321,17 @@ func (t *Transaction) Update(database, table string, where []ovsdb.Condition, ro
 
 			err = newInfo.SetField(column, native)
 			if err != nil {
-				panic(err)
+				return ovsdb.OperationResult{}, nil, err
 			}
 			// convert the native to an ovs value
 			// since the value in the RowUpdate hasn't been normalized
 			newValue, err := ovsdb.NativeToOvs(colSchema, native)
 			if err != nil {
-				panic(err)
+				return ovsdb.OperationResult{}, nil, err
 			}
 			diff, err := diff(colSchema, oldValue, newValue)
 			if err != nil {
-				panic(err)
+				return ovsdb.OperationResult{}, nil, err
 			}
 			if diff != nil {
 				rowDelta[column] = diff
@@ -335,7 +340,7 @@ func (t *Transaction) Update(database, table string, where []ovsdb.Condition, ro
 
 		newRow, err := m.NewRow(newInfo)
 		if err != nil {
-			panic(err)
+			return ovsdb.OperationResult{}, nil, err
 		}
 
 		// check for index conflicts
@@ -345,11 +350,11 @@ func (t *Transaction) Update(database, table string, where []ovsdb.Condition, ro
 				return ovsdb.OperationResult{
 					Error:   e.Error(),
 					Details: newIndexExistsDetails(*indexExists),
-				}, nil
+				}, nil, nil
 			}
 			return ovsdb.OperationResult{
 				Error: err.Error(),
-			}, nil
+			}, nil, nil
 		}
 
 		err = tableUpdate.AddRowUpdate(uuid, &ovsdb.RowUpdate2{
@@ -358,15 +363,15 @@ func (t *Transaction) Update(database, table string, where []ovsdb.Condition, ro
 			New:    &newRow,
 		})
 		if err != nil {
-			panic(err)
+			return ovsdb.OperationResult{}, nil, err
 		}
 	}
 	// FIXME: We need to filter the returned columns
 	return ovsdb.OperationResult{
 			Count: len(rows),
 		}, ovsdb.TableUpdates2{
 			table: tableUpdate,
-		}
+		}, nil
 }
 
 func (t *Transaction) Mutate(database, table string, where []ovsdb.Condition, mutations []ovsdb.Mutation) (ovsdb.OperationResult, ovsdb.TableUpdates2) {