update

Author: Dylan Huang

eval_protocol/evaluation.py

@@ -800,11 +800,11 @@ def create(self, evaluator_id, display_name=None, description=None, force=False)
                 tar_size = self._create_tar_gz_with_ignores(tar_path, cwd)
 
                 # Call GetEvaluatorUploadEndpoint
-                upload_endpoint_url = f"{self.api_base}/v1/{evaluator_name}:getUploadEndpoint"
+                upload_endpoint_path = f"v1/{evaluator_name}:getUploadEndpoint"
                 upload_payload = {"name": evaluator_name, "filename_to_size": {tar_filename: tar_size}}
 
                 logger.info(f"Requesting upload endpoint for {tar_filename}")
-                upload_response = client.post(upload_endpoint_url, json=upload_payload)
+                upload_response = client.post(upload_endpoint_path, json=upload_payload)
                 upload_response.raise_for_status()
 
                 # Check for signed URLs
@@ -884,9 +884,9 @@ def create(self, evaluator_id, display_name=None, description=None, force=False)
                             raise
 
                 # Step 3: Validate upload
-                validate_url = f"{self.api_base}/v1/{evaluator_name}:validateUpload"
+                validate_path = f"v1/{evaluator_name}:validateUpload"
                 validate_payload = {"name": evaluator_name}
-                validate_response = client.post(validate_url, json=validate_payload)
+                validate_response = client.post(validate_path, json=validate_payload)
                 validate_response.raise_for_status()
 
                 validate_data = validate_response.json()

tests/test_fireworks_api_client.py

@@ -225,5 +225,244 @@ def test_user_agent_without_api_key(self):
             assert "Authorization" not in headers
 
 
