fix: avoid adding duplicate reactions (#614)

Previously, existing reactions were filtered out but still added to the
whole `model.reactions` list. This was generating `ValueError`s although
the method promised to just ignore duplicates.

cobra/core/model.py

@@ -523,34 +523,30 @@ def add_reactions(self, reaction_list):
         reaction_list : list
             A list of `cobra.Reaction` objects
         """
+        def existing_filter(rxn):
+            if rxn.id in self.reactions:
+                LOGGER.warning(
+                    "Ignoring reaction '%s' since it already exists.", rxn.id)
+                return False
+            return True
 
-        try:
-            reaction_list = DictList(reaction_list)
-        except TypeError:
-            reaction_list = DictList([reaction_list])
-
-        # First check whether the metabolites exist in the model
-        existing = [rxn for rxn in reaction_list if rxn.id in self.reactions]
-        for rxn in existing:
-            LOGGER.info('skip adding reaction %s as already existing', rxn.id)
-        reaction_list = [rxn for rxn in reaction_list
-                         if rxn.id not in existing]
+        # First check whether the reactions exist in the model.
+        pruned = DictList(filter(existing_filter, reaction_list))
 
         context = get_context(self)
 
-        # Add reactions. Also take care of genes and metabolites in the loop
-        for reaction in reaction_list:
-            reaction._model = self  # the reaction now points to the model
-            # keys() is necessary because the dict will be modified during
-            # the loop
-            for metabolite in list(reaction._metabolites.keys()):
-                # if the metabolite is not in the model, add it
-                # should we be adding a copy instead.
+        # Add reactions. Also take care of genes and metabolites in the loop.
+        for reaction in pruned:
+            reaction._model = self
+            # Build a `list()` because the dict will be modified in the loop.
+            for metabolite in list(reaction.metabolites):
+                # TODO: Should we add a copy of the metabolite instead?
                 if metabolite not in self.metabolites:
                     self.add_metabolites(metabolite)
                 # A copy of the metabolite exists in the model, the reaction
                 # needs to point to the metabolite in the model.
                 else:
+                    # FIXME: Modifying 'private' attributes is horrible.
                     stoichiometry = reaction._metabolites.pop(metabolite)
                     model_metabolite = self.metabolites.get_by_id(
                         metabolite.id)
@@ -578,13 +574,13 @@ def add_reactions(self, reaction_list):
                         reaction._dissociate_gene(gene)
                         reaction._associate_gene(model_gene)
 
-        self.reactions += reaction_list
+        self.reactions += pruned
 
         if context:
-            context(partial(self.reactions.__isub__, reaction_list))
+            context(partial(self.reactions.__isub__, pruned))
 
         # from cameo ...
-        self._populate_solver(reaction_list)
+        self._populate_solver(pruned)
 
     def remove_reactions(self, reactions, remove_orphans=False):
         """Remove reactions from the model.

cobra/test/test_solver_model.py

@@ -578,6 +578,28 @@ def test_add_reactions(self, model):
         assert coefficients_dict[
                    model.reactions.r2.reverse_variable] == -3.
 
+    def test_add_reactions_single_existing(self, model):
+        rxn = model.reactions[0]
+        r1 = Reaction(rxn.id)
+        r1.add_metabolites({Metabolite('A'): -1, Metabolite('B'): 1})
+        r1.lower_bound, r1.upper_bound = -999999., 999999.
+        model.add_reactions([r1])
+        assert rxn in model.reactions
+        assert r1 is not model.reactions.get_by_id(rxn.id)
+
+    def test_add_reactions_duplicate(self, model):
+        rxn = model.reactions[0]
+        r1 = Reaction('r1')
+        r1.add_metabolites({Metabolite('A'): -1, Metabolite('B'): 1})
+        r1.lower_bound, r1.upper_bound = -999999., 999999.
+        r2 = Reaction(rxn.id)
+        r2.add_metabolites(
+            {Metabolite('A'): -1, Metabolite('C'): 1, Metabolite('D'): 1})
+        model.add_reactions([r1, r2])
+        assert r1 in model.reactions
+        assert rxn in model.reactions
+        assert r2 is not model.reactions.get_by_id(rxn.id)
+
     def test_add_cobra_reaction(self, model):
         r = cobra.Reaction(id="c1")
         model.add_reaction(r)