CreateAndInitCell should allow reinitializing a cell #497 (#500)

Changes Jail.createCell to Jail.obtainCell, which alters the error-throwing behaviour for better Jail.CreateAndInitCell

This PR allows cells to be reinitialized without being recreated.

Important changes:
- Calling with same cell ID won't cause any errors.
- Consecutive calls with same cell ID only reinitialize existing cell.
- Parse in library.go uses StatusAPI.CreateAndInitCell.

e2e/jail/jail_test.go

@@ -98,8 +98,9 @@ func (s *JailTestSuite) TestMultipleInitError() {
 	response := s.Jail.CreateAndInitCell(testChatID, `var _status_catalog = {}`)
 	s.Equal(`{"result": {}}`, response)
 
+	// Shouldn't cause an error and reinitialize existing
 	response = s.Jail.CreateAndInitCell(testChatID)
-	s.Equal(`{"error":"cell with id 'testChat' already exists"}`, response)
+	s.Equal(`{"result": {}}`, response)
 }
 
 // @TODO(adam): remove extra JS when checking `_status_catalog` is moved to status-react.

geth/jail/jail.go

@@ -80,18 +80,27 @@ func (j *Jail) Stop() {
 	j.cells = make(map[string]*Cell)
 }
 
-// createCell creates a new cell if it does not exists.
-func (j *Jail) createCell(chatID string) (*Cell, error) {
+// obtainCell returns an existing cell for given ID or
+// creates a new one if it does not exist.
+// Passing in true as a second argument will cause a non-nil error if the
+// cell already exists.
+func (j *Jail) obtainCell(chatID string, expectNew bool) (cell *Cell, err error) {
 	j.cellsMx.Lock()
 	defer j.cellsMx.Unlock()
 
-	if cell, ok := j.cells[chatID]; ok {
-		return cell, fmt.Errorf("cell with id '%s' already exists", chatID)
+	var ok bool
+
+	if cell, ok = j.cells[chatID]; ok {
+		// Return a non-nil error if a new cell was expected
+		if expectNew {
+			err = fmt.Errorf("cell with id '%s' already exists", chatID)
+		}
+		return
 	}
 
-	cell, err := NewCell(chatID)
+	cell, err = NewCell(chatID)
 	if err != nil {
-		return nil, err
+		return
 	}
 
 	j.cells[chatID] = cell
@@ -102,7 +111,7 @@ func (j *Jail) createCell(chatID string) (*Cell, error) {
 // CreateCell creates a new cell. It returns an error
 // if a cell with a given ID already exists.
 func (j *Jail) CreateCell(chatID string) (common.JailCell, error) {
-	return j.createCell(chatID)
+	return j.obtainCell(chatID, true)
 }
 
 // initCell initializes a cell with default JavaScript handlers and user code.
@@ -129,7 +138,7 @@ func (j *Jail) initCell(cell *Cell) error {
 
 // CreateAndInitCell creates and initializes a new Cell.
 func (j *Jail) createAndInitCell(chatID string, code ...string) (*Cell, error) {
-	cell, err := j.createCell(chatID)
+	cell, err := j.obtainCell(chatID, false)
 	if err != nil {
 		return nil, err
 	}

geth/jail/jail_test.go

@@ -60,7 +60,7 @@ func (s *JailTestSuite) TestJailGetCell() {
 
 func (s *JailTestSuite) TestJailInitCell() {
 	// InitCell on an existing cell.
-	cell, err := s.Jail.createCell("cell1")
+	cell, err := s.Jail.obtainCell("cell1", false)
 	s.NoError(err)
 	err = s.Jail.initCell(cell)
 	s.NoError(err)
@@ -102,7 +102,7 @@ func (s *JailTestSuite) TestJailCall() {
 }
 
 func (s *JailTestSuite) TestMakeCatalogVariable() {
-	cell, err := s.Jail.createCell("cell1")
+	cell, err := s.Jail.obtainCell("cell1", false)
 	s.NoError(err)
 
 	// no `_status_catalog` variable
@@ -139,12 +139,31 @@ func (s *JailTestSuite) TestPublicCreateAndInitCell() {
 	s.Equal(`{"result": {"test":true}}`, response)
 }
 
+func (s *JailTestSuite) TestPublicCreateAndInitCellConsecutive() {
+	response1 := s.Jail.CreateAndInitCell("cell1", `var _status_catalog = { test: true }`)
+	s.Contains(response1, "test")
+	cell1, err := s.Jail.Cell("cell1")
+	s.NoError(err)
+
+	// Create it again
+	response2 := s.Jail.CreateAndInitCell("cell1", `var _status_catalog = { test: true, foo: 5 }`)
+	s.Contains(response2, "test", "foo")
+	cell2, err := s.Jail.Cell("cell1")
+	s.NoError(err)
+
+	// Second cell has to be the same object as the first one
+	s.Equal(cell1, cell2)
+
+	// Second cell must have been reinitialized
+	s.NotEqual(response1, response2)
+}
+
 func (s *JailTestSuite) TestExecute() {
 	// cell does not exist
 	response := s.Jail.Execute("cell1", "('some string')")
 	s.Equal(`{"error":"cell 'cell1' not found"}`, response)
 
-	_, err := s.Jail.createCell("cell1")
+	_, err := s.Jail.obtainCell("cell1", false)
 	s.NoError(err)
 
 	// cell exists

lib/library.go

@@ -327,7 +327,7 @@ func InitJail(js *C.char) {
 //DEPRECATED in favour of CreateAndInitCell.
 //export Parse
 func Parse(chatID *C.char, js *C.char) *C.char {
-	res := statusAPI.JailParse(C.GoString(chatID), C.GoString(js))
+	res := statusAPI.CreateAndInitCell(C.GoString(chatID), C.GoString(js))
 	return C.CString(res)
 }