Skip to content

bug: add_activities_relations() silently accepts duplicate activity names β€” adding an activity "A" twice overwrites the first node without warning, producing incorrect CPM calculations with no error raisedΒ #77

Description

@prince-pokharna

πŸ› Problem Statement

The CriticalPathMethod.add_activities_relations() method accepts a list of activity names and adds them to the internal node dictionary. If a caller passes the same activity name twice β€” either by mistake or by calling the method in a loop β€” the second call silently overwrites the first node:

cpm = CriticalPathMethod()
cpm.add_activity('O', 0)
cpm.add_activities_relations(['A', 'B', 'A'], [2, 5, 99], ['-', 'A', 'B'])
# Activity 'A' is now duration=99 with predecessor 'B' β€” wrong!

This produces:

  • Incorrect Early Start / Early Finish times for the overwritten node
  • A wrong Critical Path (the overwritten node may become critical with the new duration)
  • No ValueError or warning β€” the user receives silently wrong output

For a library used in project management / academic contexts where CPM accuracy is critical, this is a data integrity bug.

πŸ’‘ Proposed Fix

Add duplicate detection in add_activities_relations() and add_activity():

# networkdiagram/cpm.py

def add_activity(self, name: str, duration: float) -> None:
    """
    Add a single activity node to the network.

    Raises:
        ValueError: If an activity with this name already exists.
    """
    if name in self.nodes:
        raise ValueError(
            f"Activity '{name}' already exists in the network. "
            f"Duplicate activity names are not allowed. "
            f"Existing node: duration={self.nodes[name].duration}"
        )
    self.nodes[name] = Node(name=name, duration=duration)

def add_activities_relations(
    self,
    activities: list[str],
    durations: list[float],
    predecessors: list[str]
) -> None:
    """
    Add multiple activities with their durations and predecessor relationships.

    Raises:
        ValueError: If activity lists have mismatched lengths.
        ValueError: If any activity name is duplicated within the input list.
        ValueError: If any activity name already exists in the network.
    """
    if not (len(activities) == len(durations) == len(predecessors)):
        raise ValueError(
            f"Length mismatch: activities={len(activities)}, "
            f"durations={len(durations)}, predecessors={len(predecessors)}. "
            "All three lists must have the same length."
        )

    # Check for duplicates within the input list itself
    seen = set()
    for name in activities:
        if name in seen:
            raise ValueError(
                f"Duplicate activity name '{name}' found in the input list. "
                "Each activity must have a unique name."
            )
        seen.add(name)

    # Add all activities (add_activity checks for existing network conflicts)
    for name, duration in zip(activities, durations):
        self.add_activity(name, duration)

    # Then set up predecessor relationships
    for name, pred_str in zip(activities, predecessors):
        self.nodes[name].predecessors = self._parse_predecessors(pred_str)

Add unit tests covering the duplicate case:

# tests/test_duplicate_activities.py

import pytest
from networkdiagram import CriticalPathMethod

def test_duplicate_in_input_list_raises():
    cpm = CriticalPathMethod()
    cpm.add_activity('O', 0)
    with pytest.raises(ValueError, match="Duplicate activity name 'A'"):
        cpm.add_activities_relations(['A', 'B', 'A'], [2, 5, 3], ['-', 'A', 'B'])

def test_add_existing_activity_raises():
    cpm = CriticalPathMethod()
    cpm.add_activity('O', 0)
    cpm.add_activity('A', 5)
    with pytest.raises(ValueError, match="Activity 'A' already exists"):
        cpm.add_activity('A', 10)

def test_unique_activities_no_error():
    cpm = CriticalPathMethod()
    cpm.add_activity('O', 0)
    cpm.add_activities_relations(['A', 'B', 'C'], [2, 5, 3], ['-', 'A', 'A'])
    assert 'A' in cpm.nodes
    assert 'C' in cpm.nodes

πŸ“ Files to Modify

File Change
networkdiagram/cpm.py Add duplicate check in add_activity() and add_activities_relations()
tests/test_duplicate_activities.py New β€” unit tests for duplicate detection

βœ… Acceptance Criteria

  • add_activity('A', 5) followed by add_activity('A', 10) raises ValueError
  • add_activities_relations(['A', 'B', 'A'], ...) raises ValueError with the duplicate name
  • All existing tests continue to pass
  • Error messages are descriptive and include the duplicate name

Suggested labels: bug, data-integrity, validation

I would like to work on this. Could you please assign it to me?

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions