diff --git a/prs/pr-1.md b/prs/pr-1.md new file mode 100644 index 0000000..ba7138a --- /dev/null +++ b/prs/pr-1.md @@ -0,0 +1,256 @@ +# PR #1: Foundation - Dependencies and Base Classes + +## Scope + +Add Pydantic V2 dependency, migrate base classes (`BaseGithubModel`, `BaseWebhookEvent`), fix `_transform()` bug, and remove helper functions. + +## Files Changed + +- `pyproject.toml` - Add Pydantic >=2.0.0 dependency +- `octohook/models.py` - Update BaseGithubModel, update imports, remove `_optional()`, fix `_transform()` +- `octohook/events.py` - Update BaseWebhookEvent, update imports + +## Detailed Changes + +### 1. Add Pydantic Dependency + +**File:** `pyproject.toml` + +Add to dependencies: +```toml +[project] +requires-python = ">=3.10" +dependencies = ["pydantic>=2.0.0"] +``` + +Run after change: +```bash +uv lock +uv sync +``` + +### 2. Update BaseGithubModel + +**File:** `octohook/models.py` + +**Add imports at top:** +```python +from __future__ import annotations +from pydantic import BaseModel, ConfigDict, Field +``` + +**Replace BaseGithubModel:** +```python +# Remove: +from abc import ABC +class BaseGithubModel(ABC): + pass + +# Add: +class BaseGithubModel(BaseModel): + model_config = ConfigDict( + extra='allow', # Allow extra fields (resilient to GitHub changes) + populate_by_name=True, # Allow both alias and field name + strict=True, # Strict type checking (no coercion) + frozen=True, # Immutable after creation + str_strip_whitespace=False, # Preserve exact GitHub data + use_enum_values=False, # Keep enum instances for type safety + validate_default=False, # Trust our defaults + revalidate_instances='never', # Trust already-validated nested models + protected_namespaces=('model_',), # Prevent conflicts with Pydantic internals + ) +``` + +### 3. Fix _transform() Bug (CRITICAL) + +**File:** `octohook/models.py` + +**Current bug:** Uses `if not value:` which incorrectly treats `0`, `False`, and empty strings as stop conditions. + +**Fix:** +```python +def _transform(url: str, local_variables: dict) -> str: + """ + Transform URL templates by replacing placeholders with actual values. + + Handles patterns like: + - {param} -> value + - {/param} -> /value + - Stops at first None or empty value + """ + local_variables.pop("self", None) + + for key, value in local_variables.items(): + # CRITICAL: Check for None and empty string explicitly + # Don't use "if not value:" - that would incorrectly catch 0 and False + if value is None or value == "": + url = url.split(f"{{/{key}}}")[0] + break + + # Convert value to string if needed (handles int, etc.) + value_str = str(value) if not isinstance(value, str) else value + + if f"{{{key}}}" in url: + url = url.replace(f"{{{key}}}", value_str) + elif f"{{/{key}}}" in url: + url = url.replace(f"{{/{key}}}", f"/{value_str}") + + return url +``` + +### 4. Remove _optional() Helper + +**File:** `octohook/models.py` + +**Remove entirely:** +```python +# Delete this function: +def _optional(payload: dict, key: str, class_type: Type[T]) -> Optional[T]: + if payload.get(key): + return class_type(payload[key]) + else: + return None +``` + +**Remove TypeVar imports:** +```python +# Remove: +from typing import TypeVar, Type +T = TypeVar("T") +``` + +### 5. Update BaseWebhookEvent + +**File:** `octohook/events.py` + +**Add imports at top:** +```python +from __future__ import annotations +from pydantic import BaseModel, ConfigDict, ValidationError +``` + +**Replace BaseWebhookEvent:** +```python +# Remove old class with __init__ method + +# Add: +class BaseWebhookEvent(BaseModel): + model_config = ConfigDict( + extra='allow', + populate_by_name=True, + strict=True, + frozen=True, + str_strip_whitespace=False, + use_enum_values=False, + validate_default=False, + revalidate_instances='never', + protected_namespaces=('model_',), + ) + + action: Optional[str] = None + sender: Optional[User] = None + repository: Optional[Repository] = None + organization: Optional[Organization] = None + enterprise: Optional[Enterprise] = None +``` + +**Remove:** +```python +# Delete import: +from octohook.models import _optional +``` + +## Dependencies + +**None** - This is the foundation PR that all others depend on. + +## Testing Strategy + +```bash +# Verify Pydantic is installed +uv run python -c "import pydantic; print(pydantic.__version__)" + +# Import should work without errors +uv run python -c "from octohook.models import BaseGithubModel; print('BaseGithubModel OK')" +uv run python -c "from octohook.events import BaseWebhookEvent; print('BaseWebhookEvent OK')" + +# Existing tests should still pass (models not migrated yet, but base classes work) +uv run pytest tests/ -v +``` + +**Note:** Tests will likely show failures because model classes still use old `__init__` pattern. This is expected - they'll be fixed in PR #2 and #3. + +## Risk Level + +**Medium** + +- Foundation changes affect all subsequent work +- Small scope reduces risk +- Breaking changes (frozen, strict) need careful testing +- _transform() bug fix is critical but well-isolated + +## Rationale + +**Why these changes together:** +- Establishes Pydantic foundation for all subsequent PRs +- Base class changes are prerequisites for model migrations +- _transform() bug fix should be included with base infrastructure (used by all template URL models) +- Removing _optional() prevents its use in subsequent PRs + +**Why this order:** +- Must come first - all model and event classes depend on base classes +- Small scope allows quick review and merge +- Unblocks parallel work on PR #2 and #3 + +## Lines Changed (Est) + +~100-150 lines + +**Breakdown:** +- pyproject.toml: +1 line +- models.py: +30 lines (BaseGithubModel), +15 lines (_transform fix), -20 lines (_optional), -5 lines (imports) +- events.py: +25 lines (BaseWebhookEvent), +5 lines (imports), -15 lines (old __init__) + +## Rollback Instructions + +**If this PR must be reverted:** + +1. Revert the commit: +```bash +git revert +``` + +2. Remove Pydantic dependency: +```bash +# Edit pyproject.toml to remove pydantic +uv lock +uv sync +``` + +3. **CRITICAL:** All subsequent PRs (2-6) must also be reverted, as they depend on these changes. + +4. Verify rollback: +```bash +uv run pytest tests/ -v +``` + +## Additional Notes + +**Pydantic configuration rationale:** + +- **`extra='allow'`**: GitHub may add new fields - we want to be resilient, not fail. Tests will check `__pydantic_extra__` to ensure we've defined all known fields. + +- **`strict=True`**: Catches type mismatches early. If GitHub sends wrong types, we want to know immediately, not silently coerce. + +- **`frozen=True`**: Immutability is a best practice for data models. Users should not modify webhook payloads. This is a breaking change if user code modifies models post-creation. + +- **`populate_by_name=True`**: Allows using either the alias (original GitHub field name) or Python field name (e.g., `links_` or `_links`). + +**Post-merge checklist:** +- ✅ Pydantic dependency added and locked +- ✅ BaseGithubModel inherits from BaseModel +- ✅ BaseWebhookEvent inherits from BaseModel +- ✅ _transform() bug fixed +- ✅ _optional() removed +- ✅ Imports updated +- ✅ Can import base classes without errors diff --git a/prs/pr-2.md b/prs/pr-2.md new file mode 100644 index 0000000..e395031 --- /dev/null +++ b/prs/pr-2.md @@ -0,0 +1,295 @@ +# PR #2: Migrate Model Classes (Part 1 - Simple & Nested) + +## Scope + +Migrate 30 simpler model classes in `octohook/models.py` - those without template URLs or minimal complexity. + +## Files Changed + +- `octohook/models.py` - Migrate 30 model classes + +## Detailed Changes + +### Migration Pattern + +For each model, apply these transformations: + +1. **Remove `payload: dict` field** +2. **Remove `__init__` method entirely** +3. **Add `= None` defaults for all `Optional` fields** +4. **For nested models:** Just use type hint (e.g., `owner: User`) - Pydantic handles instantiation +5. **For lists:** Use `List[ModelType]` - Pydantic validates each item +6. **For lists with defaults:** Use `List[str] = []` or `List[Model] = []` +7. **Keep custom methods:** `__str__`, `__repr__`, `__eq__`, `__hash__` (only Label has custom __eq__ and __repr__) + +### Models to Migrate (30 models) + +**Simple models (no nested objects, no template URLs):** + +1. `CommitUser` +2. `Permissions` +3. `Enterprise` +4. `ShortRepository` +5. `ShortInstallation` +6. `PackageVersionInfo` +7. `VulnerablePackage` +8. `SecurityVulnerabilityIdentifier` +9. `SecurityAdvisoryReference` + +**Models with nested objects only:** + +10. `StatusBranchCommit` +11. `StatusNestedCommitUser` +12. `StatusCommitVerification` +13. `PurchaseAccount` +14. `Plan` +15. `Commit` +16. `CheckSuite` +17. `CheckRunOutput` +18. `CheckRun` +19. `DeployKey` +20. `Page` +21. `Milestone` +22. `MarketplacePurchase` +23. `Membership` +24. `Asset` +25. `ProjectCard` +26. `ProjectColumn` +27. `Project` +28. `Ref` +29. `StatusNestedCommit` +30. `StatusCommit` + +### Example: Simple Model + +**Before:** +```python +class Enterprise(BaseGithubModel): + payload: dict + id: int + slug: str + name: str + node_id: str + avatar_url: str + description: Optional[str] + website_url: Optional[str] + html_url: str + created_at: str + updated_at: str + + def __init__(self, payload: dict): + self.payload = payload + self.id = payload.get("id") + self.slug = payload.get("slug") + self.name = payload.get("name") + self.node_id = payload.get("node_id") + self.avatar_url = payload.get("avatar_url") + self.description = payload.get("description") + self.website_url = payload.get("website_url") + self.html_url = payload.get("html_url") + self.created_at = payload.get("created_at") + self.updated_at = payload.get("updated_at") + + def __str__(self): + return self.name +``` + +**After:** +```python +class Enterprise(BaseGithubModel): + id: int + slug: str + name: str + node_id: str + avatar_url: str + description: Optional[str] = None + website_url: Optional[str] = None + html_url: str + created_at: str + updated_at: str + + def __str__(self): + return self.name +``` + +### Example: Model with Nested Objects + +**Before:** +```python +class Commit(BaseGithubModel): + payload: dict + sha: str + author: CommitUser + committer: CommitUser + message: str + + def __init__(self, payload: dict): + self.payload = payload + self.sha = payload.get("sha") + self.author = CommitUser(payload.get("author")) + self.committer = CommitUser(payload.get("committer")) + self.message = payload.get("message") +``` + +**After:** +```python +class Commit(BaseGithubModel): + sha: str + author: CommitUser # Pydantic auto-validates + committer: CommitUser # Pydantic auto-validates + message: str +``` + +### Example: Model with Optional Nested and Lists + +**Before:** +```python +class CheckSuite(BaseGithubModel): + payload: dict + id: int + head_branch: Optional[str] + repository: Repository + pull_requests: List[ChecksPullRequest] + + def __init__(self, payload: dict): + self.payload = payload + self.id = payload.get("id") + self.head_branch = payload.get("head_branch") + self.repository = Repository(payload.get("repository")) + self.pull_requests = [ChecksPullRequest(pr) for pr in payload.get("pull_requests", [])] +``` + +**After:** +```python +class CheckSuite(BaseGithubModel): + id: int + head_branch: Optional[str] = None + repository: Repository # Auto-validates + pull_requests: List[ChecksPullRequest] = [] # Auto-validates each item +``` + +### Special Cases + +**Models with list comprehensions:** +- `StatusCommit.parents`: `[StatusBranchCommit(parent) for parent in ...]` → `List[StatusBranchCommit]` + +**Models with `or []` pattern:** +- Change `payload.get("topics") or []` pattern to just `List[str] = []` in field definition + +## Dependencies + +**Requires:** PR #1 (base classes) + +**Blocks:** PR #4 (events depend on models) + +**Coordination note:** Some models in this PR are referenced by models in PR #3. Migration order within this file matters: +- Migrate dependency-free models first +- Then models that reference them +- E.g., `CommitUser` before `Commit` (since Commit references CommitUser) + +## Testing Strategy + +```bash +# Run full test suite +uv run pytest -v + +# Check specific model tests +uv run pytest tests/test_models.py -v + +# Verify imports work +uv run python -c "from octohook.models import Enterprise, Commit, CheckSuite; print('OK')" + +# Test instantiation with Pydantic +uv run python -c " +from octohook.models import Enterprise +e = Enterprise.model_validate({'id': 1, 'slug': 'test', 'name': 'Test', 'node_id': 'x', 'avatar_url': 'url', 'html_url': 'url', 'created_at': '2020-01-01', 'updated_at': '2020-01-01'}) +print(f'Enterprise: {e.name}') +" +``` + +**Expected outcomes:** +- ✅ All migrated models can be instantiated via `model_validate()` +- ✅ Nested models are automatically validated +- ✅ Optional fields handle None gracefully +- ✅ List fields with defaults work correctly +- ✅ Custom __str__ methods still work + +## Risk Level + +**Low** + +- Well-established patterns from PR #1 +- Simple transformations (remove __init__, add defaults) +- Pydantic handles complexity automatically +- No template URLs or special cases in this batch + +## Rationale + +**Why these 30 models:** +- Grouped by similarity: simple models and models with nested objects +- No template URLs (those are more complex, saved for PR #3) +- Establishes migration patterns for team +- Gets bulk of "easy" models done quickly + +**Why this before PR #3:** +- Many models in PR #3 depend on models in this PR +- E.g., `User`, `Repository`, `Issue` (PR #3) all reference models from this PR +- Sequential execution prevents dependency issues + +**Why separate from PR #3:** +- Keeps PR reviewable (30 models vs 29 models) +- Allows parallel review if needed (reviewer can start on this while PR #1 is being finalized) +- Different complexity levels (simple vs template URLs) + +## Lines Changed (Est) + +~400-500 lines + +**Breakdown:** +- Remove ~600 lines (30 __init__ methods, payload fields) +- Add ~100 lines (defaults for Optional fields) +- Net reduction: ~500 lines removed + +## Rollback Instructions + +**If this PR must be reverted:** + +1. Revert the commit: +```bash +git revert +``` + +2. Verify rollback: +```bash +uv run pytest tests/ -v +``` + +3. **Note:** If PR #3 or #4 is already merged, they may need to be reverted first (reverse order). + +4. Check for breakage: +- Models will revert to old __init__ pattern +- Any code using `model_validate()` on these models will break (should be minimal at this stage) + +## Additional Notes + +**Critical reminders:** + +- **Don't remove custom methods:** Only `Label` class has custom `__eq__` and `__repr__` - preserve these! +- **List defaults are safe:** Pydantic V2 handles mutable defaults correctly (doesn't share instances) +- **Nested validation is automatic:** Don't manually instantiate nested models (e.g., `User(...)`) +- **Optional always needs = None:** Even with Pydantic, be explicit about Optional defaults + +**Model dependency order within this PR:** + +Migrate in this sequence to avoid forward references: +1. Leaf models (no dependencies): CommitUser, Permissions, Enterprise, etc. +2. Models with dependencies: Commit (needs CommitUser), CheckSuite (needs Repository), etc. + +**Post-merge checklist:** +- ✅ 30 models migrated +- ✅ No __init__ methods remain in these models +- ✅ No `payload` fields remain +- ✅ All Optional fields have `= None` +- ✅ Nested models use type hints only +- ✅ Lists use type hints with defaults +- ✅ Tests pass diff --git a/prs/pr-3.md b/prs/pr-3.md new file mode 100644 index 0000000..2a60ee6 --- /dev/null +++ b/prs/pr-3.md @@ -0,0 +1,468 @@ +# PR #3: Migrate Model Classes (Part 2 - Template URLs & Complex) + +## Scope + +Migrate remaining 29 model classes in `octohook/models.py` - those with template URLs, unstructured dicts, and special cases. + +## Files Changed + +- `octohook/models.py` - Migrate 29 remaining model classes + +## Detailed Changes + +### Migration Patterns + +This PR handles three complex patterns: + +#### Pattern 1: Template URLs + +**Before:** +```python +class User(BaseGithubModel): + payload: dict + login: str + + def __init__(self, payload: dict): + self.payload = payload + self.login = payload.get("login") + + def following_url(self, other_user: str = None) -> str: + return _transform(self.payload["following_url"], locals()) +``` + +**After:** +```python +class User(BaseGithubModel): + login: str + following_url_template: str = Field(alias="following_url") + + def following_url(self, other_user: str = None) -> str: + return _transform(self.following_url_template, locals()) +``` + +**Key changes:** +1. Field name: `{method_name}_template` +2. Add alias: `Field(alias="{original_name}")` +3. Update method to use `self.{field_name}_template` +4. Keep method signature unchanged + +#### Pattern 2: Unstructured Dicts + +**Before:** +```python +class Comment(BaseGithubModel): + payload: dict + reactions: Optional[Annotated[dict, "unstructured"]] + _links: Optional[Annotated[dict, "unstructured"]] +``` + +**After:** +```python +class Comment(BaseGithubModel): + reactions: Optional[dict[str, Any]] = None + links_: Optional[dict[str, Any]] = Field(None, alias="_links") +``` + +**Key changes:** +1. `Annotated[dict, "unstructured"]` → `dict[str, Any]` +2. Fields starting with `_` must be renamed: `_links` → `links_` with `Field(alias="_links")` + +#### Pattern 3: Field Name Collision (Deployment only) + +**Before:** +```python +class Deployment(BaseGithubModel): + payload: dict + payload: Annotated[dict, "unstructured"] # Collision! +``` + +**After:** +```python +class Deployment(BaseGithubModel): + payload_data: dict[str, Any] = Field(alias="payload") +``` + +### Models to Migrate (29 models) + +**Models with template URLs (6 models):** + +1. **User** - 4 template URLs: following_url, gists_url, starred_url, subscriptions_url +2. **Organization** - 2 template URLs: repos_url, issues_url +3. **Repository** - 19 template URLs (most complex!) +4. **Issue** - 1 template URL: labels_url +5. **Team** - 1 template URL: repositories_url +6. **PullRequest** - 1 template URL: review_comments_url + +**Models with unstructured dicts (5 models):** + +7. **Comment** - `_links`, `reactions` +8. **Hook** - `config` +9. **PageBuild** - `error` +10. **Review** - `_links` +11. **ChecksPullRequest** - `head`, `base` + +**Complex models with multiple features (17 models):** + +12. **Installation** - nested objects, lists +13. **Label** - **SPECIAL: Has custom __eq__ and __repr__ - MUST preserve!** +14. **Thread** - list comprehension: `comments` +15. **Issue** - list comprehensions: `labels`, `assignees` +16. **Release** - list comprehension: `assets` +17. **PullRequest** - list comprehensions: `assignees`, `labels` +18. **Deployment** - **SPECIAL: Field name collision! (payload)** +19. **DeploymentStatus** +20. **VulnerabilityAlert** +21. **Vulnerability** +22. **SecurityAdvisory** +23. **PackageFile** +24. **PackageVersion** +25. **Registry** +26. **Package** +27. **SponsorshipTier** +28. **Sponsorship** +29. **Branch** +30. **Rule** +31. **ChecksApp** + +**Note:** Count is actually 31 models (adjusted from original estimate). + +### Example: User with Template URLs + +**Before:** +```python +class User(BaseGithubModel): + payload: dict + login: str + id: int + # ... other fields + + def __init__(self, payload: dict): + self.payload = payload + self.login = payload.get("login") + self.id = payload.get("id") + # ... + + def following_url(self, other_user: str = None) -> str: + return _transform(self.payload["following_url"], locals()) + + def gists_url(self, gist_id: str = None) -> str: + return _transform(self.payload["gists_url"], locals()) + + def starred_url(self, owner: str = None, repo: str = None) -> str: + return _transform(self.payload["starred_url"], locals()) + + def subscriptions_url(self) -> str: + return self.payload["subscriptions_url"] +``` + +**After:** +```python +class User(BaseGithubModel): + login: str + id: int + # ... other fields + + # Template URL fields + following_url_template: str = Field(alias="following_url") + gists_url_template: str = Field(alias="gists_url") + starred_url_template: str = Field(alias="starred_url") + subscriptions_url_template: str = Field(alias="subscriptions_url") + + def following_url(self, other_user: str = None) -> str: + return _transform(self.following_url_template, locals()) + + def gists_url(self, gist_id: str = None) -> str: + return _transform(self.gists_url_template, locals()) + + def starred_url(self, owner: str = None, repo: str = None) -> str: + return _transform(self.starred_url_template, locals()) + + def subscriptions_url(self) -> str: + return self.subscriptions_url_template +``` + +### Example: Comment with Unstructured Dicts + +**Before:** +```python +class Comment(BaseGithubModel): + payload: dict + id: int + body: str + reactions: Optional[Annotated[dict, "unstructured"]] + _links: Optional[Annotated[dict, "unstructured"]] + + def __init__(self, payload: dict): + self.payload = payload + self.id = payload.get("id") + self.body = payload.get("body") + self.reactions = payload.get("reactions") + self._links = payload.get("_links") +``` + +**After:** +```python +from typing import Any + +class Comment(BaseGithubModel): + id: int + body: str + reactions: Optional[dict[str, Any]] = None + links_: Optional[dict[str, Any]] = Field(None, alias="_links") +``` + +### Example: Issue with List Comprehensions + +**Before:** +```python +class Issue(BaseGithubModel): + payload: dict + number: int + labels: List[Label] + assignees: List[User] + + def __init__(self, payload: dict): + self.payload = payload + self.number = payload.get("number") + self.labels = [Label(label) for label in payload.get("labels", [])] + self.assignees = [User(assignee) for assignee in payload.get("assignees", [])] +``` + +**After:** +```python +class Issue(BaseGithubModel): + number: int + labels: List[Label] = [] # Pydantic validates each Label + assignees: List[User] = [] # Pydantic validates each User +``` + +### Example: Deployment with Field Collision + +**Before:** +```python +class Deployment(BaseGithubModel): + payload: dict + url: str + id: int + payload: Annotated[dict, "unstructured"] # Collision! + + def __init__(self, payload: dict): + self.payload = payload + self.url = payload.get("url") + self.id = payload.get("id") + self.payload = payload.get("payload") # Overwrites! +``` + +**After:** +```python +class Deployment(BaseGithubModel): + url: str + id: int + payload_data: dict[str, Any] = Field(alias="payload") +``` + +### Special Cases to Remember + +**1. Label class - Preserve custom methods:** +```python +class Label(BaseGithubModel): + # ... fields ... + + def __eq__(self, other): + # KEEP THIS! + if isinstance(other, Label): + return self.name == other.name + return False + + def __repr__(self): + # KEEP THIS! + return f"Label(name='{self.name}', color='{self.color}')" +``` + +**2. Repository - 19 template URLs:** +Most complex model in the codebase. Template URLs include: +- assignees_url, blobs_url, branches_url, collaborators_url, comments_url +- commits_url, compare_url, contents_url, contributors_url, deployments_url +- downloads_url, events_url, forks_url, git_commits_url, git_refs_url +- git_tags_url, hooks_url, issue_comment_url, issue_events_url + +**3. Models with `_links` fields:** +Must rename to `links_` with `Field(alias="_links")`: +- Comment +- Review +- (Any other model with underscore-prefixed field) + +## Dependencies + +**Requires:** +- PR #1 (base classes) +- PR #2 (many models in this PR reference models from PR #2) + +**Blocks:** PR #4 (events depend on models) + +## Testing Strategy + +```bash +# Run full test suite +uv run pytest -v + +# Test template URL models specifically +uv run python -c " +from octohook.models import User +u = User.model_validate({ + 'login': 'test', + 'id': 123, + 'following_url': 'https://api.github.com/users/test/following{/other_user}', + 'gists_url': 'https://api.github.com/users/test/gists{/gist_id}', + 'starred_url': 'https://api.github.com/users/test/starred{/owner}{/repo}', + 'subscriptions_url': 'https://api.github.com/users/test/subscriptions', + # ... other required fields +}) +print(u.following_url('johndoe')) # Should return URL with 'johndoe' +print(u.gists_url()) # Should return base URL +" + +# Test unstructured dict aliasing +uv run python -c " +from octohook.models import Comment +c = Comment.model_validate({ + 'id': 1, + 'body': 'test', + '_links': {'self': 'url'}, + 'reactions': {'+1': 5} +}) +print(c.links_) # Should print {'self': 'url'} +print(c.reactions) # Should print {'+1': 5} +" + +# Test Label custom methods +uv run python -c " +from octohook.models import Label +l1 = Label.model_validate({'id': 1, 'name': 'bug', 'color': 'red', 'node_id': 'x', 'url': 'url', 'default': False}) +l2 = Label.model_validate({'id': 2, 'name': 'bug', 'color': 'blue', 'node_id': 'y', 'url': 'url', 'default': False}) +print(l1 == l2) # Should be True (same name) +print(repr(l1)) # Should show custom repr +" + +# Test Deployment field collision resolution +uv run python -c " +from octohook.models import Deployment +d = Deployment.model_validate({ + 'url': 'http://example.com', + 'id': 1, + 'payload': {'key': 'value'}, # This goes to payload_data + # ... other required fields +}) +print(d.payload_data) # Should print {'key': 'value'} +" +``` + +**Expected outcomes:** +- ✅ Template URL methods work correctly with parameters +- ✅ Unstructured dicts accessible via renamed fields +- ✅ Field aliases work (can pass both `_links` and `links_`) +- ✅ Label equality and repr work correctly +- ✅ Deployment.payload_data receives the "payload" field +- ✅ All tests pass + +## Risk Level + +**Medium** + +- Template URLs are more complex than simple models +- Field aliasing must be correct or data won't parse +- Deployment field collision is a tricky edge case +- Repository has 19 template URLs (error-prone) +- Custom Label methods must be preserved + +## Rationale + +**Why these 29 models:** +- All models with special complexity (template URLs, unstructured dicts) +- Separates "hard" models from "easy" models (PR #2) +- Allows focused review on complex patterns +- Deployment field collision is unique edge case + +**Why after PR #2:** +- Many models here depend on models from PR #2 (e.g., Repository → User, Issue → Label) +- Sequential execution prevents dependency issues +- Reviewers can reference PR #2 patterns for basic structure + +**Why separate from PR #2:** +- Different complexity level requires different review focus +- Template URL pattern is distinct from simple models +- Keeps each PR reviewable (~400-500 lines each) + +## Lines Changed (Est) + +~400-500 lines + +**Breakdown:** +- Remove ~700 lines (29 __init__ methods, payload fields) +- Add ~200 lines (template URL fields with aliases, dict[str, Any] types) +- Net reduction: ~500 lines removed + +## Rollback Instructions + +**If this PR must be reverted:** + +1. Revert the commit: +```bash +git revert +``` + +2. **Note:** If PR #4 is already merged, it must be reverted first. + +3. Verify rollback: +```bash +uv run pytest tests/ -v +``` + +4. Check template URL functionality: +```bash +# Ensure old pattern works +uv run python -c "from octohook.models import User; print('User import OK')" +``` + +## Additional Notes + +**Critical reminders:** + +- **Label class is special:** Custom `__eq__` and `__repr__` must be preserved! +- **Template URL field naming:** Must be `{method_name}_template`, not just `{method_name}` +- **Field aliases are case-sensitive:** `Field(alias="following_url")` must match GitHub exactly +- **Underscore fields:** Pydantic forbids field names starting with `_` - must use aliases +- **Deployment is unique:** Only model with field name collision - use `payload_data` + +**Repository template URLs (19 total):** +```python +assignees_url_template: str = Field(alias="assignees_url") +blobs_url_template: str = Field(alias="blobs_url") +branches_url_template: str = Field(alias="branches_url") +collaborators_url_template: str = Field(alias="collaborators_url") +comments_url_template: str = Field(alias="comments_url") +commits_url_template: str = Field(alias="commits_url") +compare_url_template: str = Field(alias="compare_url") +contents_url_template: str = Field(alias="contents_url") +contributors_url_template: str = Field(alias="contributors_url") +deployments_url_template: str = Field(alias="deployments_url") +downloads_url_template: str = Field(alias="downloads_url") +events_url_template: str = Field(alias="events_url") +forks_url_template: str = Field(alias="forks_url") +git_commits_url_template: str = Field(alias="git_commits_url") +git_refs_url_template: str = Field(alias="git_refs_url") +git_tags_url_template: str = Field(alias="git_tags_url") +hooks_url_template: str = Field(alias="hooks_url") +issue_comment_url_template: str = Field(alias="issue_comment_url") +issue_events_url_template: str = Field(alias="issue_events_url") +``` + +**Post-merge checklist:** +- ✅ 29 models migrated +- ✅ All template URL fields have `_template` suffix and aliases +- ✅ All template URL methods updated to use `self.*_template` +- ✅ All underscore fields renamed with aliases +- ✅ Label custom methods preserved +- ✅ Deployment uses `payload_data` field +- ✅ All unstructured dicts use `dict[str, Any]` +- ✅ Tests pass diff --git a/prs/pr-4.md b/prs/pr-4.md new file mode 100644 index 0000000..986ffb6 --- /dev/null +++ b/prs/pr-4.md @@ -0,0 +1,394 @@ +# PR #4: Migrate Event Classes + +## Scope + +Migrate all 48 event classes in `octohook/events.py` to use Pydantic models instead of manual initialization. + +## Files Changed + +- `octohook/events.py` - Migrate 48 event classes + +## Detailed Changes + +### Migration Pattern + +For **ALL** event classes, apply this transformation: + +**Before:** +```python +class PullRequestEvent(BaseWebhookEvent): + number: int + pull_request: PullRequest + assignee: Optional[User] + label: Optional[Label] + changes: Optional[Annotated[dict, "unstructured"]] + before: Optional[str] + after: Optional[str] + requested_reviewer: Optional[User] + + def __init__(self, payload: dict): + super().__init__(payload) + self.number = payload.get("number") + self.pull_request = PullRequest(payload.get("pull_request")) + self.assignee = _optional(payload, "assignee", User) + self.label = _optional(payload, "label", Label) + self.changes = payload.get("changes") + self.before = payload.get("before") + self.after = payload.get("after") + self.requested_reviewer = _optional(payload, "requested_reviewer", User) +``` + +**After:** +```python +class PullRequestEvent(BaseWebhookEvent): + number: int + pull_request: PullRequest + assignee: Optional[User] = None + label: Optional[Label] = None + changes: Optional[dict[str, Any]] = None + before: Optional[str] = None + after: Optional[str] = None + requested_reviewer: Optional[User] = None +``` + +**Transformation steps:** +1. ✅ Remove `__init__` entirely (including `super().__init__()`) +2. ✅ Keep all field definitions +3. ✅ Add `= None` to all `Optional` fields +4. ✅ Change `Annotated[dict, "unstructured"]` → `dict[str, Any]` +5. ✅ Keep docstrings if present + +### Event Classes to Migrate (48 total) + +1. `BranchProtectionRuleEvent` +2. `CheckRunEvent` +3. `CheckSuiteEvent` +4. `CommitCommentEvent` +5. `CreateEvent` +6. `DeleteEvent` +7. `DeployKeyEvent` +8. `DeploymentEvent` +9. `DeploymentStatusEvent` +10. `ForkEvent` +11. `GitHubAppAuthorizationEvent` +12. `GollumEvent` +13. `InstallationEvent` +14. `InstallationRepositoriesEvent` +15. `IssueCommentEvent` +16. `IssuesEvent` +17. `LabelEvent` +18. `MarketplacePurchaseEvent` +19. `MemberEvent` +20. `MembershipEvent` +21. `MetaEvent` +22. `MilestoneEvent` +23. `OrganizationEvent` +24. `OrgBlockEvent` +25. `PackageEvent` +26. `PageBuildEvent` +27. `ProjectCardEvent` +28. `ProjectColumnEvent` +29. `ProjectEvent` +30. `PublicEvent` +31. `PullRequestEvent` +32. `PullRequestReviewEvent` +33. `PullRequestReviewCommentEvent` +34. `PullRequestReviewThreadEvent` +35. `PushEvent` +36. `ReleaseEvent` +37. `RepositoryDispatchEvent` +38. `RepositoryEvent` +39. `RepositoryImportEvent` +40. `RepositoryVulnerabilityAlertEvent` +41. `SecurityAdvisoryEvent` ⚠️ **Special case** +42. `SponsorshipEvent` +43. `StarEvent` +44. `StatusEvent` +45. `TeamEvent` +46. `TeamAddEvent` +47. `WatchEvent` +48. `PingEvent` + +### Special Case: SecurityAdvisoryEvent + +**Note:** `SecurityAdvisoryEvent` does NOT inherit from `BaseWebhookEvent` (it's a standalone Pydantic model). + +**Before:** +```python +class SecurityAdvisoryEvent: + payload: dict + action: str + security_advisory: SecurityAdvisory + + def __init__(self, payload: dict): + self.payload = payload + self.action = payload.get("action") + self.security_advisory = SecurityAdvisory(payload.get("security_advisory")) +``` + +**After:** +```python +from pydantic import BaseModel, ConfigDict + +class SecurityAdvisoryEvent(BaseModel): + model_config = ConfigDict( + extra='allow', + populate_by_name=True, + strict=True, + frozen=True, + str_strip_whitespace=False, + use_enum_values=False, + validate_default=False, + revalidate_instances='never', + protected_namespaces=('model_',), + ) + + action: str + security_advisory: SecurityAdvisory +``` + +### Example: Simple Event + +**Before:** +```python +class CreateEvent(BaseWebhookEvent): + ref: str + ref_type: str + master_branch: str + description: Optional[str] + pusher_type: str + + def __init__(self, payload: dict): + super().__init__(payload) + self.ref = payload.get("ref") + self.ref_type = payload.get("ref_type") + self.master_branch = payload.get("master_branch") + self.description = payload.get("description") + self.pusher_type = payload.get("pusher_type") +``` + +**After:** +```python +class CreateEvent(BaseWebhookEvent): + ref: str + ref_type: str + master_branch: str + description: Optional[str] = None + pusher_type: str +``` + +### Example: Event with Nested Models + +**Before:** +```python +class IssuesEvent(BaseWebhookEvent): + issue: Issue + assignee: Optional[User] + label: Optional[Label] + changes: Optional[Annotated[dict, "unstructured"]] + + def __init__(self, payload: dict): + super().__init__(payload) + self.issue = Issue(payload.get("issue")) + self.assignee = _optional(payload, "assignee", User) + self.label = _optional(payload, "label", Label) + self.changes = payload.get("changes") +``` + +**After:** +```python +class IssuesEvent(BaseWebhookEvent): + issue: Issue # Pydantic auto-validates + assignee: Optional[User] = None + label: Optional[Label] = None + changes: Optional[dict[str, Any]] = None +``` + +### Example: Event with Lists + +**Before:** +```python +class GollumEvent(BaseWebhookEvent): + pages: List[Page] + + def __init__(self, payload: dict): + super().__init__(payload) + self.pages = [Page(page) for page in payload.get("pages", [])] +``` + +**After:** +```python +class GollumEvent(BaseWebhookEvent): + pages: List[Page] = [] # Pydantic validates each Page +``` + +## Dependencies + +**Requires:** +- PR #1 (BaseWebhookEvent) +- PR #2 (simple models) +- PR #3 (complex models) + +**Blocks:** PR #5 (parse function needs migrated events) + +**Critical:** All model classes must be migrated before event classes, since events reference models. + +## Testing Strategy + +```bash +# Run full test suite +uv run pytest -v + +# Test event parsing specifically +uv run pytest tests/test_models.py::test_model_has_all_keys_in_json -v + +# Test event instantiation +uv run python -c " +from octohook.events import PullRequestEvent +event = PullRequestEvent.model_validate({ + 'action': 'opened', + 'number': 123, + 'pull_request': { + 'id': 1, + 'number': 123, + 'state': 'open', + 'title': 'Test PR', + # ... other required fields + }, + # ... other fields +}) +print(f'Event action: {event.action}') +print(f'PR number: {event.number}') +" + +# Test SecurityAdvisoryEvent (special case) +uv run python -c " +from octohook.events import SecurityAdvisoryEvent +event = SecurityAdvisoryEvent.model_validate({ + 'action': 'published', + 'security_advisory': { + 'ghsa_id': 'GHSA-xxxx-xxxx-xxxx', + 'summary': 'Test advisory', + # ... other required fields + } +}) +print(f'Advisory action: {event.action}') +" + +# Verify BaseWebhookEvent fields inherited correctly +uv run python -c " +from octohook.events import IssuesEvent +event = IssuesEvent.model_validate({ + 'action': 'opened', + 'issue': {...}, # Required issue data + 'sender': {...}, # BaseWebhookEvent field + 'repository': {...}, # BaseWebhookEvent field +}) +print(f'Has sender: {event.sender is not None}') +print(f'Has repository: {event.repository is not None}') +" +``` + +**Expected outcomes:** +- ✅ All 48 events can be instantiated via `model_validate()` +- ✅ Nested models (User, Label, Issue, etc.) are auto-validated +- ✅ Optional fields handle None gracefully +- ✅ List fields auto-validate each item +- ✅ BaseWebhookEvent fields (sender, repository, etc.) inherited correctly +- ✅ SecurityAdvisoryEvent works as standalone model +- ✅ All tests pass + +## Risk Level + +**Low** + +- Straightforward pattern (simpler than models with template URLs) +- All dependencies (models) are already migrated +- BaseWebhookEvent provides consistent foundation +- Well-tested webhook fixtures catch issues + +## Rationale + +**Why all 48 events together:** +- Events are simpler than models (no template URLs, fewer special cases) +- Consistent pattern across all events (remove __init__, add defaults) +- Logical unit: "all event parsing" in one PR +- Events depend on models, so must come after PR #2 and #3 + +**Why after model PRs:** +- Events reference models (User, Issue, PullRequest, etc.) +- Must wait for all models to be migrated +- Clean dependency chain: base → models → events + +**Why before parse function:** +- Parse function instantiates events +- Better to have events migrated before changing parse logic +- Allows testing events with old parse, then update parse separately + +## Lines Changed (Est) + +~300-400 lines + +**Breakdown:** +- Remove ~600 lines (48 __init__ methods) +- Add ~200 lines (defaults for Optional fields) +- Net reduction: ~400 lines removed + +## Rollback Instructions + +**If this PR must be reverted:** + +1. Revert the commit: +```bash +git revert +``` + +2. **Note:** If PR #5 is already merged, it should be reverted first. + +3. Verify rollback: +```bash +uv run pytest tests/ -v +``` + +4. Check event instantiation: +```bash +# Ensure old pattern works +uv run python -c "from octohook.events import PullRequestEvent; print('Import OK')" +``` + +## Additional Notes + +**Critical reminders:** + +- **super().__init__(payload)** must be removed - Pydantic handles parent initialization +- **All Optional fields need = None** - even though inherited fields from BaseWebhookEvent have defaults +- **Nested models are automatic** - no `User(...)` or `_optional(...)` needed +- **List comprehensions go away** - `[Page(p) for p in ...]` becomes just `List[Page]` +- **SecurityAdvisoryEvent is special** - needs full `model_config` since it doesn't inherit from BaseWebhookEvent + +**Event inheritance:** +All events (except SecurityAdvisoryEvent) inherit these fields from BaseWebhookEvent: +- `action: Optional[str] = None` +- `sender: Optional[User] = None` +- `repository: Optional[Repository] = None` +- `organization: Optional[Organization] = None` +- `enterprise: Optional[Enterprise] = None` + +**Events with unstructured dicts:** +Several events have `changes` field: +- `IssuesEvent` +- `PullRequestEvent` +- `MilestoneEvent` +- Others + +Change `Optional[Annotated[dict, "unstructured"]]` to `Optional[dict[str, Any]] = None` + +**Post-merge checklist:** +- ✅ 48 event classes migrated +- ✅ No __init__ methods remain +- ✅ No super().__init__() calls remain +- ✅ All Optional fields have `= None` +- ✅ All Annotated[dict, "unstructured"] changed to dict[str, Any] +- ✅ SecurityAdvisoryEvent has model_config +- ✅ Tests pass +- ✅ All webhook fixtures parse correctly diff --git a/prs/pr-5.md b/prs/pr-5.md new file mode 100644 index 0000000..2cc0b8b --- /dev/null +++ b/prs/pr-5.md @@ -0,0 +1,239 @@ +# PR #5: Update Parse Function + +## Scope + +Update the `parse()` function in `octohook/events.py` to use Pydantic's `model_validate()` method instead of direct instantiation. Review `decorators.py` for any needed changes. + +## Files Changed + +- `octohook/events.py` - Update `parse()` function +- `octohook/decorators.py` - Review (likely no changes) + +## Detailed Changes + +### 1. Update parse() Function + +**File:** `octohook/events.py` + +**Before:** +```python +def parse(event_name, payload: dict): + try: + return event_map[WebhookEvent(event_name)](payload) + except Exception: + logger.exception(f"Could not parse event {event_name}") + return BaseWebhookEvent(payload) +``` + +**After:** +```python +def parse(event_name, payload: dict): + try: + event_class = event_map[WebhookEvent(event_name)] + return event_class.model_validate(payload) + except (KeyError, ValidationError) as e: + logger.exception(f"Could not parse event {event_name}: {e}") + return BaseWebhookEvent.model_validate(payload) +``` + +**Key changes:** +1. ✅ Use `model_validate()` instead of direct call `(payload)` +2. ✅ Catch specific exceptions: `KeyError` (unknown event), `ValidationError` (bad data) +3. ✅ Include exception in log message for better debugging +4. ✅ Apply `model_validate()` to fallback `BaseWebhookEvent` as well + +**Note:** `ValidationError` was already imported in PR #1 (Step 3.1). + +### 2. Review decorators.py + +**File:** `octohook/decorators.py` + +**Analysis:** +The decorator system's `handle_webhook()` function calls `parse()` internally: + +```python +def handle_webhook(event_name: str, payload: dict): + event = parse(event_name, payload) + # ... dispatch to registered handlers +``` + +**Expected result:** No changes needed. The `parse()` function returns an event object, and the decorator doesn't care how it was instantiated. + +**Verification:** Review the file to confirm no changes are needed, then note in PR description that decorators.py was reviewed and requires no modifications. + +## Dependencies + +**Requires:** +- PR #1 (BaseWebhookEvent and ValidationError import) +- PR #2 (model migrations) +- PR #3 (model migrations) +- PR #4 (event migrations) + +**Blocks:** None (PR #6 can proceed independently) + +**Critical:** All models and events must be migrated before updating parse(), since parse() instantiates events which reference models. + +## Testing Strategy + +```bash +# Run full test suite +uv run pytest -v + +# Test parse function with real webhook payloads +uv run python -c " +from octohook.events import parse +import json + +# Test valid event +payload = { + 'action': 'opened', + 'number': 123, + 'pull_request': { + 'id': 1, + 'number': 123, + 'state': 'open', + 'title': 'Test', + # ... minimal required fields + } +} +event = parse('pull_request', payload) +print(f'Parsed event type: {type(event).__name__}') +print(f'Event action: {event.action}') +" + +# Test unknown event fallback +uv run python -c " +from octohook.events import parse, BaseWebhookEvent + +payload = {'action': 'unknown_action', 'sender': {...}} +event = parse('unknown_event', payload) +print(f'Fallback type: {type(event).__name__}') +print(f'Is BaseWebhookEvent: {isinstance(event, BaseWebhookEvent)}') +" + +# Test validation error handling +uv run python -c " +from octohook.events import parse +import json + +# Invalid payload (missing required fields) +payload = {'action': 'opened'} # Missing required pull_request field +event = parse('pull_request', payload) +print(f'Fallback on error: {type(event).__name__}') +" + +# Test with actual webhook fixtures +uv run pytest tests/test_models.py::test_model_has_all_keys_in_json -v -k pull_request +``` + +**Expected outcomes:** +- ✅ Valid events parse correctly using `model_validate()` +- ✅ Unknown events fall back to `BaseWebhookEvent` +- ✅ Invalid payloads are caught and logged, falling back to `BaseWebhookEvent` +- ✅ All webhook fixtures parse without errors +- ✅ Decorator system continues to work (handlers receive proper event objects) +- ✅ All tests pass + +## Risk Level + +**Low** + +- Small, focused change (single function) +- Well-isolated (only affects parsing logic) +- Fallback behavior preserved (unknown events → BaseWebhookEvent) +- All dependencies (models, events) already migrated and tested + +## Rationale + +**Why separate PR:** +- Small, logical unit: "how we instantiate events" +- Easy to review (< 10 lines changed) +- Can be quickly approved and merged +- Allows testing models/events with old parse logic first + +**Why after event migrations:** +- Events must be Pydantic models before we can call `model_validate()` +- Clean separation: "migrate events" → "update how we instantiate them" +- Reduces risk: events are tested with old parse before new parse is introduced + +**Why before tests PR:** +- Parse function changes affect test behavior +- Better to update parse first, then update tests to match +- Tests in PR #6 will validate the new parse logic + +## Lines Changed (Est) + +~10-15 lines + +**Breakdown:** +- `parse()` function: 5 lines changed +- `decorators.py`: 0 lines (review only, no changes expected) +- Documentation/comments: ~5 lines if needed + +## Rollback Instructions + +**If this PR must be reverted:** + +1. Revert the commit: +```bash +git revert +``` + +2. Verify rollback: +```bash +uv run pytest tests/ -v +``` + +3. **Note:** This PR is independent enough that reverting it doesn't require reverting other PRs (unless tests in PR #6 specifically depend on new parse behavior). + +4. Check parse functionality: +```bash +# Ensure old pattern works +uv run python -c "from octohook.events import parse; print('Parse import OK')" +``` + +## Additional Notes + +**Why model_validate() instead of model_validate_json()?** + +- GitHub webhooks are received as Python dicts (already parsed from JSON) +- `model_validate()` accepts dicts +- `model_validate_json()` would require re-serializing to JSON string (inefficient) + +**Error handling improvements:** + +The new version provides better error information: +- **Before:** Generic `Exception` catch → hard to debug +- **After:** Specific `KeyError` (unknown event) and `ValidationError` (bad data) → clear error type +- **Logging:** Exception included in message → easier debugging + +**Fallback behavior:** + +Both old and new versions fall back to `BaseWebhookEvent` on error, but new version: +- Uses `model_validate()` for consistency +- Provides more specific error logging +- Catches validation errors explicitly + +**Integration with decorators:** + +The decorator system calls `parse()` and expects an event object back: +```python +def handle_webhook(event_name: str, payload: dict): + event = parse(event_name, payload) # Returns event object + # ... filters and dispatches to handlers +``` + +No changes needed in decorators because: +- Return type doesn't change (still returns event object) +- Event object interface doesn't change (same attributes and methods) +- Pydantic models are transparent to the decorator system + +**Post-merge checklist:** +- ✅ `parse()` uses `model_validate()` +- ✅ `BaseWebhookEvent` fallback uses `model_validate()` +- ✅ Catches `KeyError` and `ValidationError` specifically +- ✅ Log messages include exception details +- ✅ `decorators.py` reviewed (no changes needed) +- ✅ All webhook fixtures parse correctly +- ✅ Tests pass +- ✅ Error cases fall back gracefully diff --git a/prs/pr-6.md b/prs/pr-6.md new file mode 100644 index 0000000..4b90c76 --- /dev/null +++ b/prs/pr-6.md @@ -0,0 +1,469 @@ +# PR #6: Update Test Infrastructure and Cleanup + +## Scope + +Update test validation logic to use Pydantic's `__pydantic_extra__` checking, remove obsolete validation functions, cleanup unused imports, and verify final migration state. + +## Files Changed + +- `tests/test_models.py` - Replace validation logic, remove obsolete tests +- `tests/conftest.py` - Review and update if needed +- `octohook/__init__.py` - Verify exports +- `octohook/models.py` - Remove unused imports +- `octohook/events.py` - Remove unused imports +- `pyproject.toml` - Optional: Add mypy for type checking + +## Detailed Changes + +### 1. Update test_models.py + +**File:** `tests/test_models.py` + +#### Change 1: Update Imports + +**Add:** +```python +from pydantic import BaseModel +``` + +**Remove:** +```python +from typing import get_origin, get_args, Annotated, Union +``` + +#### Change 2: Replace check_model() with check_no_extra_fields() + +**Remove entire function:** +```python +def check_model(data, obj): + """ + Checks if every key in the json is represented either as an + Annotated[dict, "unstructured"] or a nested object. + ... + """ + # ... entire function (many lines) +``` + +**Add instead:** +```python +def check_no_extra_fields(obj): + """ + Verify that all fields from the JSON payload are defined in the model. + + When extra='allow', Pydantic stores undefined fields in __pydantic_extra__. + This test ensures we've defined all fields that GitHub sends. + """ + from pydantic import BaseModel + + # Only check Pydantic models + if not isinstance(obj, BaseModel): + return + + extra_fields = getattr(obj, '__pydantic_extra__', None) + + if extra_fields: + raise AssertionError( + f"{type(obj).__name__} has undefined fields: {list(extra_fields.keys())}\n" + f"These fields exist in the webhook payload but are not defined in the model." + ) + + # Recursively check nested models + for field_name in obj.model_fields: + value = getattr(obj, field_name) + + if isinstance(value, BaseModel): + check_no_extra_fields(value) + elif isinstance(value, list): + for item in value: + if isinstance(item, BaseModel): + check_no_extra_fields(item) +``` + +#### Change 3: Update test_model_has_all_keys_in_json + +**Before:** +```python +@pytest.mark.parametrize("event_name", testcases) +def test_model_has_all_keys_in_json(event_name, fixture_loader): + """ + Verify that every JSON key from GitHub is accessible in the parsed object. + ... + """ + examples = fixture_loader.load(event_name) + for example in examples: + check_model(example, parse(event_name, example)) +``` + +**After:** +```python +@pytest.mark.parametrize("event_name", testcases) +def test_model_has_all_keys_in_json(event_name, fixture_loader): + """ + Verify that every JSON key from GitHub is defined in the model. + + With extra='allow', this test ensures we haven't missed any fields + by checking that __pydantic_extra__ is empty. + """ + examples = fixture_loader.load(event_name) + for example in examples: + event = parse(event_name, example) + check_no_extra_fields(event) +``` + +#### Change 4: Remove Utility Functions + +**Remove entirely:** +```python +def _unwrap_annotated(): + # ... + +def _is_unstructured_dict(): + # ... + +def _is_primitive_type(): + # ... +``` + +**Rationale:** These were used by `check_model()`. Pydantic handles type validation now. + +#### Change 5: Remove Type Hint Validation + +**Remove entirely:** +```python +def _validate_simple_type(): + # ... + +def _validate_list_items(): + # ... + +def _validate_complex_type(): + # ... + +def check_type_hints(): + # ... + +def test_all_type_hints_are_correct(): + # ... +``` + +**Rationale:** Pydantic V2's strict mode provides more thorough type validation than our custom checker. + +#### Change 6: Remove Unannotated Dict Test + +**Remove entirely:** +```python +def test_unannotated_dict_enforcement(): + """ + Verify that check_model enforces Annotated[dict, "unstructured"] requirement. + ... + """ + # ... entire test +``` + +**Rationale:** With `dict[str, Any]` and `__pydantic_extra__` checking, this test is obsolete. + +### 2. Update conftest.py (if needed) + +**File:** `tests/conftest.py` + +**Review for:** +- Any direct model instantiation (change to `model_validate()`) +- Any fixtures that might need updating +- The autouse fixture should still work (calls `reset()` which clears handlers) + +**Expected result:** Most likely no changes needed. If there is direct instantiation like `User(payload)`, change to `User.model_validate(payload)`. + +### 3. Cleanup Unused Imports + +**File:** `octohook/models.py` + +**Remove if present:** +```python +from abc import ABC +from typing import TypeVar, Type + +T = TypeVar("T") +``` + +**Verify not needed:** +- `ABC` - removed when BaseGithubModel switched to BaseModel +- `TypeVar`, `Type` - used by `_optional()` which was removed + +**File:** `octohook/events.py` + +**Remove if present:** +```python +from octohook.models import _optional +``` + +**Verify not needed:** +- `_optional()` was removed in PR #1 and is no longer used + +### 4. Verify Exports + +**File:** `octohook/__init__.py` + +**Check exports remain unchanged:** +```python +__all__ = [ + "events", + "handle_webhook", + "hook", + "models", + "OctohookConfigError", + "parse", + "reset", + "setup", + "WebhookEvent", + "WebhookEventAction", +] +``` + +**Expected result:** No changes needed. Public API is preserved. + +### 5. Optional: Add Type Checking with mypy + +**File:** `pyproject.toml` + +**Add to dev dependencies:** +```toml +[dependency-groups] +dev = [ + "pre-commit>=4.1.0", + "pytest>=8.3.4", + "pytest-mock>=3.14.0", + "pytest-cov>=6.0.0", + "mypy>=1.0.0", # NEW +] +``` + +**Add mypy configuration:** +```toml +[tool.mypy] +python_version = "3.10" +warn_return_any = true +warn_unused_configs = true +disallow_untyped_defs = false +disallow_incomplete_defs = false +check_untyped_defs = true +no_implicit_optional = true +warn_redundant_casts = true +warn_unused_ignores = true +warn_no_return = true +strict_equality = true + +[[tool.mypy.overrides]] +module = "tests.*" +disallow_untyped_defs = false +``` + +**Run after adding:** +```bash +uv sync +uv run mypy octohook +``` + +**Note:** This is optional but recommended for catching type issues early. + +## Dependencies + +**Requires:** +- PR #1 (base classes) +- PR #2 (model migrations) +- PR #3 (model migrations) +- PR #4 (event migrations) +- PR #5 (parse function) + +**Blocks:** None (final PR in sequence) + +## Testing Strategy + +```bash +# Run full test suite +uv run pytest -v + +# Run with coverage +uv run pytest --cov=octohook --cov-report=term-missing + +# Test the new check_no_extra_fields function +uv run pytest tests/test_models.py::test_model_has_all_keys_in_json -v + +# Verify no extra fields in webhook fixtures +uv run python -c " +from octohook.events import parse +import json + +# Load a fixture and verify no extra fields +with open('tests/fixtures/complete/pull_request_opened.json') as f: + payload = json.load(f) +event = parse('pull_request', payload) + +# Check __pydantic_extra__ +extra = getattr(event, '__pydantic_extra__', None) +if extra: + print(f'WARNING: Extra fields found: {list(extra.keys())}') +else: + print('✓ No extra fields - all fields defined') +" + +# If mypy added, run type checking +uv run mypy octohook + +# Verify imports work +uv run python -c "from octohook import *; print('All imports OK')" + +# Verify public API unchanged +uv run python -c " +import octohook +expected = [ + 'events', 'handle_webhook', 'hook', 'models', + 'OctohookConfigError', 'parse', 'reset', 'setup', + 'WebhookEvent', 'WebhookEventAction' +] +actual = octohook.__all__ +assert set(expected) == set(actual), f'API changed: {set(actual) - set(expected)}' +print('✓ Public API preserved') +" +``` + +**Expected outcomes:** +- ✅ All tests pass +- ✅ Coverage maintained or improved (~90%+) +- ✅ `__pydantic_extra__` is empty for all webhook fixtures +- ✅ No unused imports remain +- ✅ Public API unchanged +- ✅ mypy reports no errors (if added) +- ✅ Test suite runs faster (less manual validation) + +## Risk Level + +**Low** + +- Test-only changes (no production code impact) +- Cleanup of unused code (reduces maintenance burden) +- Public API preserved +- All functional changes already merged in previous PRs + +## Rationale + +**Why last:** +- All production code changes complete (models, events, parse) +- Tests should validate the final state +- Safe to remove obsolete validation code once migration is complete +- Allows comprehensive verification of entire migration + +**Why together:** +- All test updates are related (validation logic) +- Cleanup is related (remove code made obsolete by migration) +- Logical unit: "verify migration complete and cleanup" + +**Why test changes:** +- Old `check_model()` inspected raw dicts and type hints manually +- New `check_no_extra_fields()` leverages Pydantic's `__pydantic_extra__` +- Simpler, more reliable, faster +- Pydantic's strict mode validates types better than our custom checker + +## Lines Changed (Est) + +~200-300 lines + +**Breakdown:** +- Remove ~400 lines (old validation functions, utility functions, obsolete tests) +- Add ~100 lines (new check_no_extra_fields, updated test docstrings, mypy config) +- Net reduction: ~300 lines removed + +## Rollback Instructions + +**If this PR must be reverted:** + +1. Revert the commit: +```bash +git revert +``` + +2. Verify tests still pass with old validation: +```bash +uv run pytest tests/ -v +``` + +3. **Note:** This PR only affects tests, so reverting it doesn't break production code. Previous PRs (1-5) remain functional. + +4. **Important:** Old validation functions expected old model structure. If this PR is reverted but PRs 2-5 are not, tests will fail (expected - old tests don't work with new models). + +## Additional Notes + +**Why check_no_extra_fields is better:** + +Old approach: +- Manually inspected type hints with `get_type_hints()` +- Parsed `Annotated[dict, "unstructured"]` manually +- Recursively traversed dict and checked against class attributes +- Fragile, hard to maintain, slow + +New approach: +- Pydantic automatically populates `__pydantic_extra__` with undefined fields +- Simple check: is `__pydantic_extra__` empty? +- Recursive checking for nested models +- Reliable, simple, fast + +**Coverage improvements:** + +Expected coverage to remain high or improve because: +- Fewer lines of code (60% reduction) +- Pydantic handles more logic internally +- Tests focus on behavior, not implementation + +**mypy benefits (if added):** + +- Catches type errors at development time +- Validates Pydantic models correctly +- Integrates with IDE for better autocomplete +- Prevents type regressions + +**Post-merge checklist:** +- ✅ `check_no_extra_fields()` function added +- ✅ `test_model_has_all_keys_in_json` updated to use new function +- ✅ Old validation functions removed +- ✅ Old utility functions removed +- ✅ `test_all_type_hints_are_correct` removed +- ✅ `test_unannotated_dict_enforcement` removed +- ✅ Unused imports removed from models.py and events.py +- ✅ Public API verified unchanged +- ✅ All tests pass +- ✅ Coverage maintained +- ✅ mypy added and passes (if included) + +## Migration Complete! + +After this PR merges, the Pydantic V2 migration is **100% complete**! 🎉 + +**Final verification:** +```bash +# All tests pass +uv run pytest -v + +# Coverage good +uv run pytest --cov=octohook --cov-report=term-missing + +# No extra fields +# (check_no_extra_fields validates this) + +# Code reduction achieved +git diff main --stat # Should show ~1800 lines removed + +# Public API preserved +uv run python -c "from octohook import *; print('Migration complete!')" +``` + +**Success criteria met:** +- ✅ All 59 model classes converted +- ✅ All 48 event classes converted +- ✅ Parse function uses model_validate() +- ✅ Tests updated for Pydantic +- ✅ All tests pass +- ✅ Coverage maintained +- ✅ No __init__ methods in models/events +- ✅ No payload fields +- ✅ URL templates work +- ✅ ~60% code reduction +- ✅ Public API preserved diff --git a/prs/pr-overview.md b/prs/pr-overview.md new file mode 100644 index 0000000..b08b87d --- /dev/null +++ b/prs/pr-overview.md @@ -0,0 +1,153 @@ +# Pydantic V2 Migration - Pull Request Overview + +## Summary + +The Pydantic V2 migration will be executed through **6 atomic pull requests**, each leaving the codebase in a working, testable state. The strategy prioritizes foundation-first (base classes and infrastructure) before model/event migrations, ending with test updates. + +**Total scope:** +- ~59 model classes in `octohook/models.py` +- ~48 event classes in `octohook/events.py` +- Test infrastructure updates in `tests/test_models.py` +- Parse function updates +- ~1800 lines of code reduction (from ~3000 to ~1200) + +## PR Sequence + +1. **PR #1: Foundation - Dependencies and Base Classes** (~100-150 lines) + - Add Pydantic dependency, update base classes, fix critical bug + +2. **PR #2: Migrate Model Classes (Part 1 - Simple & Nested)** (~400-500 lines) + - Migrate 30 simpler model classes (no template URLs) + +3. **PR #3: Migrate Model Classes (Part 2 - Template URLs & Complex)** (~400-500 lines) + - Migrate 29 complex model classes with template URLs and special cases + +4. **PR #4: Migrate Event Classes** (~300-400 lines) + - Migrate all 48 event classes in `events.py` + +5. **PR #5: Update Parse Function** (~50 lines) + - Update `parse()` to use `model_validate()`, review decorators + +6. **PR #6: Update Test Infrastructure and Cleanup** (~200-300 lines) + - Update test validation logic, cleanup unused code, final verification + +## Critical Path + +**Sequential dependencies:** +- PR #1 MUST be merged first (all others depend on base classes) +- PR #2 and #3 can be done in parallel OR sequentially (preference: sequential due to model interdependencies) +- PR #4 depends on PR #2 and #3 (events reference models) +- PR #5 depends on PR #4 (parse function instantiates events) +- PR #6 can proceed once all code changes are complete + +**Minimum viable sequence:** +PR #1 → PR #2 → PR #3 → PR #4 → PR #5 → PR #6 + +**Parallel opportunities:** +- Once PR #1 is merged, PR #2 and #3 *could* proceed in parallel if careful coordination on shared models +- PR #6 test updates can be prepared in parallel with PR #5 + +## Rollback Strategy + +Each PR is independently revertible: + +1. **Identify failing PR** - Check which PR introduced the issue +2. **Revert via git** - `git revert ` or `git revert ` +3. **Handle dependencies** - If reverting an early PR (e.g., PR #1), all subsequent PRs must be reverted in reverse order +4. **Test after revert** - Run full test suite to confirm stability + +**Critical rollback considerations:** +- PR #1 revert requires reverting all subsequent PRs (foundation change) +- PR #2-4 reverts are localized to their respective files +- PR #5 revert is simple (single function change) +- PR #6 revert only affects test infrastructure + +## Breaking Changes + +### Immutability (`frozen=True`) + +**Impact:** Models and events are frozen after creation. Any code attempting to modify attributes will raise `ValidationError`. + +**Example:** +```python +# Before: This worked +user = User(payload) +user.login = "newname" # Allowed + +# After: This fails +user = User.model_validate(payload) +user.login = "newname" # ValidationError: Instance is frozen +``` + +**Mitigation:** User code should not modify models post-creation. If modification is needed, use `model_copy()` or create new instances. + +### Strict Typing (`strict=True`) + +**Impact:** No automatic type coercion. GitHub API must send correct types. + +**Example:** +```python +# Before: This worked (coercion allowed) +{"id": "123"} # String coerced to int + +# After: This fails (strict validation) +{"id": "123"} # ValidationError: Expected int, got str +``` + +**Mitigation:** This catches API inconsistencies early. If GitHub changes types, we'll detect it immediately rather than silently coercing. + +### Public API Preservation + +**Good news:** All existing methods and attributes remain accessible. The breaking changes only affect: +1. Post-creation modification attempts +2. Type validation strictness + +Existing code that reads fields and calls methods will continue to work without changes. + +## Risk Assessment + +| PR | Risk Level | Rationale | +|----|-----------|-----------| +| #1 | **Medium** | Foundation changes affect everything, but small scope | +| #2 | **Low** | Simple models, well-tested patterns | +| #3 | **Medium** | Template URLs and special cases need careful handling | +| #4 | **Low** | Straightforward pattern, depends on stable models | +| #5 | **Low** | Single function change, well-isolated | +| #6 | **Low** | Test-only changes, no production impact | + +## Success Metrics + +Migration is complete when: + +- ✅ All 6 PRs merged to main +- ✅ All tests pass: `uv run pytest -v` +- ✅ Coverage maintained or improved +- ✅ No `__init__` methods in models/events +- ✅ No `payload` fields in models/events +- ✅ URL template methods work correctly +- ✅ `__pydantic_extra__` checks pass (no undefined fields) +- ✅ Code reduced by ~60% (LOC comparison) + +## Timeline Estimate + +Assuming 1 PR per day with review: +- **Best case:** 6 days (sequential, no issues) +- **Realistic:** 8-10 days (reviews, minor fixes) +- **Worst case:** 12-15 days (significant review feedback, rewrites) + +## Communication Plan + +**Before starting:** +- Share this overview with team +- Confirm breaking change implications with stakeholders +- Set up review assignments for each PR + +**During migration:** +- Daily updates on PR status +- Flag any unexpected issues immediately +- Update this document if strategy changes + +**After completion:** +- Document any deviations from plan +- Update CHANGELOG with breaking changes +- Create migration guide for users if needed