Fix null coordinates validation and cloning for Point objects (#1138)

* Initial plan

* Implement null coordinates validation and partial cloning fix

Co-authored-by: jdkent <[email protected]>

* Complete null coordinates fix - validation and cloning support

Co-authored-by: jdkent <[email protected]>

* Refactor PointSchema: simplify analysis_id logic and fix style issues

- Move ValidationError import to top of file
- Simplify analysis_id extraction logic to avoid duplication
- Fix flake8 style issues (whitespace, line length)
- Apply black formatting for consistent code style

Co-authored-by: jdkent <[email protected]>

* fix style issues

---------

Co-authored-by: copilot-swe-agent[bot] <[email protected]>
Co-authored-by: jdkent <[email protected]>
Co-authored-by: James Kent <[email protected]>

Author: Copilot

store/backend/neurostore/schemas/data.py

@@ -7,6 +7,7 @@
     pre_load,
     post_load,
     EXCLUDE,
+    ValidationError,
 )
 
 from sqlalchemy import func
@@ -273,26 +274,52 @@ class PointSchema(BaseDataSchema):
     coordinates = fields.List(fields.Float(), dump_only=True)
 
     # deserialization
-    x = fields.Float(load_only=True)
-    y = fields.Float(load_only=True)
-    z = fields.Float(load_only=True)
+    x = fields.Float(load_only=True, allow_none=True)
+    y = fields.Float(load_only=True, allow_none=True)
+    z = fields.Float(load_only=True, allow_none=True)
 
     class Meta:
         additional = ("kind", "space", "image", "label_id")
-        allow_none = ("kind", "space", "image", "label_id")
+        allow_none = ("kind", "space", "image", "label_id", "x", "y", "z")
 
     @pre_load
     def process_values(self, data, **kwargs):
-        # PointValues need special handling
-        if data.get("coordinates"):
-            coords = [float(c) for c in data.pop("coordinates")]
-            data["x"], data["y"], data["z"] = coords
+        # Handle case where data might be a string ID instead of dict
+        if not isinstance(data, dict):
+            return data
+
+        # Only process coordinates if they exist in the data
+        if "coordinates" in data and data["coordinates"] is not None:
+            coords = data.pop("coordinates")
+
+            # Check if all coordinates are null
+            if all(c is None for c in coords):
+                # During cloning, allow null coordinates but store them as None
+                if self.context.get("clone"):
+                    data["x"], data["y"], data["z"] = None, None, None
+                else:
+                    # Don't save points with all null coordinates to database
+                    raise ValidationError("Points cannot have all null coordinates")
+            else:
+                # Convert coordinates to float, handling potential null values
+                try:
+                    converted_coords = [
+                        float(c) if c is not None else None for c in coords
+                    ]
+                    data["x"], data["y"], data["z"] = converted_coords
+                except (TypeError, ValueError) as e:
+                    raise ValidationError(f"Invalid coordinate values: {e}")
 
         if data.get("order") is None:
-            if data.get("analysis_id") is not None:
+            # Extract analysis_id first, then check if it exists
+            analysis_id = data.get("analysis_id") or (
+                data.get("analysis") if isinstance(data.get("analysis"), str) else None
+            )
+
+            if analysis_id:
                 max_order = (
                     db.session.query(func.max(Point.order))
-                    .filter_by(analysis_id=data["analysis_id"])
+                    .filter_by(analysis_id=analysis_id)
                     .scalar()
                 )
                 data["order"] = 1 if max_order is None else max_order + 1

store/backend/neurostore/tests/test_null_coordinates.py

@@ -0,0 +1,106 @@
+#!/usr/bin/env python3
+
+import pytest
+from marshmallow import ValidationError
+
+
+def test_point_schema_null_coordinates_validation():
+    """Test that PointSchema rejects null coordinates for new points"""
+    from neurostore.schemas.data import PointSchema
+
+    point_data = {
+        "analysis": "test_analysis_id",
+        "coordinates": [None, None, None],
+        "space": "MNI",
+        "order": 0,
+    }
+
+    schema = PointSchema()
+
+    with pytest.raises(ValidationError) as exc_info:
+        schema.load(point_data)
+
+    assert "Points cannot have all null coordinates" in str(exc_info.value)
+
+
+def test_point_schema_null_coordinates_cloning():
+    """Test that PointSchema handles null coordinates during cloning"""
+    from neurostore.schemas.data import PointSchema
+
+    point_data = {
+        "analysis": "test_analysis_id",
+        "coordinates": [None, None, None],
+        "space": "MNI",
+        "order": 0,
+    }
+
+    schema = PointSchema(context={"clone": True})
+
+    # During cloning, null coordinates should be allowed and stored as None
+    result = schema.load(point_data)
+
+    assert result["x"] is None
+    assert result["y"] is None
+    assert result["z"] is None
+
+
+def test_point_schema_valid_coordinates():
+    """Test that valid coordinates work normally"""
+    from neurostore.schemas.data import PointSchema
+
+    point_data = {
+        "analysis": "test_analysis_id",
+        "coordinates": [1.0, 2.0, 3.0],
+        "space": "MNI",
+        "order": 0,
+    }
+
+    schema = PointSchema()
+    result = schema.load(point_data)
+
+    assert result["x"] == 1.0
+    assert result["y"] == 2.0
+    assert result["z"] == 3.0
+
+
+def test_analysis_schema_allows_null_coordinates_during_cloning():
+    """Test that AnalysisSchema allows null coordinate points during cloning"""
+    from neurostore.schemas.data import AnalysisSchema
+
+    analysis_data = {
+        "study": "test_study_id",
+        "name": "test_analysis",
+        "points": [
+            {
+                "analysis": "test_analysis_id",
+                "coordinates": [1.0, 2.0, 3.0],
+                "space": "MNI",
+                "order": 0,
+            },
+            {
+                "analysis": "test_analysis_id",
+                "coordinates": [None, None, None],
+                "space": "MNI",
+                "order": 1,
+            },
+        ],
+    }
+
+    schema = AnalysisSchema(context={"clone": True, "nested": True})
+    result = schema.load(analysis_data)
+
+    # Should have both points, including the null coordinate one
+    points = result.get("points", [])
+    assert len(points) == 2  # Both points should be present
+
+    # Verify the valid point has valid coordinates
+    valid_point = next(p for p in points if p["x"] == 1.0)
+    assert valid_point["x"] == 1.0
+    assert valid_point["y"] == 2.0
+    assert valid_point["z"] == 3.0
+
+    # Verify the null coordinate point has null coordinates
+    null_point = next(p for p in points if p["x"] is None)
+    assert null_point["x"] is None
+    assert null_point["y"] is None
+    assert null_point["z"] is None