Conversation
…ns for events and productions
There was a problem hiding this comment.
Pull request overview
This pull request adds a RESTful API to the asimov project along with substantial improvements to dependency management, prior specifications, and HTML reporting. The changes include:
Changes:
- Addition of a REST API with Flask for programmatic access to events and productions
- New Python API for project creation and management via
Projectclass - Enhanced dependency management with property-based queries and optional dependencies
- Prior specification system using Pydantic models with pipeline-specific interfaces
- Improved HTML reporting with visual workflow graphs and modal dialogs
- Directory structure change from
C01_offlinetoanalyses
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| asimov/api/* | New REST API implementation with Flask, authentication, and CRUD operations |
| asimov/project.py | New Python API for project management with context manager pattern |
| asimov/priors.py | New prior specification system with Pydantic validation |
| asimov/pipelines/*.py | Pipeline-specific prior interfaces and improved result detection |
| asimov/event.py | Enhanced dependency graph management and HTML visualization |
| asimov/ledger.py | Added file locking for thread-safe operations |
| asimov/git.py | Added default branch detection (main/master) |
| asimov/cli/report.py | Significantly enhanced HTML reporting with filters and modals |
| tests/* | Comprehensive new test coverage for all new features |
| pyproject.toml | Updated dependencies including flask-cors, filelock, pydantic |
| docs/* | New documentation for Python API, priors, and dependency management |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
asimov/api/app.py
Outdated
| app = Flask(__name__) | ||
|
|
||
| # Configuration | ||
| app.config['SECRET_KEY'] = config.get('api', 'secret_key', fallback='dev-secret-key') |
There was a problem hiding this comment.
The fallback secret key 'dev-secret-key' is insecure for production use. This creates a security vulnerability where the application could be deployed without a proper secret key configured. Consider raising an error if SECRET_KEY is not configured rather than falling back to a default value, or at minimum log a warning when using the fallback.
| app.config['SECRET_KEY'] = config.get('api', 'secret_key', fallback='dev-secret-key') | |
| secret_key = config.get('api', 'secret_key', fallback=None) | |
| if not secret_key: | |
| raise RuntimeError( | |
| "SECRET_KEY is not configured. Please set the 'api.secret_key' configuration " | |
| "to a strong, unpredictable value before starting the application." | |
| ) | |
| app.config['SECRET_KEY'] = secret_key |
asimov/api/app.py
Outdated
| app.config['SECRET_KEY'] = config.get('api', 'secret_key', fallback='dev-secret-key') | ||
|
|
||
| # CORS for web interface | ||
| CORS(app) |
There was a problem hiding this comment.
CORS is configured to allow all origins by default. This is a security risk in production environments. Consider configuring CORS with specific allowed origins or requiring explicit configuration for production deployments.
| CORS(app) | |
| env = app.config.get("ENV", "production") | |
| if env == "production": | |
| # In production, require explicit configuration of allowed origins | |
| cors_origins = config.get('api', 'cors_origins', fallback=None) | |
| if cors_origins: | |
| origins = [origin.strip() for origin in cors_origins.split(',') if origin.strip()] | |
| if origins: | |
| CORS(app, origins=origins) | |
| else: | |
| # In non-production environments, allow all origins for development convenience | |
| CORS(app) |
asimov/api/blueprints/events.py
Outdated
| Provides CRUD operations for gravitational wave events. | ||
| """ | ||
|
|
||
| from flask import Blueprint, request, jsonify, g |
There was a problem hiding this comment.
Import of 'g' is not used.
| from flask import Blueprint, request, jsonify, g | |
| from flask import Blueprint, request, jsonify |
asimov/api/auth.py
Outdated
| with open(keys_file, 'r') as f: | ||
| data = yaml.safe_load(f) | ||
| return data.get('api_keys', {}) | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 15 out of 15 changed files in this pull request and generated 18 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| except Exception as e: | ||
| return jsonify({'error': str(e)}), 500 |
There was a problem hiding this comment.
The error handler catches bare Exception, which can mask programming errors and make debugging difficult. Consider catching more specific exceptions or at least logging the full exception before returning a generic error message.
|
|
||
| app = create_app() | ||
| click.echo(f"Starting API server on {host}:{port}") | ||
| click.echo(f"Health check: http://{host}:{port}/api/v1/health") |
There was a problem hiding this comment.
Using Flask's built-in development server (app.run) is not suitable for production use. While the documentation mentions using Gunicorn for production, the serve command should at minimum warn users when debug mode is disabled that they should use a production WSGI server like Gunicorn instead.
| click.echo(f"Health check: http://{host}:{port}/api/v1/health") | |
| click.echo(f"Health check: http://{host}:{port}/api/v1/health") | |
| if not debug: | |
| click.echo( | |
| "Warning: You are running the Flask development server with debug disabled. " | |
| "This server is not suitable for production. Use a production WSGI server " | |
| "such as Gunicorn to run this application in production.", | |
| err=True, | |
| ) |
| class APIEventsTestCase(AsimovTestCase): | ||
| """Tests for Events API endpoints.""" | ||
|
|
||
| def setUp(self): | ||
| """Set up test environment.""" | ||
| super().setUp() | ||
|
|
||
| # Inject test API keys before creating app | ||
| import asimov.api.auth as auth_module | ||
| auth_module._api_keys_cache = {'test-token-12345': 'test-user'} | ||
|
|
||
| self.app = create_app() | ||
| self.app.config['TESTING'] = True | ||
| self.client = self.app.test_client() | ||
| self.auth_headers = {'Authorization': 'Bearer test-token-12345'} | ||
|
|
||
| def tearDown(self): | ||
| """Clean up test environment.""" | ||
| # Reset API keys cache | ||
| import asimov.api.auth as auth_module | ||
| auth_module._api_keys_cache = None | ||
| super().tearDown() | ||
|
|
||
| def test_list_events_empty(self): | ||
| """Test listing events when none exist.""" | ||
| response = self.client.get('/api/v1/events/') | ||
| self.assertEqual(response.status_code, 200) | ||
| data = json.loads(response.data) | ||
| self.assertIn('events', data) | ||
| self.assertEqual(len(data['events']), 0) | ||
|
|
||
| def test_create_event_no_auth(self): | ||
| """Test creating event without authentication fails.""" | ||
| response = self.client.post( | ||
| '/api/v1/events/', | ||
| data=json.dumps({'name': 'GW150914'}), | ||
| content_type='application/json' | ||
| ) | ||
| self.assertEqual(response.status_code, 401) | ||
| data = json.loads(response.data) | ||
| self.assertIn('error', data) | ||
|
|
||
| def test_create_event_invalid_auth(self): | ||
| """Test creating event with invalid token fails.""" | ||
| response = self.client.post( | ||
| '/api/v1/events/', | ||
| data=json.dumps({'name': 'GW150914'}), | ||
| headers={'Authorization': 'Bearer invalid-token'}, | ||
| content_type='application/json' | ||
| ) | ||
| self.assertEqual(response.status_code, 401) | ||
|
|
||
| def test_create_event_missing_name(self): | ||
| """Test creating event without name fails validation.""" | ||
| response = self.client.post( | ||
| '/api/v1/events/', | ||
| data=json.dumps({'meta': {'test': True}}), | ||
| headers=self.auth_headers, | ||
| content_type='application/json' | ||
| ) | ||
| self.assertEqual(response.status_code, 400) | ||
| data = json.loads(response.data) | ||
| self.assertIn('error', data) | ||
|
|
||
| def test_get_nonexistent_event(self): | ||
| """Test getting non-existent event returns 404.""" | ||
| response = self.client.get('/api/v1/events/NonExistent') | ||
| self.assertEqual(response.status_code, 404) | ||
| data = json.loads(response.data) | ||
| self.assertEqual(data['error'], 'Event not found') | ||
|
|
||
| def test_update_nonexistent_event(self): | ||
| """Test updating non-existent event returns 404.""" | ||
| response = self.client.put( | ||
| '/api/v1/events/NonExistent', | ||
| data=json.dumps({'meta': {'test': True}}), | ||
| headers=self.auth_headers, | ||
| content_type='application/json' | ||
| ) | ||
| self.assertEqual(response.status_code, 404) | ||
|
|
||
| def test_delete_nonexistent_event(self): | ||
| """Test deleting non-existent event returns 404.""" | ||
| response = self.client.delete( | ||
| '/api/v1/events/NonExistent', | ||
| headers=self.auth_headers | ||
| ) | ||
| self.assertEqual(response.status_code, 404) | ||
|
|
||
| def test_list_productions_nonexistent_event(self): | ||
| """Test listing productions for non-existent event returns 404.""" | ||
| response = self.client.get('/api/v1/events/NonExistent/productions') | ||
| self.assertEqual(response.status_code, 404) | ||
|
|
||
|
|
There was a problem hiding this comment.
There are no tests for the successful event creation, update, or deletion paths. While error cases are tested, it's important to verify that the happy path works correctly. Consider adding tests for: creating an event successfully, updating an event successfully, deleting an event successfully, and listing events with data.
| except Exception as e: | ||
| return jsonify({'error': str(e)}), 500 |
There was a problem hiding this comment.
The error handler catches bare Exception, which can mask programming errors and make debugging difficult. Consider catching more specific exceptions or at least logging the full exception before returning a generic error message.
asimov/api/auth.py
Outdated
| keys = data.get('api_keys', {}) | ||
| if keys: | ||
| return keys | ||
| except Exception: |
There was a problem hiding this comment.
The bare except catches all exceptions including system exceptions like KeyboardInterrupt and SystemExit. This can make the code difficult to interrupt or debug. Consider catching specific exceptions like IOError, yaml.YAMLError, or OSError instead.
| except Exception: | |
| except (OSError, IOError, yaml.YAMLError): | |
| # Ignore expected file and YAML parsing errors and fall back to | |
| # other configuration mechanisms. |
asimov/api/blueprints/events.py
Outdated
| Updated event data or error message. | ||
| """ | ||
| try: | ||
| data = EventUpdate(**request.json) |
There was a problem hiding this comment.
request.json can be None if the request has no JSON payload or an invalid Content-Type header. This will cause a TypeError when passed to EventUpdate(**request.json). Consider validating that request.json is not None before using it, or use request.get_json() with error handling.
| if 'ledger' not in g: | ||
| ledger_path = config.get("ledger", "location") | ||
| g.ledger = YAMLLedger(ledger_path) | ||
| return g.ledger |
There was a problem hiding this comment.
The get_ledger function creates a new YAMLLedger instance for each request, which loads the entire ledger from disk. In a multi-worker deployment, each worker will have its own ledger instance, and changes made by one worker won't be immediately visible to others until they reload. This could lead to race conditions where concurrent requests see stale data. Consider implementing a proper cache invalidation strategy or reloading the ledger before each operation.
tests/test_api.py
Outdated
| import unittest | ||
| import json | ||
| import os | ||
| import shutil |
There was a problem hiding this comment.
Import of 'shutil' is not used.
| import shutil |
tests/test_api.py
Outdated
| import shutil | ||
| from asimov.api.app import create_app | ||
| from asimov.testing import AsimovTestCase | ||
| from asimov.ledger import YAMLLedger |
There was a problem hiding this comment.
Import of 'YAMLLedger' is not used.
| from asimov.ledger import YAMLLedger |
asimov/api/auth.py
Outdated
| keys = data.get('api_keys', {}) | ||
| if keys: | ||
| return keys | ||
| except Exception: |
There was a problem hiding this comment.
'except' clause does nothing but pass and there is no explanatory comment.
|
@copilot Can you please address the review recommendations? |
|
@transientlunatic I've opened a new pull request, #90, to work on those changes. Once the pull request is ready, I'll request review from you. |
) * Initial plan * Address security and code quality review comments Co-authored-by: transientlunatic <4365778+transientlunatic@users.noreply.github.com> * Add logging to exception handlers and fix production creation bugs - Add logging to bare Exception handlers to aid debugging - Fix production creation to properly instantiate Production objects - Add workaround for FileLock serialization issue in production operations - Fix production.to_dict() calls to include event=False for correct serialization - Add success tests for event creation, update, deletion, and listing - Fix CORS test to configure origins explicitly - Fix error handling test to include authentication Note: Some production tests still fail due to pre-existing architectural issue with deepcopy and FileLock when reloading ledger from disk after production creation. This requires broader refactoring beyond the scope of review comments. Co-authored-by: transientlunatic <4365778+transientlunatic@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: transientlunatic <4365778+transientlunatic@users.noreply.github.com>
…into add-restful-api
… productions API comments for clarity
…ing pipelines in known_pipelines
Closes #67