PR comment fixes

Author: tpellissier

src/dataverse_sdk/data/odata.py

@@ -715,6 +715,9 @@ def _logical_to_schema_name(self, logical_name: str) -> str:
         
         Use this ONLY when creating new entities where we control the SchemaName.
         For existing entities, use _get_entity_schema_name() to get the actual SchemaName from server.
+        
+        Note: All callers (_create_table, _create_columns) validate that logical_name contains
+        an underscore before calling this method, enforcing Dataverse's publisher prefix requirement.
         """
         # Normalize logical name first
         logical_name = self._normalize_logical_name(logical_name)
@@ -723,10 +726,7 @@ def _logical_to_schema_name(self, logical_name: str) -> str:
         if not logical_name:
             raise ValueError("logical_name cannot be empty or whitespace-only")
 
-        if "_" not in logical_name:
-            # No prefix, just capitalize first letter
-            return logical_name[:1].upper() + logical_name[1:]
-        
+        # Split on first underscore to get prefix and name parts
         prefix, rest = logical_name.split("_", 1)
 
         # Validate that rest is not empty (e.g., "new_" is invalid)
@@ -764,7 +764,7 @@ def _get_attribute_schema_name(self, entity_logical: str, attribute_logical: str
             return self._attribute_schema_cache[cache_key]
 
         # Get entity metadata ID first
-        ent = self._get_entity_by_logical(entity_logical)
+        ent = self._get_entity_by_logical_name(entity_logical)
         if not ent or not ent.get("MetadataId"):
             raise MetadataError(
                 f"Entity '{entity_logical}' not found.",
@@ -829,7 +829,7 @@ def _get_entity_by_schema(
         items = r.json().get("value", [])
         return items[0] if items else None
 
-    def _get_entity_by_logical(
+    def _get_entity_by_logical_name(
         self,
         logical_name: str,
         headers: Optional[Dict[str, str]] = None,
@@ -874,7 +874,7 @@ def _create_entity(
         if solution_unique_name:
             params = {"SolutionUniqueName": solution_unique_name}
         self._request("post", url, json=payload, params=params)
-        ent = self._get_entity_by_logical(
+        ent = self._get_entity_by_logical_name(
             logical_name,
             headers={"Consistency": "Strong"},
         )
@@ -1280,7 +1280,7 @@ def _get_table_info(self, logical_name: str) -> Optional[Dict[str, Any]]:
         # Normalize logical name for case-insensitive handling
         logical_name = self._normalize_logical_name(logical_name)
 
-        ent = self._get_entity_by_logical(logical_name)
+        ent = self._get_entity_by_logical_name(logical_name)
         if not ent:
             return None
         return {
@@ -1305,7 +1305,7 @@ def _delete_table(self, logical_name: str) -> None:
         logical_name = self._normalize_logical_name(logical_name)
 
         # Use strong consistency to ensure we get latest metadata after recent create
-        ent = self._get_entity_by_logical(logical_name, headers={"Consistency": "Strong"})
+        ent = self._get_entity_by_logical_name(logical_name, headers={"Consistency": "Strong"})
         if not ent or not ent.get("MetadataId"):
             raise MetadataError(
                 f"Table '{logical_name}' not found.",
@@ -1379,7 +1379,7 @@ def _create_columns(
         # Normalize logical name for case-insensitive handling
         logical_name = self._normalize_logical_name(logical_name)
 
-        ent = self._get_entity_by_logical(logical_name)
+        ent = self._get_entity_by_logical_name(logical_name)
         if not ent or not ent.get("MetadataId"):
             raise MetadataError(
                 f"Table '{logical_name}' not found.",
@@ -1393,6 +1393,11 @@ def _create_columns(
         for column_name, column_type in columns.items():
             # Normalize column logical name for case-insensitive handling
             column_name_normalized = self._normalize_logical_name(column_name)
+            
+            # Validate that column name includes publisher prefix
+            if "_" not in column_name_normalized:
+                raise ValueError(f"Column logical name must include publisher prefix (e.g., 'new_columnname'), got: '{column_name}'")
+            
             # Convert column logical name to SchemaName
             col_schema = self._logical_to_schema_name(column_name_normalized)
             payload = self._attribute_payload(col_schema, column_type)
@@ -1428,7 +1433,7 @@ def _delete_columns(
         # Normalize logical name for case-insensitive handling
         logical_name = self._normalize_logical_name(logical_name)
 
-        ent = self._get_entity_by_logical(logical_name)
+        ent = self._get_entity_by_logical_name(logical_name)
         if not ent or not ent.get("MetadataId"):
             raise MetadataError(
                 f"Table '{logical_name}' not found.",

tests/unit/data/test_naming_normalization.py

@@ -76,13 +76,6 @@ def test_logical_to_schema_name_basic():
     assert c._logical_to_schema_name("abc_myentity") == "abc_Myentity"
 
 
-def test_logical_to_schema_name_no_underscore():
-    """Test SchemaName when no underscore (capitalize first letter)."""
-    c = TestableClient([])
-    assert c._logical_to_schema_name("account") == "Account"
-    assert c._logical_to_schema_name("contact") == "Contact"
-
-
 # ============================================================================
 # Tests for _get_entity_schema_name
 # ============================================================================
@@ -114,7 +107,7 @@ def test_get_entity_schema_name_not_found():
 def test_get_attribute_schema_name_lookup():
     """Test that _get_attribute_schema_name retrieves attribute SchemaName."""
     responses = [
-        (200, {}, {"value": [MD_ENTITY_BY_LOGICAL]}),  # _get_entity_by_logical uses EntityDefinitions endpoint
+        (200, {}, {"value": [MD_ENTITY_BY_LOGICAL]}),  # _get_entity_by_logical_name uses EntityDefinitions endpoint
         (200, {}, MD_ATTRIBUTE_TITLE)      # Attribute lookup
     ]
     c = TestableClient(responses)
@@ -306,14 +299,14 @@ def test_entity_set_from_logical_case_insensitive():
     assert len(c._http.calls) == 1
 
 
-def test_get_entity_by_logical_normalization():
-    """Test that _get_entity_by_logical normalizes input."""
+def test_get_entity_by_logical_name_normalization():
+    """Test that _get_entity_by_logical_name normalizes input."""
     responses = [
         (200, {}, {"value": [MD_ENTITY_BY_LOGICAL]})
     ]
     c = TestableClient(responses)
 
-    entity = c._get_entity_by_logical("NEW_SAMPLEITEM")
+    entity = c._get_entity_by_logical_name("NEW_SAMPLEITEM")
     assert entity["LogicalName"] == "new_sampleitem"
 
     # Check that the query parameters used normalized name
@@ -638,11 +631,6 @@ def test_logical_to_schema_name_case_insensitive():
     assert c._logical_to_schema_name("NEW_SAMPLEITEM") == "new_Sampleitem"
     assert c._logical_to_schema_name("New_SampleItem") == "new_Sampleitem"
     assert c._logical_to_schema_name("  new_sampleitem  ") == "new_Sampleitem"
-    
-    # Test without prefix
-    assert c._logical_to_schema_name("account") == "Account"
-    assert c._logical_to_schema_name("ACCOUNT") == "Account"
-    assert c._logical_to_schema_name("Account") == "Account"
 
 
 def test_logical_to_schema_name_edge_cases():
@@ -661,9 +649,41 @@ def test_logical_to_schema_name_edge_cases():
     with pytest.raises(ValueError, match="empty part after underscore"):
         c._logical_to_schema_name("new_")
 
+    # Name without underscore should raise ValueError (since split will fail)
+    # This should never happen in practice because callers validate presence of underscore
+    with pytest.raises(ValueError):
+        c._logical_to_schema_name("account")
+    
     # Multiple underscores - only first split matters
     assert c._logical_to_schema_name("new_sample_item") == "new_Sample_item"
 
 
+def test_create_columns_requires_prefix():
+    """Test that _create_columns rejects column names without publisher prefix."""
+    responses = [
+        (200, {}, {"value": [MD_ENTITY_BY_LOGICAL]}),  # Get entity by logical
+    ]
+    c = TestableClient(responses)
+    
+    # Column name without underscore should raise ValueError
+    with pytest.raises(ValueError, match="Column logical name must include publisher prefix"):
+        c._create_columns(
+            logical_name="new_sampleitem",
+            columns={"mycolumn": "string"}  # Missing prefix
+        )
+
+
+def test_create_table_requires_prefix():
+    """Test that _create_table rejects table names without publisher prefix."""
+    c = TestableClient([])
+    
+    # Table name without underscore should raise ValueError
+    with pytest.raises(ValueError, match="Table logical name must include publisher prefix"):
+        c._create_table(
+            logical_name="mytable",  # Missing prefix
+            schema={"new_field": "string"}
+        )
+
+
 if __name__ == "__main__":
     pytest.main([__file__, "-v"])