+class TestFireworksAPIClientPathHandling:
+    """Test that FireworksAPIClient correctly handles relative paths and prevents URL construction bugs."""
+
+    def test_post_relative_path_combines_with_api_base(self):
+        """Test that POST requests correctly combine relative paths with api_base."""
+        api_base = "https://api.fireworks.ai"
+        client = FireworksAPIClient(api_key="test_key", api_base=api_base)
+
+        mock_response = MagicMock()
+        mock_response.status_code = 200
+
+        with patch.object(client._session, "post", return_value=mock_response) as mock_post:
+            relative_path = "v1/test/evaluator:getUploadEndpoint"
+            client.post(relative_path, json={"name": "test"})
+
+            mock_post.assert_called_once()
+            call_args = mock_post.call_args
+            # Check the URL passed to requests.post
+            assert call_args[0][0] == f"{api_base}/{relative_path}"
+            assert not call_args[0][0].startswith(f"{api_base}/{api_base}")
+
+    def test_get_relative_path_combines_with_api_base(self):
+        """Test that GET requests correctly combine relative paths with api_base."""
+        api_base = "https://api.fireworks.ai"
+        client = FireworksAPIClient(api_key="test_key", api_base=api_base)
+
+        mock_response = MagicMock()
+        mock_response.status_code = 200
+
+        with patch.object(client._session, "get", return_value=mock_response) as mock_get:
+            relative_path = "verifyApiKey"
+            client.get(relative_path)
+
+            mock_get.assert_called_once()
+            call_args = mock_get.call_args
+            assert call_args[0][0] == f"{api_base}/{relative_path}"
+
+    def test_post_get_upload_endpoint_path(self):
+        """Test the specific getUploadEndpoint path that was buggy.
+
+        This ensures relative paths like 'v1/{name}:getUploadEndpoint' are handled correctly
+        and don't get double-prefixed with api_base.
+        """
+        api_base = "https://api.fireworks.ai"
+        client = FireworksAPIClient(api_key="test_key", api_base=api_base)
+
+        mock_response = MagicMock()
+        mock_response.status_code = 200
+        mock_response.json.return_value = {"filenameToSignedUrls": {"test.tar.gz": "https://signed.url"}}
+
+        with patch.object(client._session, "post", return_value=mock_response) as mock_post:
+            evaluator_name = "test-evaluator"
+            # This is the correct pattern - relative path, not full URL
+            upload_endpoint_path = f"v1/{evaluator_name}:getUploadEndpoint"
+            client.post(upload_endpoint_path, json={"name": evaluator_name})
+
+            mock_post.assert_called_once()
+            call_args = mock_post.call_args
+            expected_url = f"{api_base}/{upload_endpoint_path}"
+            actual_url = call_args[0][0]
+            assert actual_url == expected_url, f"Expected {expected_url}, got {actual_url}"
+            # Ensure it doesn't have the buggy double-prefix
+            assert not actual_url.startswith(f"{api_base}/{api_base}")
+
+    def test_post_validate_upload_path(self):
+        """Test the specific validateUpload path that was buggy.
+
+        This ensures relative paths like 'v1/{name}:validateUpload' are handled correctly.
+        """
+        api_base = "https://api.fireworks.ai"
+        client = FireworksAPIClient(api_key="test_key", api_base=api_base)
+
+        mock_response = MagicMock()
+        mock_response.status_code = 200
+        mock_response.json.return_value = {"status": "validated"}
+
+        with patch.object(client._session, "post", return_value=mock_response) as mock_post:
+            evaluator_name = "test-evaluator"
+            # This is the correct pattern - relative path, not full URL
+            validate_path = f"v1/{evaluator_name}:validateUpload"
+            client.post(validate_path, json={"name": evaluator_name})
+
+            mock_post.assert_called_once()
+            call_args = mock_post.call_args
+            expected_url = f"{api_base}/{validate_path}"
+            actual_url = call_args[0][0]
+            assert actual_url == expected_url, f"Expected {expected_url}, got {actual_url}"
+            # Ensure it doesn't have the buggy double-prefix
+            assert not actual_url.startswith(f"{api_base}/{api_base}")
+
+    def test_path_with_leading_slash_stripped(self):
+        """Test that leading slashes in paths are correctly handled."""
+        api_base = "https://api.fireworks.ai"
+        client = FireworksAPIClient(api_key="test_key", api_base=api_base)
+
+        mock_response = MagicMock()
+        mock_response.status_code = 200
+
+        with patch.object(client._session, "get", return_value=mock_response) as mock_get:
+            # Path with leading slash should be handled correctly
+            client.get("/v1/test/path")
+
+            mock_get.assert_called_once()
+            call_args = mock_get.call_args
+            # Should not have double slash
+            assert call_args[0][0] == f"{api_base}/v1/test/path"
+
+    def test_api_base_with_trailing_slash(self):
+        """Test that api_base with trailing slash is handled correctly."""
+        api_base = "https://api.fireworks.ai/"
+        client = FireworksAPIClient(api_key="test_key", api_base=api_base)
+
+        mock_response = MagicMock()
+        mock_response.status_code = 200
+
+        with patch.object(client._session, "post", return_value=mock_response) as mock_post:
+            relative_path = "v1/test/path"
+            client.post(relative_path, json={})
+
+            mock_post.assert_called_once()
+            call_args = mock_post.call_args
+            # Should not have double slash
+            assert call_args[0][0] == f"https://api.fireworks.ai/{relative_path}"
+
+    def test_all_http_methods_with_relative_paths(self):
+        """Test that all HTTP methods correctly handle relative paths."""
+        api_base = "https://api.fireworks.ai"
+        client = FireworksAPIClient(api_key="test_key", api_base=api_base)
+
+        mock_response = MagicMock()
+        mock_response.status_code = 200
+
+        test_path = "v1/accounts/test/evaluators"
+
+        methods = [
+            ("get", lambda p: client.get(p)),
+            ("post", lambda p: client.post(p, json={})),
+            ("put", lambda p: client.put(p, json={})),
+            ("patch", lambda p: client.patch(p, json={})),
+            ("delete", lambda p: client.delete(p)),
+        ]
+
+        for method_name, method_call in methods:
+            with patch.object(client._session, method_name, return_value=mock_response) as mock_method:
+                method_call(test_path)
+
+                mock_method.assert_called_once()
+                call_args = mock_method.call_args
+                expected_url = f"{api_base}/{test_path}"
+                actual_url = call_args[0][0]
+                assert actual_url == expected_url, f"{method_name.upper()} expected {expected_url}, got {actual_url}"
+                # Ensure no double-prefix bug
+                assert not actual_url.startswith(f"{api_base}/{api_base}"), (
+                    f"{method_name.upper()} URL has double-prefix bug: {actual_url}"
+                )
+
+    def test_paths_containing_v1_pattern(self):
+        """Test various v1 API paths to ensure correct URL construction."""
+        api_base = "https://api.fireworks.ai"
+        client = FireworksAPIClient(api_key="test_key", api_base=api_base)
+
+        mock_response = MagicMock()
+        mock_response.status_code = 200
+
+        test_cases = [
+            "v1/accounts/test/evaluators",
+            "v1/accounts/test/evaluators/eval-id",
+            "v1/accounts/test/evaluatorsV2",
+            "v1/accounts/test/evaluators:previewEvaluator",
+            "v1/test-evaluator:getUploadEndpoint",
+            "v1/test-evaluator:validateUpload",
+        ]
+
+        with patch.object(client._session, "post", return_value=mock_response) as mock_post:
+            for path in test_cases:
+                client.post(path, json={})
+
+                call_args = mock_post.call_args
+                actual_url = call_args[0][0]
+                expected_url = f"{api_base}/{path}"
+
+                assert actual_url == expected_url, (
+                    f"Path '{path}' resulted in URL '{actual_url}', expected '{expected_url}'"
+                )
+                assert not actual_url.startswith(f"{api_base}/{api_base}"), (
+                    f"Path '{path}' has double-prefix bug: {actual_url}"
+                )
+
+                mock_post.reset_mock()
+
+    def test_full_url_passed_by_mistake_detected(self):
+        """Test that accidentally passing a full URL instead of relative path is detected.
+
+        This test documents the bug pattern: if a full URL like '{api_base}/v1/path'
+        is passed instead of a relative path like 'v1/path', it will result in a
+        malformed URL like '{api_base}/{api_base}/v1/path'.
+
+        This test verifies that our code correctly handles relative paths (which prevents
+        the bug), and documents what would happen if the bug occurred.
+        """
+        api_base = "https://api.fireworks.ai"
+        client = FireworksAPIClient(api_key="test_key", api_base=api_base)
+
+        mock_response = MagicMock()
+        mock_response.status_code = 200
+
+        # CORRECT: Relative path (what we should use)
+        with patch.object(client._session, "post", return_value=mock_response) as mock_post:
+            correct_relative_path = "v1/test-evaluator:getUploadEndpoint"
+            client.post(correct_relative_path, json={})
+
+            call_args = mock_post.call_args
+            correct_url = call_args[0][0]
+            expected_correct_url = f"{api_base}/{correct_relative_path}"
+            assert correct_url == expected_correct_url
+
+        # INCORRECT: Full URL (this would cause the bug - but we're not actually testing this,
+        # just documenting that our current implementation would create a malformed URL)
+        # If someone accidentally did: client.post(f"{api_base}/v1/path", ...)
+        # The result would be: f"{api_base}/{api_base}/v1/path" which is wrong.
+        # Our tests above ensure we use relative paths, preventing this bug.
+        mock_post.reset_mock()
+        with patch.object(client._session, "post", return_value=mock_response) as mock_post:
+            # Simulating what WOULD happen if buggy code passed full URL
+            buggy_full_url = f"{api_base}/v1/test-evaluator:getUploadEndpoint"
+            client.post(buggy_full_url, json={})
+
+            call_args = mock_post.call_args
+            buggy_url = call_args[0][0]
+            # This shows what the buggy URL would look like
+            buggy_expected = f"{api_base}/{buggy_full_url}"
+
+            # This assertion documents the bug pattern - the URL would be malformed
+            assert buggy_url == buggy_expected
+            assert buggy_url.startswith(f"{api_base}/{api_base}"), (
+                "This documents the bug: passing full URL creates double-prefix. Always use relative paths!"
+            )
+
+
 if __name__ == "__main__":
     pytest.main([__file__, "-v"])