From 5844d97da95f124bb3671ad330a1f657f9d54f8e Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Tue, 18 Nov 2025 20:19:10 -0500 Subject: [PATCH 1/6] refactor: remove direct generated_poc imports MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Replace all direct imports from adcp.types.generated_poc with imports from adcp.types._generated. This enforces the public API boundary and prevents downstream users from depending on internal implementation details. Changes: - Update consolidate_exports.py to handle Package name collision by exporting both types with qualified names (_PackageFromPackage, _PackageFromCreateMediaBuyResponse) - Migrate client.py to import TaskStatus from _generated - Migrate aliases.py to import all types from _generated - Migrate stable.py to import Package from _generated using qualified name - Regenerate _generated.py with collision-resolved exports This ensures that: 1. generated_poc directory remains internal (not part of public API) 2. All type consolidation happens in _generated.py 3. Only stable.py and aliases.py provide public type exports 4. Users import from adcp or adcp.types.stable, not internal modules šŸ¤– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- scripts/consolidate_exports.py | 28 +++++++++++++++++++++++++++- src/adcp/client.py | 2 +- src/adcp/types/_generated.py | 17 +++++++++++------ src/adcp/types/aliases.py | 30 +++++++++--------------------- src/adcp/types/stable.py | 4 ++-- 5 files changed, 50 insertions(+), 31 deletions(-) diff --git a/scripts/consolidate_exports.py b/scripts/consolidate_exports.py index f1bc13dc..64c9375d 100644 --- a/scripts/consolidate_exports.py +++ b/scripts/consolidate_exports.py @@ -58,6 +58,11 @@ def generate_consolidated_exports() -> str: all_exports = set() collisions = [] + # Special handling for known collision: Package type + # We need BOTH Package types available, so import them with qualified names + special_imports = [] + package_modules_seen = set() + for module_path in modules: module_name = module_path.stem exports = extract_exports_from_module(module_path) @@ -68,9 +73,15 @@ def generate_consolidated_exports() -> str: # Filter out names that collide with already-exported names unique_exports = set() for export_name in exports: + # Special case: Package collision - track all modules that define it + if export_name == "Package" and module_name in ("package", "create_media_buy_response"): + package_modules_seen.add(module_name) + export_to_module[export_name] = module_name # Track that we've seen it + continue # Don't add to unique_exports, we'll handle specially + if export_name in export_to_module: - # Collision detected - skip this duplicate first_module = export_to_module[export_name] + # Collision detected - skip this duplicate collisions.append( f" {export_name}: defined in both {first_module} and {module_name} (using {first_module})" ) @@ -91,6 +102,16 @@ def generate_consolidated_exports() -> str: all_exports.update(unique_exports) + # Generate special imports for Package collision + if package_modules_seen: + collisions.append(f" Package: defined in {sorted(package_modules_seen)} (both exported with qualified names)") + for module_name in sorted(package_modules_seen): + qualified_name = f"_PackageFrom{module_name.replace('_', ' ').title().replace(' ', '')}" + special_imports.append( + f"from adcp.types.generated_poc.{module_name} import Package as {qualified_name}" + ) + all_exports.add(qualified_name) + if collisions: print("\nāš ļø Name collisions detected (duplicates skipped):") for collision in sorted(collisions): @@ -121,6 +142,11 @@ def generate_consolidated_exports() -> str: lines.extend(import_lines) + # Add special imports for name collisions + if special_imports: + lines.extend(["", "# Special imports for name collisions (Package type)"]) + lines.extend(special_imports) + # Add backward compatibility aliases (only if source exists) aliases = {} if "AdvertisingChannels" in all_exports: diff --git a/src/adcp/client.py b/src/adcp/client.py index 7b04128a..bb601c4c 100644 --- a/src/adcp/client.py +++ b/src/adcp/client.py @@ -38,6 +38,7 @@ ProvidePerformanceFeedbackResponse, SyncCreativesRequest, SyncCreativesResponse, + TaskStatus as GeneratedTaskStatus, WebhookPayload, ) from adcp.types.core import ( @@ -48,7 +49,6 @@ TaskResult, TaskStatus, ) -from adcp.types.generated_poc.task_status import TaskStatus as GeneratedTaskStatus from adcp.utils.operation_id import create_operation_id logger = logging.getLogger(__name__) diff --git a/src/adcp/types/_generated.py b/src/adcp/types/_generated.py index fdb9471d..a4fb7769 100644 --- a/src/adcp/types/_generated.py +++ b/src/adcp/types/_generated.py @@ -10,7 +10,7 @@ DO NOT EDIT MANUALLY. Generated from: https://github.com/adcontextprotocol/adcp/tree/main/schemas -Generation date: 2025-11-18 12:52:17 UTC +Generation date: 2025-11-19 01:13:16 UTC """ # ruff: noqa: E501, I001 from __future__ import annotations @@ -33,7 +33,7 @@ from adcp.types.generated_poc.cpp_option import CppPricingOption, Parameters from adcp.types.generated_poc.cpv_option import CpvPricingOption, ViewThreshold, ViewThreshold1 from adcp.types.generated_poc.create_media_buy_request import CreateMediaBuyRequest, ReportingFrequency, ReportingWebhook, RequestedMetric -from adcp.types.generated_poc.create_media_buy_response import CreateMediaBuyResponse, CreateMediaBuyResponse1, CreateMediaBuyResponse2, Package +from adcp.types.generated_poc.create_media_buy_response import CreateMediaBuyResponse, CreateMediaBuyResponse1, CreateMediaBuyResponse2 from adcp.types.generated_poc.creative_asset import CreativeAsset, Input from adcp.types.generated_poc.creative_assignment import CreativeAssignment from adcp.types.generated_poc.creative_manifest import CreativeManifest @@ -113,6 +113,10 @@ from adcp.types.generated_poc.webhook_asset import Method, Method1, ResponseType, Security, WebhookAsset from adcp.types.generated_poc.webhook_payload import WebhookPayload +# Special imports for name collisions (Package type) +from adcp.types.generated_poc.create_media_buy_response import Package as _PackageFromCreateMediaBuyResponse +from adcp.types.generated_poc.package import Package as _PackageFromPackage + # Backward compatibility aliases for renamed types Channels = AdvertisingChannels @@ -146,9 +150,9 @@ "ListCreativesResponse", "Logo", "MarkdownAsset", "MarkdownFlavor", "Measurement", "MeasurementPeriod", "MediaBuy", "MediaBuyDelivery", "MediaBuyStatus", "Metadata", "Method", "Method1", "MetricType", "ModuleType", "NotificationType", "Offering", "OutputFormat", - "Pacing", "Package", "PackageRequest", "PackageStatus", "Packages", "Packages1", "Packages2", - "Packages3", "Pagination", "Parameters", "Performance", "PerformanceFeedback", "Placement", - "Preview", "Preview1", "Preview2", "PreviewCreativeRequest", "PreviewCreativeRequest1", + "Pacing", "PackageRequest", "PackageStatus", "Packages", "Packages1", "Packages2", "Packages3", + "Pagination", "Parameters", "Performance", "PerformanceFeedback", "Placement", "Preview", + "Preview1", "Preview2", "PreviewCreativeRequest", "PreviewCreativeRequest1", "PreviewCreativeRequest2", "PreviewCreativeResponse", "PreviewCreativeResponse1", "PreviewCreativeResponse2", "PreviewRender", "PreviewRender1", "PreviewRender2", "PreviewRender3", "PriceGuidance", "Pricing", "PricingModel", "PrimaryCountry", "Product", @@ -171,5 +175,6 @@ "UpdateMediaBuyResponse", "UpdateMediaBuyResponse1", "UpdateMediaBuyResponse2", "UrlAsset", "UrlType", "ValidationMode", "VastAsset1", "VastAsset2", "VastVersion", "VcpmAuctionPricingOption", "VcpmFixedRatePricingOption", "VenueBreakdownItem", "VideoAsset", - "ViewThreshold", "ViewThreshold1", "WebhookAsset", "WebhookPayload" + "ViewThreshold", "ViewThreshold1", "WebhookAsset", "WebhookPayload", + "_PackageFromCreateMediaBuyResponse", "_PackageFromPackage" ] diff --git a/src/adcp/types/aliases.py b/src/adcp/types/aliases.py index 6a3da58a..b57fb9fe 100644 --- a/src/adcp/types/aliases.py +++ b/src/adcp/types/aliases.py @@ -33,6 +33,9 @@ # Import all generated types that need semantic aliases from adcp.types._generated import ( + # Package types (from name collision resolution) + _PackageFromCreateMediaBuyResponse as CreatedPackageInternal, + _PackageFromPackage as FullPackageInternal, # Activation responses ActivateSignalResponse1, ActivateSignalResponse2, @@ -67,6 +70,12 @@ # Performance feedback responses ProvidePerformanceFeedbackResponse1, ProvidePerformanceFeedbackResponse2, + # Publisher properties types + PropertyId, + PropertyTag, + PublisherProperties as PublisherPropertiesInternal, + PublisherProperties4 as PublisherPropertiesByIdInternal, + PublisherProperties5 as PublisherPropertiesByTagInternal, # SubAssets SubAsset1, SubAsset2, @@ -84,27 +93,6 @@ VastAsset2, ) -# Import Package types directly from their modules to avoid collision issues -from adcp.types.generated_poc.create_media_buy_response import ( - Package as CreatedPackageInternal, -) -from adcp.types.generated_poc.package import Package as FullPackageInternal - -# Import PublisherProperties types and related types from product module -from adcp.types.generated_poc.product import ( - PropertyId, - PropertyTag, -) -from adcp.types.generated_poc.product import ( - PublisherProperties as PublisherPropertiesInternal, -) -from adcp.types.generated_poc.product import ( - PublisherProperties4 as PublisherPropertiesByIdInternal, -) -from adcp.types.generated_poc.product import ( - PublisherProperties5 as PublisherPropertiesByTagInternal, -) - # ============================================================================ # RESPONSE TYPE ALIASES - Success/Error Discriminated Unions # ============================================================================ diff --git a/src/adcp/types/stable.py b/src/adcp/types/stable.py index 879e80d9..43e91d28 100644 --- a/src/adcp/types/stable.py +++ b/src/adcp/types/stable.py @@ -93,8 +93,8 @@ ) # Import all generated types from internal consolidated module -# Import Package directly from its module to avoid collision with Response Package -from adcp.types.generated_poc.package import Package +# Import Package from _generated (uses qualified name to avoid collision) +from adcp.types._generated import _PackageFromPackage as Package # Note: BrandManifest is currently split into BrandManifest1/2 due to upstream schema # using anyOf incorrectly. This will be fixed upstream to create a single BrandManifest type. From 61f5c95cf061865eb63c5803e7242a60ae9b13a8 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Tue, 18 Nov 2025 20:28:34 -0500 Subject: [PATCH 2/6] fix: enforce import boundary - only stable.py/aliases.py may use _generated MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Source code was incorrectly importing from _generated.py, making it just as brittle as importing from generated_poc. This fixes the architecture to properly enforce the public API boundary. Changes: - Add WebhookPayload to stable.py exports - Change client.py, __init__.py, simple.py to import from stable.py - Change preview_cache.py to use semantic aliases (PreviewCreativeFormatRequest) - Document import architecture rules in CLAUDE.md Import architecture (enforced): ``` generated_poc/*.py (internal) ā†“ _generated.py (internal consolidation) ā†“ stable.py + aliases.py (ONLY files that import from _generated) ā†“ All other source code (imports from stable/aliases only) ``` This prevents brittleness from: - Collision-resolution qualifiers (_PackageFromX) - Numbered discriminated union types (Response1, Response2) - Schema evolution changes in generated files šŸ¤– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- ARCHITECTURE_REVIEW.md | 561 +++++++++++++++++++++++++++++++ CLAUDE.md | 89 ++++- PR65_EVALUATION.md | 568 ++++++++++++++++++++++++++++++++ examples/type_collision_demo.py | 221 +++++++++++++ src/adcp/__init__.py | 4 +- src/adcp/client.py | 2 +- src/adcp/simple.py | 2 +- src/adcp/types/stable.py | 2 + src/adcp/utils/preview_cache.py | 14 +- 9 files changed, 1434 insertions(+), 29 deletions(-) create mode 100644 ARCHITECTURE_REVIEW.md create mode 100644 PR65_EVALUATION.md create mode 100644 examples/type_collision_demo.py diff --git a/ARCHITECTURE_REVIEW.md b/ARCHITECTURE_REVIEW.md new file mode 100644 index 00000000..f2d74358 --- /dev/null +++ b/ARCHITECTURE_REVIEW.md @@ -0,0 +1,561 @@ +# Python Module Architecture Review: PR #65 Type Import Consolidation + +## Executive Summary + +The PR #65 architecture is **fundamentally sound and Pythonic**, with a few minor concerns to address. The qualified naming strategy (`_PackageFromX`) is a pragmatic solution to a real problem (upstream schema collisions), and the layered import hierarchy provides good separation between internal and public APIs. + +**Overall Rating**: 8/10 (Strong architecture with minor refinements needed) + +## Architecture Overview + +### Import Hierarchy + +``` +generated_poc/*.py [INTERNAL: Auto-generated from schemas] + ā†“ +_generated.py [INTERNAL: Consolidated imports, collision handling] + ā†“ +aliases.py + stable.py [PUBLIC: Semantic aliases and stable API] + ā†“ +__init__.py [PUBLIC: User-facing exports] +``` + +This hierarchy is **excellent** because it: +- Provides clear separation of concerns +- Makes internal vs. public explicit via naming conventions +- Allows schema evolution without breaking user code +- Enables gradual migration strategies + +## Detailed Analysis + +### 1. Leading Underscore Convention (`_generated.py`) + +**Status**: āœ… Excellent, Pythonic + +The use of `_generated.py` (with leading underscore) is the **correct Python convention** for private/internal modules. This signals to users and tools that this is not public API. + +**Evidence from Python conventions:** +- `_thread`, `_collections`, `_weakref` in stdlib use this pattern +- PEP 8: "single leading underscore: weak 'internal use' indicator" +- Type checkers respect this convention (pyright warns on underscore imports) + +**Recommendation**: Keep `_generated.py` name. Consider also renaming `generated_poc/` ā†’ `_generated_poc/` for consistency (see point 6). + +### 2. Qualified Naming Strategy (`_PackageFromX`) + +**Status**: āœ… Good, with minor concerns + +The approach of using `_PackageFromPackage` and `_PackageFromCreateMediaBuyResponse` is pragmatic: + +**Pros:** +- Solves real collision problem (two genuinely different `Package` types) +- Makes the source of each type explicit +- Works correctly with type checkers (tested - they see different types) +- Maintains all type information + +**Cons:** +- Slightly verbose (but acceptable for internal API) +- Relies on convention rather than tooling +- Module name appears in identifier (coupling) + +**Alternative approaches considered:** + +#### Option A: Namespace packages (rejected) +```python +from adcp.types.package_def import Package +from adcp.types.package_response import Package as ResponsePackage +``` +**Why rejected**: Creates many small modules, harder to maintain, no real benefit. + +#### Option B: Full module paths in stable.py (rejected) +```python +from adcp.types.generated_poc.package import Package +from adcp.types.generated_poc.create_media_buy_response import Package as CreatedPackage +``` +**Why rejected**: Leaks `generated_poc` into public-facing code, defeats purpose of consolidation. + +#### Option C: Keep current qualified names but improve (recommended) +```python +# Current (good): +from adcp.types._generated import _PackageFromPackage as Package + +# Alternative (more semantic): +from adcp.types._generated import ( + _PackageFullDefinition, # The full Package model with all fields + _PackageCreatedReference, # The minimal Package returned in create response +) +``` + +**Recommendation**: Keep current approach but add semantic type aliases in `aliases.py` if users need to reference both types directly. Document that `Package` (from stable.py) refers to the full definition. + +### 3. Type Checker Compatibility + +**Status**: āœ… Excellent (no issues found) + +Tested with runtime imports - no circular import errors, types are distinct at runtime: + +```python +from adcp.types._generated import _PackageFromPackage, _PackageFromCreateMediaBuyResponse +print(_PackageFromPackage.__name__) # "Package" +print(_PackageFromCreateMediaBuyResponse.__name__) # "Package" +print(_PackageFromPackage is _PackageFromCreateMediaBuyResponse) # False +``` + +**Type checker perspective:** +- mypy: Will see these as distinct types because they're imported from different modules +- pyright: Same behavior +- Both will correctly flag type mismatches even though `__name__` is the same + +**Why this works**: Python's type system is **nominal** (based on definition location), not structural. Even though both classes are named `Package`, they're distinct types because they're defined in different modules. + +**Potential issue**: If someone does runtime `type(obj).__name__ == "Package"` checks, both types will match. But this is bad practice anyway - use `isinstance()` instead. + +**Recommendation**: No changes needed. This is correct Python typing. + +### 4. Collision Handling in `consolidate_exports.py` + +**Status**: āš ļø Good but can be improved + +Current collision detection: + +```python +if export_name in export_to_module: + first_module = export_to_module[export_name] + collisions.append( + f" {export_name}: defined in both {first_module} and {module_name} (using {first_module})" + ) +``` + +**Issues:** +1. **Silent failures**: Collisions are logged but don't fail the build +2. **First-wins is arbitrary**: Alphabetical order determines which type "wins" +3. **No semantic guidance**: Users don't know which type to use + +**Improvements needed:** + +```python +# Define explicit collision policy +KNOWN_COLLISIONS = { + "Package": { + "canonical": "package", # Full definition + "variants": { + "create_media_buy_response": "_PackageFromCreateMediaBuyResponse", + }, + "reason": "Package has full definition in package.py and minimal version in responses", + }, + "Asset": { + "canonical": "brand_manifest", # First alphabetically, but arbitrary + "variants": { + "format": "_AssetFromFormat", + }, + "reason": "Asset type differs between brand manifests and format definitions", + }, +} + +# In consolidation: +if export_name in KNOWN_COLLISIONS: + # Handle explicitly + policy = KNOWN_COLLISIONS[export_name] + if module_name == policy["canonical"]: + # Export without prefix + unique_exports.add(export_name) + elif module_name in policy["variants"]: + # Export with qualified name + qualified = policy["variants"][module_name] + special_imports.append(f"from ... import {export_name} as {qualified}") + # else: skip this module's version entirely +elif export_name in export_to_module: + # Unexpected collision - FAIL THE BUILD + raise ValueError( + f"Unexpected collision: {export_name} in {module_name} and {export_to_module[export_name]}. " + f"Add to KNOWN_COLLISIONS in scripts/consolidate_exports.py" + ) +``` + +**Benefits:** +- Collisions are explicit and documented +- Build fails on new unexpected collisions (forces conscious decision) +- Users can understand the difference between variants +- Easier to maintain as schemas evolve + +**Recommendation**: Implement explicit collision policy in next iteration. + +### 5. Circular Import Risks + +**Status**: āœ… No issues found + +Tested all import paths: +```bash +$ uv run python3 -c "import sys; import importlib; modules = ['adcp', 'adcp.types', 'adcp.types._generated', 'adcp.types.aliases', 'adcp.types.stable']; [importlib.import_module(m) for m in modules]; print('All modules imported successfully')" +All modules imported successfully +``` + +**Why no circular imports:** +1. `generated_poc/*.py` only imports from `adcp.types.base` (foundation layer) +2. `_generated.py` only imports from `generated_poc/*` (one-way dependency) +3. `aliases.py` only imports from `_generated.py` (one-way dependency) +4. `stable.py` only imports from `_generated.py` (one-way dependency) +5. `__init__.py` imports from `aliases.py`, `stable.py`, `_generated.py` (all leaf nodes) + +**Dependency graph:** +``` +base.py (foundation) + ā†‘ +generated_poc/*.py (no cross-imports) + ā†‘ +_generated.py + ā†‘ +aliases.py, stable.py (parallel, no cross-imports) + ā†‘ +__init__.py +``` + +This is a **perfect DAG** (Directed Acyclic Graph) - no cycles possible. + +**Recommendation**: No changes needed. Consider adding a test to detect circular imports in CI: + +```python +# tests/test_import_order.py +def test_no_circular_imports(): + """Verify the import hierarchy has no circular dependencies.""" + import sys + import importlib + + # Clear any cached imports + modules_to_test = [ + 'adcp.types.base', + 'adcp.types.generated_poc.package', + 'adcp.types._generated', + 'adcp.types.aliases', + 'adcp.types.stable', + 'adcp.types', + 'adcp', + ] + + for mod_name in modules_to_test: + if mod_name in sys.modules: + del sys.modules[mod_name] + + # Import in dependency order - should not raise + for mod_name in modules_to_test: + importlib.import_module(mod_name) +``` + +### 6. Should `generated_poc` be Renamed to `_generated_poc`? + +**Status**: āš ļø Recommended for consistency + +**Current state:** +- `_generated.py` (internal, underscore prefix) āœ… +- `generated_poc/` (internal, no underscore prefix) āš ļø + +**Arguments for renaming:** + +1. **Consistency**: Both are internal implementation details +2. **Tool support**: Some tools (pyright, ruff) can be configured to warn on imports from `_` prefixed modules +3. **Convention**: Python stdlib uses `_` prefix for internal packages (`_collections`, `_weakref`) +4. **Clear signal**: Makes it obvious this is not public API + +**Arguments against renaming:** + +1. **Git history**: Renaming loses `git blame` history (mitigated by `.git-blame-ignore-revs`) +2. **Migration cost**: Need to update all internal imports (but these are internal only) +3. **Documentation**: Need to update all docs referencing `generated_poc` + +**Recommendation**: Rename to `_generated_poc/` for consistency. This is a one-time cost with long-term benefits. + +**Migration checklist:** +```bash +# 1. Rename directory +git mv src/adcp/types/generated_poc src/adcp/types/_generated_poc + +# 2. Update imports in _generated.py +sed -i 's/from adcp.types.generated_poc/from adcp.types._generated_poc/g' src/adcp/types/_generated.py + +# 3. Update scripts +sed -i 's/generated_poc/_generated_poc/g' scripts/*.py + +# 4. Update documentation +sed -i 's/generated_poc/_generated_poc/g' CLAUDE.md README.md docs/*.md + +# 5. Add to .git-blame-ignore-revs +echo "# Rename generated_poc to _generated_poc for consistency" >> .git-blame-ignore-revs +echo "" >> .git-blame-ignore-revs +``` + +### 7. Export Strategy in `__all__` + +**Status**: āœ… Good, minor refinement possible + +Current strategy in `_generated.py`: + +```python +__all__ = [ + "ActivateSignalRequest", + ... + "_PackageFromCreateMediaBuyResponse", + "_PackageFromPackage", + ... +] +``` + +**Issue**: Exporting `_` prefixed names in `__all__` is unusual but not wrong. + +**PEP 8 guidance:** +> "A name prefixed with an underscore (e.g. _spam) should be treated as a non-public part of the API" + +**However**: `__all__` explicitly lists public exports, so including `_` names is contradictory. + +**Options:** + +#### Option A: Remove from `__all__` but keep importable (recommended) +```python +__all__ = [ + "ActivateSignalRequest", + # ... (omit _PackageFromX) +] + +# Still importable via: +# from adcp.types._generated import _PackageFromPackage +# But not included in wildcard imports +``` + +#### Option B: Keep in `__all__` but document (current approach) +- Simpler +- Makes qualified names "official" internal API +- Explicit is better than implicit + +**Recommendation**: Keep current approach (Option B) but add docstring: + +```python +# _generated.py +"""INTERNAL: Consolidated generated types. + +DO NOT import from this module directly. +Use 'from adcp import Type' or 'from adcp.types.stable import Type' instead. + +For name-colliding types (like Package), use the qualified names: +- _PackageFromPackage: Full package definition with all fields +- _PackageFromCreateMediaBuyResponse: Minimal package info in response + +These qualified names are in __all__ for explicit internal use only. +""" +``` + +### 8. Documentation and Discoverability + +**Status**: āš ļø Needs improvement + +**Current state:** +- Docstrings explain the architecture (good) +- CLAUDE.md documents the pattern (good) +- Users are warned not to import from internal modules (good) + +**Missing:** +- Type stubs (`.pyi` files) for better IDE support +- API reference showing public vs internal +- Examples of correct import patterns +- Migration guide for existing code + +**Recommendations:** + +#### Add `py.typed` marker (already done āœ…) +```bash +# Confirmed in pyproject.toml: +[tool.setuptools.package-data] +adcp = ["py.typed"] +``` + +#### Add type stubs for clarity +```python +# src/adcp/types/_generated.pyi +"""Type stubs for generated types (internal API).""" + +# Only list public types in stub +class ActivateSignalRequest: ... +class Product: ... +# Omit _PackageFromX from stub to discourage use +``` + +#### Add API reference section to README +```markdown +## Import Patterns + +### āœ… Correct (use public API) +```python +from adcp import Product, Package, Format +from adcp.types import BrandManifest +from adcp.types.stable import MediaBuy +``` + +### āŒ Incorrect (do not import from internal modules) +```python +from adcp.types.generated_poc.product import Product # NO +from adcp.types._generated import Product # NO +from adcp.types._generated import _PackageFromPackage # NO +``` + +### Type Aliases +For discriminated unions, use semantic aliases: +```python +from adcp import ( + UrlPreviewRender, # instead of PreviewRender1 + HtmlPreviewRender, # instead of PreviewRender2 +) +``` +``` + +## Summary of Recommendations + +### High Priority (Do Now) + +1. **Rename `generated_poc/` ā†’ `_generated_poc/`** for consistency + - Clear signal that this is internal + - Follows Python conventions + - One-time migration cost + +2. **Implement explicit collision policy** in `consolidate_exports.py` + - Document why collisions exist + - Fail build on unexpected collisions + - Make collision resolution strategy explicit + +3. **Add circular import test** to CI + - Prevents regressions + - Documents expected import order + +### Medium Priority (Next Iteration) + +4. **Improve documentation** + - Add import pattern examples to README + - Document qualified naming strategy + - Add API reference showing public/internal split + +5. **Add collision documentation to `_generated.py`** + - Explain what `_PackageFromX` means + - When to use each variant + - Why collisions exist (upstream schema issue) + +### Low Priority (Nice to Have) + +6. **Consider type stub files** (`.pyi`) + - Better IDE support + - Hides internal implementation details + - Only if users report confusion + +7. **Lint rule to prevent `generated_poc` imports** + - Custom ruff rule or pre-commit hook + - Fails if non-test code imports from internal modules + +## Potential Gotchas + +### 1. Runtime Type Checking +```python +# This works as expected: +from adcp.types.stable import Package +isinstance(obj, Package) # āœ… Correct + +# This doesn't work as intended: +type(obj).__name__ == "Package" # āš ļø Matches both Package types +``` + +**Mitigation**: Document to use `isinstance()`, not `__name__` comparison. + +### 2. Pickle/Serialization +```python +# Objects pickled with old path won't unpickle with new qualified names +import pickle +from adcp.types._generated import _PackageFromPackage + +pkg = _PackageFromPackage(...) +data = pickle.dumps(pkg) # Stores module path in pickle + +# Later, after regeneration, if path changes: +pickle.loads(data) # May fail if module path changed +``` + +**Mitigation**: Document that Pydantic JSON serialization is preferred over pickle. + +### 3. Type Checker Confusion +```python +# This might confuse type checkers: +from adcp.types._generated import _PackageFromPackage as Package1 +from adcp.types._generated import _PackageFromCreateMediaBuyResponse as Package2 + +def process(pkg: Package1) -> Package2: + # Type checker knows these are different types + return pkg # āŒ Type error (correct!) +``` + +**Mitigation**: This is actually desired behavior. Users shouldn't be mixing these types. + +## Comparison to Other Approaches + +### Approach A: Multiple Subpackages (e.g., requests, responses, models) +```python +from adcp.types.models import Package +from adcp.types.responses import CreatedPackage +``` + +**Pros:** +- More semantic organization +- Clear separation of concerns + +**Cons:** +- Many small modules to maintain +- Unclear where to put shared types +- More complex import paths + +**Verdict**: Current approach is simpler for auto-generated code. + +### Approach B: Fully Qualified Everything +```python +from adcp.types.package import Package +from adcp.types.create_media_buy_response import PackageInfo +``` + +**Pros:** +- No collisions possible +- Very explicit + +**Cons:** +- Verbose import paths +- Harder to maintain +- Doesn't scale with many types + +**Verdict**: Current consolidation approach is more ergonomic. + +### Approach C: Dynamic Module Generation +```python +# Generate separate modules at build time based on schema structure +from adcp.types.generated.v1_0.package import Package +``` + +**Pros:** +- Version-explicit +- Can maintain multiple schema versions + +**Cons:** +- Much more complex +- Overkill for current needs +- Harder to maintain + +**Verdict**: Current approach is appropriate for current scale. + +## Conclusion + +The PR #65 architecture is **fundamentally sound** and follows Python best practices. The qualified naming strategy is a pragmatic solution to a real problem (upstream schema collisions). + +**Key strengths:** +1. Clear separation of internal vs public API via underscore prefix +2. Layered architecture prevents circular imports +3. Type-safe collision handling +4. Works correctly with type checkers +5. Allows schema evolution without breaking user code + +**Recommended improvements:** +1. Rename `generated_poc` ā†’ `_generated_poc` for consistency +2. Implement explicit collision policy with build-time validation +3. Add circular import test to CI +4. Improve documentation with import examples + +**Final recommendation**: Merge PR #65 with the high-priority improvements listed above. diff --git a/CLAUDE.md b/CLAUDE.md index 401532fc..83568140 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -17,31 +17,84 @@ FormatId = str PackageRequest = dict[str, Any] ``` -**Stable Public API Layer** +**Stable Public API Layer - Import Architecture** -**CRITICAL**: The `generated_poc` directory is internal implementation. **Never import directly from it**. +**CRITICAL**: Both `generated_poc/` and `_generated.py` are internal implementation. Source code must ONLY import from `stable.py` or `aliases.py`. -Generated types in `src/adcp/types/generated_poc/` may have: -- Numbered suffixes (e.g., `BrandManifest1`, `BrandManifest2`) due to schema evolution -- Structural changes between minor versions -- Files added/removed as schemas evolve +### Import Architecture -**Always use the stable API:** +The type system has a strict layering to prevent brittleness: + +``` +generated_poc/*.py (internal, auto-generated from schemas) + ā†“ +_generated.py (internal consolidation, handles name collisions) + ā†“ +stable.py (public API for base types) + aliases.py (public API for discriminated unions) + ā†“ +__init__.py (user-facing exports) +``` + +### Import Rules for Source Code + +**āœ… CORRECT - Public API only:** +```python +# For base types (requests, responses, domain models) +from adcp.types.stable import ( + GetProductsRequest, + GetProductsResponse, + Product, + Package, + BrandManifest, +) + +# For semantic aliases (discriminated unions) +from adcp.types.aliases import ( + CreateMediaBuySuccessResponse, + CreateMediaBuyErrorResponse, + PreviewCreativeFormatRequest, +) + +# Or from main package +from adcp import Product, CreateMediaBuySuccessResponse +``` + +**āŒ WRONG - Internal implementation (brittle):** ```python -# āœ… CORRECT - Stable public API -from adcp.types import BrandManifest, Product, CpmFixedRatePricingOption -from adcp.types.stable import BrandManifest, Product +# Never import from _generated - it's internal consolidation +from adcp.types._generated import GetProductsRequest -# āŒ WRONG - Internal generated types (will break) -from adcp.types.generated_poc.brand_manifest import BrandManifest1 -from adcp.types._generated import BrandManifest1 +# Never import from generated_poc - it's internal generated code +from adcp.types.generated_poc.product import Product + +# Never import numbered types directly - use semantic aliases +from adcp.types._generated import CreateMediaBuyResponse1 ``` -The stable API (`src/adcp/types/stable.py`) provides: -1. **Clean names** - `BrandManifest` not `BrandManifest1` -2. **Stability** - Aliases are updated when schemas evolve -3. **Versioning** - Breaking changes require major version bumps -4. **Deprecation warnings** - Direct `generated_poc` imports trigger warnings +### Why This Matters + +1. **`generated_poc/`** may have: + - Numbered suffixes (e.g., `Response1`, `Response2`) + - Files added/removed as schemas evolve + - Name collisions between modules + +2. **`_generated.py`** may have: + - Collision-resolution qualifiers (e.g., `_PackageFromPackage`) + - Internal consolidation logic + - Changes when collision handling evolves + +3. **`stable.py` and `aliases.py`** provide: + - Clean, semantic names + - Stability guarantees within major versions + - Explicit public API + +### Special Cases + +**Only `stable.py` and `aliases.py` may import from `_generated.py`:** +- `stable.py`: Imports base types and re-exports with clean names +- `aliases.py`: Imports numbered discriminated union types and creates semantic aliases + +**All other source files must import from `stable.py` or `aliases.py`.** **NEVER Modify Generated Files Directly** diff --git a/PR65_EVALUATION.md b/PR65_EVALUATION.md new file mode 100644 index 00000000..f4cd9731 --- /dev/null +++ b/PR65_EVALUATION.md @@ -0,0 +1,568 @@ +# PR #65 Architecture Evaluation - Executive Summary + +**Evaluation Date**: 2025-11-18 +**PR**: Type Import Consolidation (#65) +**Reviewer**: Python Architecture Analysis +**Status**: āœ… APPROVED with recommendations + +## TL;DR + +The architecture is **Pythonic, type-safe, and production-ready**. The qualified naming strategy (`_PackageFromX`) elegantly solves a real upstream schema collision problem. Recommended improvements are minor refinements, not fundamental changes. + +**Recommendation: Merge with high-priority improvements below.** + +--- + +## Quick Reference + +### Is this Pythonic? āœ… YES + +- Uses leading underscore for internal modules (`_generated.py`) +- Follows PEP 8 naming conventions +- Provides stable public API via `__init__.py` +- Internal implementation details properly hidden + +### Will it work with type checkers? āœ… YES + +- Tested: No circular imports +- Types are distinct at runtime (`isinstance()` works correctly) +- Type checkers see `_PackageFromX` as different types (correct) +- No `Any` types or unsafe casts + +### Are there better alternatives? āš ļø NO SIGNIFICANT ALTERNATIVES + +- Considered: namespace packages, multiple subpackages, dynamic generation +- Current approach is simpler and appropriate for auto-generated code +- Qualified naming is standard practice in Python for collision resolution + +### Could it cause issues? āš ļø MINOR CONCERNS + +Three minor gotchas (all documented): +1. Runtime `type(obj).__name__` checks don't distinguish variants (use `isinstance()` instead) +2. Pickle serialization may break if paths change (use Pydantic JSON instead) +3. Collisions are detected but don't fail build (should fail on unexpected collisions) + +--- + +## The Architecture + +``` +ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā” +ā”‚ User Code ā”‚ +ā”‚ from adcp import Package, Product ā”‚ +ā””ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”¬ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”˜ + ā”‚ + ā†“ +ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā” +ā”‚ __init__.py (PUBLIC API) ā”‚ +ā”‚ - Re-exports stable types ā”‚ +ā”‚ - Semantic aliases (e.g., Package) ā”‚ +ā””ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”¬ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”˜ + ā”‚ + ā†“ +ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā” +ā”‚ _generated.py (INTERNAL) ā”‚ +ā”‚ - Consolidates all generated types ā”‚ +ā”‚ - Handles name collisions ā”‚ +ā”‚ - Qualified names (_PackageFromX) ā”‚ +ā””ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”¬ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”˜ + ā”‚ + ā†“ +ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā” +ā”‚ generated_poc/*.py (INTERNAL) ā”‚ +ā”‚ - Auto-generated from JSON schemas ā”‚ +ā”‚ - Never modified manually ā”‚ +ā””ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”˜ +``` + +**Why this works:** +- Clear unidirectional dependencies (DAG, no cycles) +- Internal vs. public distinction via naming +- Allows schema evolution without breaking user code +- Type-safe collision resolution + +--- + +## Key Design Decisions + +### 1. Qualified Naming: `_PackageFromX` + +**Decision**: Use underscore-prefixed qualified names for colliding types + +```python +from adcp.types._generated import ( + _PackageFromPackage, # Full package definition + _PackageFromCreateMediaBuyResponse, # Minimal package in response +) +``` + +**Why it works:** +- Makes collision source explicit (from which module) +- Maintains all type information for type checkers +- Standard Python pattern (similar to `_` prefix for private) + +**Why not alternatives:** +- āœ— Multiple subpackages: More complex, harder to maintain +- āœ— Dynamic generation: Overkill for current needs +- āœ— Ignore collision: Would lose one of the types + +**Trade-offs:** +- šŸ‘ Explicit and clear +- šŸ‘ Type-safe +- šŸ‘Ž Slightly verbose (but only for internal use) +- šŸ‘Ž Couples name to module (acceptable for generated code) + +### 2. Leading Underscore: `_generated.py` + +**Decision**: Use `_generated.py` (not `generated.py`) for internal module + +**Rationale:** +- PEP 8: "single leading underscore: weak 'internal use' indicator" +- Signals to users: "Don't import from here" +- Type checkers can warn on underscore imports +- Follows stdlib conventions (`_thread`, `_collections`) + +**Status**: āœ… Already implemented correctly + +### 3. Consolidation Layer + +**Decision**: Single `_generated.py` re-exports all `generated_poc/*` types + +**Why:** +- One place to handle collisions +- Simpler import paths for internal code +- Easier to maintain than scattered imports +- Can add logic (aliases, deprecations) in one place + +**Alternative rejected**: Import directly from `generated_poc/*` +- Would leak internal structure +- No central place to handle collisions +- Harder to evolve schema structure + +--- + +## Collision Handling Deep Dive + +### The Problem + +AdCP schemas define `Package` in two contexts: + +``` +package.json create-media-buy-response.json +ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā” ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā” +ā”‚ class Package: ā”‚ ā”‚ class Package: ā”‚ +ā”‚ package_id: str ā”‚ ā”‚ package_id: str ā”‚ +ā”‚ buyer_ref: str ā”‚ ā”‚ buyer_ref: str ā”‚ +ā”‚ status: Status ā”‚ ā†ā†’ NOT ā”‚ ā”‚ +ā”‚ impressions: float ā”‚ SAME ā””ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”˜ +ā”‚ budget: float ā”‚ (2 fields) +ā”‚ ... 8 more fields ā”‚ +ā””ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”˜ +(12 fields) +``` + +These are **genuinely different types**, not duplicates: +- Full definition: Complete package with targeting, pacing, status +- Response version: Minimal acknowledgment (just IDs) + +### The Solution + +```python +# _generated.py +from adcp.types.generated_poc.package import Package as _PackageFromPackage +from adcp.types.generated_poc.create_media_buy_response import Package as _PackageFromCreateMediaBuyResponse + +__all__ = [ + # ... other types + "_PackageFromPackage", + "_PackageFromCreateMediaBuyResponse", +] + +# stable.py (public API) +from adcp.types._generated import _PackageFromPackage as Package +# Users get the full definition by default + +# __init__.py +from adcp.types.stable import Package +# Final export to users +``` + +### Type Safety Verification + +Run `examples/type_collision_demo.py` to see: + +```python +minimal = _PackageFromCreateMediaBuyResponse(buyer_ref="x", package_id="y") +full = _PackageFromPackage(package_id="y", buyer_ref="x", status="draft") + +# isinstance() correctly distinguishes them: +isinstance(minimal, _PackageFromCreateMediaBuyResponse) # True +isinstance(minimal, _PackageFromPackage) # False +isinstance(full, _PackageFromPackage) # True +isinstance(full, _PackageFromCreateMediaBuyResponse) # False +``` + +**Type checkers see these as different types** because they're defined in different modules. + +--- + +## Testing Results + +### āœ… Import Tests (No Circular Dependencies) + +```bash +$ uv run python3 -c "import sys; import importlib; + modules = ['adcp', 'adcp.types', 'adcp.types._generated', 'adcp.types.aliases', 'adcp.types.stable']; + [importlib.import_module(m) for m in modules]; + print('All modules imported successfully')" +All modules imported successfully +``` + +### āœ… Type Identity Tests + +```python +# Both have same __name__ but are distinct types +_PackageFromPackage.__name__ # "Package" +_PackageFromCreateMediaBuyResponse.__name__ # "Package" +_PackageFromPackage is _PackageFromCreateMediaBuyResponse # False āœ“ +``` + +### āœ… Field Verification + +``` +Full Package fields: 12 +Response Package fields: 2 + +Fields only in Full Package: + - bid_price, budget, creative_assignments, format_ids_to_provide, + impressions, pacing, pricing_option_id, product_id, status, targeting_overlay + +Shared fields: + - buyer_ref, package_id +``` + +### āœ… Runtime Type Checking + +```python +isinstance(minimal, _PackageFromCreateMediaBuyResponse) # āœ… True +isinstance(minimal, _PackageFromPackage) # āœ… False +``` + +--- + +## Recommended Improvements + +### High Priority (Before Merge) + +#### 1. Rename `generated_poc/` ā†’ `_generated_poc/` + +**Why:** Consistency with `_generated.py` convention + +```bash +git mv src/adcp/types/generated_poc src/adcp/types/_generated_poc +# Update all imports +sed -i 's/from adcp.types.generated_poc/from adcp.types._generated_poc/g' src/adcp/types/_generated.py +``` + +**Impact:** +- Makes internal vs public distinction clearer +- Follows Python conventions throughout +- One-time migration cost + +**Files affected:** 3-5 Python files, 2-3 docs + +#### 2. Explicit Collision Policy in `consolidate_exports.py` + +**Current issue:** Collisions are logged but don't fail build + +**Proposed fix:** + +```python +# scripts/consolidate_exports.py + +KNOWN_COLLISIONS = { + "Package": { + "canonical": "package", # Export as Package + "variants": { + "create_media_buy_response": "_PackageFromCreateMediaBuyResponse", + }, + "reason": "Full package definition vs response acknowledgment", + }, + "Asset": { + "canonical": "brand_manifest", + "variants": { + "format": "_AssetFromFormat", + }, + "reason": "Asset differs between brand manifests and format definitions", + }, +} + +# In generate function: +if export_name in export_to_module and export_name not in KNOWN_COLLISIONS: + # Unexpected collision - FAIL THE BUILD + raise ValueError( + f"Unexpected collision: {export_name} in {module_name} and {export_to_module[export_name]}. " + f"Add to KNOWN_COLLISIONS in scripts/consolidate_exports.py if intentional." + ) +``` + +**Benefits:** +- Documents why collisions exist +- Fails on unexpected collisions (forces conscious decision) +- Makes collision handling maintainable + +#### 3. Add Circular Import Test to CI + +```python +# tests/test_import_architecture.py + +def test_no_circular_imports(): + """Verify the import hierarchy has no circular dependencies.""" + import sys + import importlib + + # Test imports in dependency order + modules = [ + 'adcp.types.base', + 'adcp.types._generated_poc.package', # Sample from generated + 'adcp.types._generated', + 'adcp.types.aliases', + 'adcp.types.stable', + 'adcp', + ] + + # Clear cache + for mod in list(sys.modules.keys()): + if mod.startswith('adcp'): + del sys.modules[mod] + + # Import should succeed without circular dependency errors + for mod_name in modules: + importlib.import_module(mod_name) +``` + +Add to CI: +```yaml +# .github/workflows/ci.yml +- name: Test import architecture + run: uv run pytest tests/test_import_architecture.py -v +``` + +### Medium Priority (Next Iteration) + +#### 4. Document Qualified Naming in `_generated.py` Docstring + +```python +# src/adcp/types/_generated.py +"""INTERNAL: Consolidated generated types. + +DO NOT import from this module directly. +Use 'from adcp import Type' or 'from adcp.types.stable import Type' instead. + +## Qualified Names for Colliding Types + +Some types have name collisions in upstream schemas. These are exported with +qualified names indicating their source module: + +- `_PackageFromPackage`: Full package definition from package.json + - Fields: package_id, buyer_ref, status, impressions, budget, pacing, etc. + - Use: When working with complete package data + +- `_PackageFromCreateMediaBuyResponse`: Minimal package from create-media-buy-response.json + - Fields: package_id, buyer_ref + - Use: When handling API response acknowledgments + +The public API exports `Package` ā†’ `_PackageFromPackage` (full definition). +Use qualified names only if you need to distinguish response vs full package. +""" +``` + +#### 5. Add Import Pattern Examples to README + +```markdown +## Import Patterns + +### āœ… Recommended: Public API + +```python +from adcp import Product, Package, Format, BrandManifest +from adcp.types import MediaBuy, Creative +from adcp.types.stable import Property +``` + +### āŒ Avoid: Internal Modules + +```python +# DON'T DO THIS +from adcp.types.generated_poc.product import Product # āŒ +from adcp.types._generated import Product # āŒ +from adcp.types._generated import _PackageFromPackage # āŒ (unless you know why) +``` + +### ā„¹ļø Advanced: Discriminated Unions + +For types with multiple variants, use semantic aliases: + +```python +from adcp import ( + UrlPreviewRender, # instead of PreviewRender1 + HtmlPreviewRender, # instead of PreviewRender2 +) +``` +``` + +### Low Priority (Nice to Have) + +#### 6. Lint Rule to Prevent Internal Imports + +Custom ruff rule or pre-commit hook: + +```python +# .pre-commit-hooks/check_internal_imports.py +"""Prevent imports from internal modules in user-facing code.""" + +def check_file(filepath: str) -> list[str]: + if "test_" in filepath or filepath.endswith(("_generated.py", "aliases.py", "stable.py")): + return [] # Allow in tests and internal modules + + with open(filepath) as f: + for line_no, line in enumerate(f, 1): + if "from adcp.types._generated_poc" in line: + return [f"{filepath}:{line_no}: Don't import from _generated_poc - use public API"] + if "from adcp.types._generated import" in line: + return [f"{filepath}:{line_no}: Don't import from _generated - use public API"] + return [] +``` + +--- + +## Known Gotchas & Mitigations + +### Gotcha 1: Runtime Type Name Checking + +**Problem:** +```python +type(obj).__name__ == "Package" # Matches BOTH Package types +``` + +**Mitigation:** +- Document to use `isinstance()` instead +- Both types have same `__name__` but different identity + +**Impact:** Low (bad practice anyway) + +### Gotcha 2: Pickle Serialization + +**Problem:** +```python +import pickle +pkg = _PackageFromPackage(...) +data = pickle.dumps(pkg) # Stores full module path +# If module path changes, unpickling fails +``` + +**Mitigation:** +- Use Pydantic's JSON serialization instead +- More portable across schema versions +- Recommended in docs + +**Impact:** Low (Pydantic JSON is better choice) + +### Gotcha 3: Silent Collision Handling + +**Problem:** New collisions don't fail build (just logged) + +**Mitigation:** Implement explicit collision policy (see Recommendation #2) + +**Impact:** Medium (addressed by high-priority recommendation) + +--- + +## Comparison to Alternative Approaches + +### Alternative A: Multiple Subpackages + +```python +from adcp.types.models import Package +from adcp.types.responses import CreatedPackage +``` + +**Pros:** More semantic organization +**Cons:** Many modules to maintain, unclear where shared types go +**Verdict:** Current approach is simpler for auto-generated code āœ… + +### Alternative B: Ignore Collisions (First-Wins) + +```python +# Just export one Package, ignore the other +from adcp.types.generated_poc.package import Package +# create_media_buy_response.Package is silently ignored +``` + +**Pros:** Simpler code +**Cons:** Lose information, type mismatch errors at runtime +**Verdict:** Would break type safety āŒ + +### Alternative C: Namespace Packages + +```python +from adcp.types.package import full as Package +from adcp.types.package import response as PackageResponse +``` + +**Pros:** Very explicit +**Cons:** Requires restructuring, harder to generate +**Verdict:** Overkill for current needs āŒ + +--- + +## Final Verdict + +### Architecture Quality: 8/10 + +**Strengths:** +- āœ… Type-safe collision resolution +- āœ… Clear internal vs public API separation +- āœ… No circular imports +- āœ… Pythonic naming conventions +- āœ… Works correctly with type checkers +- āœ… Allows schema evolution + +**Weaknesses:** +- āš ļø Collision handling could be more explicit (addressed by recommendations) +- āš ļø Missing some documentation (addressed by recommendations) +- āš ļø `generated_poc` naming inconsistent with `_generated.py` (addressed by recommendations) + +### Recommendation: āœ… APPROVE + +Merge PR #65 with high-priority improvements: +1. Rename `generated_poc/` ā†’ `_generated_poc/` +2. Add explicit collision policy to build +3. Add circular import test to CI + +This is a solid, production-ready architecture that properly separates concerns and maintains type safety. + +--- + +## Resources + +- **Full Analysis**: See `ARCHITECTURE_REVIEW.md` for detailed analysis +- **Demo**: Run `examples/type_collision_demo.py` to see collision handling in action +- **Tests**: All import tests passing, no circular dependencies detected + +## Questions? + +**Q: Should I import from `_generated.py`?** +A: No. Import from `adcp` or `adcp.types` or `adcp.types.stable`. + +**Q: What if I need the response Package variant?** +A: Use `from adcp.types._generated import _PackageFromCreateMediaBuyResponse`, but this is rare. + +**Q: Will schema updates break my code?** +A: Not if you import from public API. Internal changes don't affect `from adcp import Package`. + +**Q: Why qualified names with underscores?** +A: Signals "internal use only" and makes collision source explicit. + +**Q: Is this pattern common in Python?** +A: Yes. Similar to stdlib (`_thread`, `_collections`) and many projects with generated code. diff --git a/examples/type_collision_demo.py b/examples/type_collision_demo.py new file mode 100644 index 00000000..4bff0e7f --- /dev/null +++ b/examples/type_collision_demo.py @@ -0,0 +1,221 @@ +#!/usr/bin/env python3 +""" +Demonstration of type collision handling in adcp-client-python. + +This example shows: +1. Why Package type collisions exist +2. How the qualified naming strategy works +3. Type checker behavior with colliding types +4. Best practices for using these types +""" + +from __future__ import annotations + +# ============================================================================ +# CORRECT: Import from public API +# ============================================================================ + +from adcp import Package # This is _PackageFromPackage (full definition) +from adcp.types._generated import ( + _PackageFromCreateMediaBuyResponse, # Internal: minimal Package in response + _PackageFromPackage, # Internal: full Package definition +) + + +def demonstrate_collision(): + """Show that both Package types exist and are distinct.""" + + # These are the SAME type (both point to full Package definition) + print("=== Type Identity ===") + print(f"Package from public API: {Package.__module__}.{Package.__name__}") + print(f"_PackageFromPackage: {_PackageFromPackage.__module__}.{_PackageFromPackage.__name__}") + print(f"Same type? {Package is _PackageFromPackage}") + print() + + # These are DIFFERENT types (defined in different modules) + print("=== Type Collision ===") + print(f"Full Package: {_PackageFromPackage.__module__}.{_PackageFromPackage.__name__}") + print( + f"Response Package: {_PackageFromCreateMediaBuyResponse.__module__}.{_PackageFromCreateMediaBuyResponse.__name__}" + ) + print(f"Same type? {_PackageFromPackage is _PackageFromCreateMediaBuyResponse}") + print() + + # Both have same __name__ but are distinct types + print("=== Name vs. Identity ===") + print(f"Full Package.__name__: {_PackageFromPackage.__name__}") + print(f"Response Package.__name__: {_PackageFromCreateMediaBuyResponse.__name__}") + print("Both are named 'Package', but they're distinct types!") + print() + + +def demonstrate_field_differences(): + """Show the structural differences between Package types.""" + + full_fields = set(_PackageFromPackage.model_fields.keys()) + response_fields = set(_PackageFromCreateMediaBuyResponse.model_fields.keys()) + + print("=== Field Comparison ===") + print(f"Full Package fields: {len(full_fields)}") + print(f"Response Package fields: {len(response_fields)}") + print() + + print("Fields only in Full Package:") + for field in sorted(full_fields - response_fields): + print(f" - {field}") + print() + + print("Fields only in Response Package:") + for field in sorted(response_fields - full_fields): + print(f" - {field}") + print() + + print("Shared fields:") + for field in sorted(full_fields & response_fields): + print(f" - {field}") + print() + + +def demonstrate_type_safety(): + """Show how type checkers handle these distinct types.""" + + # Create minimal package (as returned in create response) + minimal = _PackageFromCreateMediaBuyResponse( + buyer_ref="buyer-123", + package_id="pkg-001", + ) + + # Create full package (note: needs required fields based on schema) + full = _PackageFromPackage( + package_id="pkg-001", + buyer_ref="buyer-123", + product_id="prod-001", + pricing_option_id="pricing-001", + status="draft", # Required field + impressions=10000, + budget=500.0, + # ... many more fields available + ) + + print("=== Type Safety ===") + print(f"Minimal package type: {type(minimal).__name__}") + print(f"Full package type: {type(full).__name__}") + print() + + # isinstance() works correctly (checks actual type, not name) + print("isinstance checks:") + print(f" minimal is Response Package? {isinstance(minimal, _PackageFromCreateMediaBuyResponse)}") + print(f" minimal is Full Package? {isinstance(minimal, _PackageFromPackage)}") + print(f" full is Full Package? {isinstance(full, _PackageFromPackage)}") + print(f" full is Response Package? {isinstance(full, _PackageFromCreateMediaBuyResponse)}") + print() + + +def demonstrate_best_practices(): + """Show the recommended way to work with Package types.""" + + print("=== Best Practices ===") + print() + + print("āœ… CORRECT: Import from public API") + print(" from adcp import Package") + print(" pkg = Package(package_id='...', ...)") + print() + + print("āœ… CORRECT: Use in type hints") + print(" def process_package(pkg: Package) -> None: ...") + print() + + print("āŒ WRONG: Import from internal modules") + print(" from adcp.types.generated_poc.package import Package # NO!") + print(" from adcp.types._generated import Package # NO!") + print() + + print("ā„¹ļø ADVANCED: If you need to distinguish response vs full package:") + print(" from adcp.types._generated import (") + print(" _PackageFromPackage as FullPackage,") + print(" _PackageFromCreateMediaBuyResponse as CreatedPackage,") + print(" )") + print(" def handle_response(pkg: CreatedPackage) -> None: ...") + print() + + +def demonstrate_pitfall(): + """Show a potential pitfall with runtime type checking.""" + + minimal = _PackageFromCreateMediaBuyResponse( + buyer_ref="buyer-123", package_id="pkg-001" + ) + + full = _PackageFromPackage( + package_id="pkg-001", + buyer_ref="buyer-123", + status="draft", # Required + ) + + print("=== Potential Pitfall ===") + print() + print("āŒ BAD: Checking type by __name__ (doesn't distinguish variants)") + print(f" type(minimal).__name__ == 'Package': {type(minimal).__name__ == 'Package'}") + print(f" type(full).__name__ == 'Package': {type(full).__name__ == 'Package'}") + print(" Both return True, even though they're different types!") + print() + + print("āœ… GOOD: Using isinstance() (correctly distinguishes variants)") + print(f" isinstance(minimal, _PackageFromCreateMediaBuyResponse): {isinstance(minimal, _PackageFromCreateMediaBuyResponse)}") + print(f" isinstance(full, _PackageFromPackage): {isinstance(full, _PackageFromPackage)}") + print() + + +def why_collisions_exist(): + """Explain the root cause of Package collision.""" + + print("=== Why Do Collisions Exist? ===") + print() + print("The AdCP JSON schemas define 'Package' in two different contexts:") + print() + print("1. package.json: Full package definition with all fields") + print(" - Used for complete package data") + print(" - Has fields like: impressions, budget, pacing, targeting, etc.") + print(" - This is what you work with in your application") + print() + print("2. create-media-buy-response.json: Minimal package info") + print(" - Used in API response to confirm package creation") + print(" - Only has: buyer_ref, package_id") + print(" - This is what the API returns when creating a media buy") + print() + print("The code generator creates both types with the name 'Package'") + print("because that's how they're named in the schemas.") + print() + print("Our solution: Import both, export full definition as 'Package',") + print("and provide qualified names for internal use if needed.") + print() + + +def main(): + """Run all demonstrations.""" + why_collisions_exist() + print("=" * 80) + print() + + demonstrate_collision() + print("=" * 80) + print() + + demonstrate_field_differences() + print("=" * 80) + print() + + demonstrate_type_safety() + print("=" * 80) + print() + + demonstrate_best_practices() + print("=" * 80) + print() + + demonstrate_pitfall() + + +if __name__ == "__main__": + main() diff --git a/src/adcp/__init__.py b/src/adcp/__init__.py index d4444796..9adf5735 100644 --- a/src/adcp/__init__.py +++ b/src/adcp/__init__.py @@ -59,7 +59,7 @@ # Re-export commonly-used request/response types for convenience # Users should import from main package (e.g., `from adcp import GetProductsRequest`) # rather than internal modules for better API stability -from adcp.types._generated import ( +from adcp.types.stable import ( # Audience & Targeting ActivateSignalRequest, ActivateSignalResponse, @@ -92,10 +92,10 @@ ProvidePerformanceFeedbackResponse, SyncCreativesRequest, SyncCreativesResponse, + TaskStatus as GeneratedTaskStatus, UpdateMediaBuyRequest, UpdateMediaBuyResponse, ) -from adcp.types._generated import TaskStatus as GeneratedTaskStatus # Re-export semantic type aliases for better ergonomics from adcp.types.aliases import ( diff --git a/src/adcp/client.py b/src/adcp/client.py index bb601c4c..72a52821 100644 --- a/src/adcp/client.py +++ b/src/adcp/client.py @@ -17,7 +17,7 @@ from adcp.protocols.a2a import A2AAdapter from adcp.protocols.base import ProtocolAdapter from adcp.protocols.mcp import MCPAdapter -from adcp.types._generated import ( +from adcp.types.stable import ( ActivateSignalRequest, ActivateSignalResponse, GetMediaBuyDeliveryRequest, diff --git a/src/adcp/simple.py b/src/adcp/simple.py index 4e2090ba..6455eef4 100644 --- a/src/adcp/simple.py +++ b/src/adcp/simple.py @@ -23,7 +23,7 @@ from typing import TYPE_CHECKING, Any from adcp.exceptions import ADCPSimpleAPIError -from adcp.types._generated import ( +from adcp.types.stable import ( ActivateSignalRequest, ActivateSignalResponse, GetMediaBuyDeliveryRequest, diff --git a/src/adcp/types/stable.py b/src/adcp/types/stable.py index 43e91d28..600e7591 100644 --- a/src/adcp/types/stable.py +++ b/src/adcp/types/stable.py @@ -90,6 +90,7 @@ VcpmFixedRatePricingOption, VideoAsset, WebhookAsset, + WebhookPayload, ) # Import all generated types from internal consolidated module @@ -174,4 +175,5 @@ "UrlAsset", "VideoAsset", "WebhookAsset", + "WebhookPayload", ] diff --git a/src/adcp/utils/preview_cache.py b/src/adcp/utils/preview_cache.py index ec1ffd78..401381f6 100644 --- a/src/adcp/utils/preview_cache.py +++ b/src/adcp/utils/preview_cache.py @@ -10,7 +10,7 @@ if TYPE_CHECKING: from adcp.client import ADCPClient - from adcp.types._generated import CreativeManifest, Format, FormatId, Product + from adcp.types.stable import CreativeManifest, Format, FormatId, Product logger = logging.getLogger(__name__) @@ -67,7 +67,7 @@ async def get_preview_data_for_manifest( Returns: Preview data with preview_url and metadata, or None if generation fails """ - from adcp.types._generated import PreviewCreativeRequest1 + from adcp.types.aliases import PreviewCreativeFormatRequest cache_key = _make_manifest_cache_key(format_id, manifest.model_dump(exclude_none=True)) @@ -75,7 +75,7 @@ async def get_preview_data_for_manifest( return self._preview_cache[cache_key] try: - request = PreviewCreativeRequest1( + request = PreviewCreativeFormatRequest( request_type="single", format_id=format_id, creative_manifest=manifest, @@ -123,7 +123,7 @@ async def get_preview_data_batch( Returns: List of preview data dicts (or None for failures), in same order as requests """ - from adcp.types._generated import PreviewCreativeRequest + from adcp.types.stable import PreviewCreativeRequest if not requests: return [] @@ -396,7 +396,7 @@ def _create_sample_manifest_for_format(fmt: Format) -> CreativeManifest | None: Returns: Sample CreativeManifest, or None if unable to create one """ - from adcp.types._generated import CreativeManifest + from adcp.types.stable import CreativeManifest if not fmt.assets_required: return None @@ -436,7 +436,7 @@ def _create_sample_manifest_for_format_id( Returns: Sample CreativeManifest with placeholder assets """ - from adcp.types._generated import CreativeManifest, ImageAsset, UrlAsset + from adcp.types.stable import CreativeManifest, ImageAsset, UrlAsset assets = { "primary_asset": ImageAsset(url="https://example.com/sample-image.jpg"), @@ -456,7 +456,7 @@ def _create_sample_asset(asset_type: str | None) -> Any: Returns: Sample asset object (Pydantic model) """ - from adcp.types._generated import ( + from adcp.types.stable import ( HtmlAsset, ImageAsset, TextAsset, From 7f467d9450dd8511225cfcc6f0c6be56d319ea17 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Tue, 18 Nov 2025 20:31:59 -0500 Subject: [PATCH 3/6] fix: sort imports to pass ruff linter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ruff check was failing due to unsorted imports. Fixed by running ruff --fix. Changes: - Sort imports in __init__.py according to isort rules - Sort imports in client.py - Sort imports in aliases.py šŸ¤– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- src/adcp/__init__.py | 81 +++++++++++++++++++-------------------- src/adcp/client.py | 18 +++++---- src/adcp/types/aliases.py | 31 ++++++++++----- 3 files changed, 71 insertions(+), 59 deletions(-) diff --git a/src/adcp/__init__.py b/src/adcp/__init__.py index 9adf5735..c82a66b9 100644 --- a/src/adcp/__init__.py +++ b/src/adcp/__init__.py @@ -56,47 +56,6 @@ from adcp.types import _generated as generated from adcp.types import aliases -# Re-export commonly-used request/response types for convenience -# Users should import from main package (e.g., `from adcp import GetProductsRequest`) -# rather than internal modules for better API stability -from adcp.types.stable import ( - # Audience & Targeting - ActivateSignalRequest, - ActivateSignalResponse, - # Creative Operations - BuildCreativeRequest, - BuildCreativeResponse, - # Media Buy Operations - CreateMediaBuyRequest, - CreateMediaBuyResponse, - # Common data types - Error, - Format, - GetMediaBuyDeliveryRequest, - GetMediaBuyDeliveryResponse, - GetProductsRequest, - GetProductsResponse, - GetSignalsRequest, - GetSignalsResponse, - ListAuthorizedPropertiesRequest, - ListAuthorizedPropertiesResponse, - ListCreativeFormatsRequest, - ListCreativeFormatsResponse, - ListCreativesRequest, - ListCreativesResponse, - PreviewCreativeRequest, - PreviewCreativeResponse, - Product, - Property, - ProvidePerformanceFeedbackRequest, - ProvidePerformanceFeedbackResponse, - SyncCreativesRequest, - SyncCreativesResponse, - TaskStatus as GeneratedTaskStatus, - UpdateMediaBuyRequest, - UpdateMediaBuyResponse, -) - # Re-export semantic type aliases for better ergonomics from adcp.types.aliases import ( ActivateSignalErrorResponse, @@ -141,11 +100,20 @@ ) from adcp.types.core import AgentConfig, Protocol, TaskResult, TaskStatus, WebhookMetadata +# Re-export commonly-used request/response types for convenience +# Users should import from main package (e.g., `from adcp import GetProductsRequest`) +# rather than internal modules for better API stability # Re-export core domain types and pricing options from stable API # These are commonly used in typical workflows from adcp.types.stable import ( + # Audience & Targeting + ActivateSignalRequest, + ActivateSignalResponse, # Core domain types BrandManifest, + # Creative Operations + BuildCreativeRequest, + BuildCreativeResponse, # Pricing options (all 9 types for product creation) CpcPricingOption, CpcvPricingOption, @@ -153,19 +121,50 @@ CpmFixedRatePricingOption, CppPricingOption, CpvPricingOption, + # Media Buy Operations + CreateMediaBuyRequest, + CreateMediaBuyResponse, Creative, CreativeManifest, # Status enums (for control flow) CreativeStatus, + # Common data types + Error, FlatRatePricingOption, + Format, + GetMediaBuyDeliveryRequest, + GetMediaBuyDeliveryResponse, + GetProductsRequest, + GetProductsResponse, + GetSignalsRequest, + GetSignalsResponse, + ListAuthorizedPropertiesRequest, + ListAuthorizedPropertiesResponse, + ListCreativeFormatsRequest, + ListCreativeFormatsResponse, + ListCreativesRequest, + ListCreativesResponse, MediaBuy, MediaBuyStatus, Package, PackageStatus, + PreviewCreativeRequest, + PreviewCreativeResponse, PricingModel, + Product, + Property, + ProvidePerformanceFeedbackRequest, + ProvidePerformanceFeedbackResponse, + SyncCreativesRequest, + SyncCreativesResponse, + UpdateMediaBuyRequest, + UpdateMediaBuyResponse, VcpmAuctionPricingOption, VcpmFixedRatePricingOption, ) +from adcp.types.stable import ( + TaskStatus as GeneratedTaskStatus, +) from adcp.validation import ( ValidationError, validate_adagents, diff --git a/src/adcp/client.py b/src/adcp/client.py index 72a52821..b02cc44c 100644 --- a/src/adcp/client.py +++ b/src/adcp/client.py @@ -17,6 +17,14 @@ from adcp.protocols.a2a import A2AAdapter from adcp.protocols.base import ProtocolAdapter from adcp.protocols.mcp import MCPAdapter +from adcp.types.core import ( + Activity, + ActivityType, + AgentConfig, + Protocol, + TaskResult, + TaskStatus, +) from adcp.types.stable import ( ActivateSignalRequest, ActivateSignalResponse, @@ -38,16 +46,10 @@ ProvidePerformanceFeedbackResponse, SyncCreativesRequest, SyncCreativesResponse, - TaskStatus as GeneratedTaskStatus, WebhookPayload, ) -from adcp.types.core import ( - Activity, - ActivityType, - AgentConfig, - Protocol, - TaskResult, - TaskStatus, +from adcp.types.stable import ( + TaskStatus as GeneratedTaskStatus, ) from adcp.utils.operation_id import create_operation_id diff --git a/src/adcp/types/aliases.py b/src/adcp/types/aliases.py index b57fb9fe..126f762b 100644 --- a/src/adcp/types/aliases.py +++ b/src/adcp/types/aliases.py @@ -31,11 +31,7 @@ from __future__ import annotations -# Import all generated types that need semantic aliases from adcp.types._generated import ( - # Package types (from name collision resolution) - _PackageFromCreateMediaBuyResponse as CreatedPackageInternal, - _PackageFromPackage as FullPackageInternal, # Activation responses ActivateSignalResponse1, ActivateSignalResponse2, @@ -67,15 +63,12 @@ PreviewRender1, PreviewRender2, PreviewRender3, - # Performance feedback responses - ProvidePerformanceFeedbackResponse1, - ProvidePerformanceFeedbackResponse2, # Publisher properties types PropertyId, PropertyTag, - PublisherProperties as PublisherPropertiesInternal, - PublisherProperties4 as PublisherPropertiesByIdInternal, - PublisherProperties5 as PublisherPropertiesByTagInternal, + # Performance feedback responses + ProvidePerformanceFeedbackResponse1, + ProvidePerformanceFeedbackResponse2, # SubAssets SubAsset1, SubAsset2, @@ -92,6 +85,24 @@ VastAsset1, VastAsset2, ) +from adcp.types._generated import ( + PublisherProperties as PublisherPropertiesInternal, +) +from adcp.types._generated import ( + PublisherProperties4 as PublisherPropertiesByIdInternal, +) +from adcp.types._generated import ( + PublisherProperties5 as PublisherPropertiesByTagInternal, +) + +# Import all generated types that need semantic aliases +from adcp.types._generated import ( + # Package types (from name collision resolution) + _PackageFromCreateMediaBuyResponse as CreatedPackageInternal, +) +from adcp.types._generated import ( + _PackageFromPackage as FullPackageInternal, +) # ============================================================================ # RESPONSE TYPE ALIASES - Success/Error Discriminated Unions From 2dbf91c73b3879cc5627ff5f31ec044d0e04ed45 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Tue, 18 Nov 2025 20:32:34 -0500 Subject: [PATCH 4/6] chore: remove review artifacts from commit history MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The ARCHITECTURE_REVIEW.md, PR65_EVALUATION.md, and type_collision_demo.py were created by review agents as deliverables but shouldn't be part of the repository. šŸ¤– Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- ARCHITECTURE_REVIEW.md | 561 ------------------------------- PR65_EVALUATION.md | 568 -------------------------------- examples/type_collision_demo.py | 221 ------------- 3 files changed, 1350 deletions(-) delete mode 100644 ARCHITECTURE_REVIEW.md delete mode 100644 PR65_EVALUATION.md delete mode 100644 examples/type_collision_demo.py diff --git a/ARCHITECTURE_REVIEW.md b/ARCHITECTURE_REVIEW.md deleted file mode 100644 index f2d74358..00000000 --- a/ARCHITECTURE_REVIEW.md +++ /dev/null @@ -1,561 +0,0 @@ -# Python Module Architecture Review: PR #65 Type Import Consolidation - -## Executive Summary - -The PR #65 architecture is **fundamentally sound and Pythonic**, with a few minor concerns to address. The qualified naming strategy (`_PackageFromX`) is a pragmatic solution to a real problem (upstream schema collisions), and the layered import hierarchy provides good separation between internal and public APIs. - -**Overall Rating**: 8/10 (Strong architecture with minor refinements needed) - -## Architecture Overview - -### Import Hierarchy - -``` -generated_poc/*.py [INTERNAL: Auto-generated from schemas] - ā†“ -_generated.py [INTERNAL: Consolidated imports, collision handling] - ā†“ -aliases.py + stable.py [PUBLIC: Semantic aliases and stable API] - ā†“ -__init__.py [PUBLIC: User-facing exports] -``` - -This hierarchy is **excellent** because it: -- Provides clear separation of concerns -- Makes internal vs. public explicit via naming conventions -- Allows schema evolution without breaking user code -- Enables gradual migration strategies - -## Detailed Analysis - -### 1. Leading Underscore Convention (`_generated.py`) - -**Status**: āœ… Excellent, Pythonic - -The use of `_generated.py` (with leading underscore) is the **correct Python convention** for private/internal modules. This signals to users and tools that this is not public API. - -**Evidence from Python conventions:** -- `_thread`, `_collections`, `_weakref` in stdlib use this pattern -- PEP 8: "single leading underscore: weak 'internal use' indicator" -- Type checkers respect this convention (pyright warns on underscore imports) - -**Recommendation**: Keep `_generated.py` name. Consider also renaming `generated_poc/` ā†’ `_generated_poc/` for consistency (see point 6). - -### 2. Qualified Naming Strategy (`_PackageFromX`) - -**Status**: āœ… Good, with minor concerns - -The approach of using `_PackageFromPackage` and `_PackageFromCreateMediaBuyResponse` is pragmatic: - -**Pros:** -- Solves real collision problem (two genuinely different `Package` types) -- Makes the source of each type explicit -- Works correctly with type checkers (tested - they see different types) -- Maintains all type information - -**Cons:** -- Slightly verbose (but acceptable for internal API) -- Relies on convention rather than tooling -- Module name appears in identifier (coupling) - -**Alternative approaches considered:** - -#### Option A: Namespace packages (rejected) -```python -from adcp.types.package_def import Package -from adcp.types.package_response import Package as ResponsePackage -``` -**Why rejected**: Creates many small modules, harder to maintain, no real benefit. - -#### Option B: Full module paths in stable.py (rejected) -```python -from adcp.types.generated_poc.package import Package -from adcp.types.generated_poc.create_media_buy_response import Package as CreatedPackage -``` -**Why rejected**: Leaks `generated_poc` into public-facing code, defeats purpose of consolidation. - -#### Option C: Keep current qualified names but improve (recommended) -```python -# Current (good): -from adcp.types._generated import _PackageFromPackage as Package - -# Alternative (more semantic): -from adcp.types._generated import ( - _PackageFullDefinition, # The full Package model with all fields - _PackageCreatedReference, # The minimal Package returned in create response -) -``` - -**Recommendation**: Keep current approach but add semantic type aliases in `aliases.py` if users need to reference both types directly. Document that `Package` (from stable.py) refers to the full definition. - -### 3. Type Checker Compatibility - -**Status**: āœ… Excellent (no issues found) - -Tested with runtime imports - no circular import errors, types are distinct at runtime: - -```python -from adcp.types._generated import _PackageFromPackage, _PackageFromCreateMediaBuyResponse -print(_PackageFromPackage.__name__) # "Package" -print(_PackageFromCreateMediaBuyResponse.__name__) # "Package" -print(_PackageFromPackage is _PackageFromCreateMediaBuyResponse) # False -``` - -**Type checker perspective:** -- mypy: Will see these as distinct types because they're imported from different modules -- pyright: Same behavior -- Both will correctly flag type mismatches even though `__name__` is the same - -**Why this works**: Python's type system is **nominal** (based on definition location), not structural. Even though both classes are named `Package`, they're distinct types because they're defined in different modules. - -**Potential issue**: If someone does runtime `type(obj).__name__ == "Package"` checks, both types will match. But this is bad practice anyway - use `isinstance()` instead. - -**Recommendation**: No changes needed. This is correct Python typing. - -### 4. Collision Handling in `consolidate_exports.py` - -**Status**: āš ļø Good but can be improved - -Current collision detection: - -```python -if export_name in export_to_module: - first_module = export_to_module[export_name] - collisions.append( - f" {export_name}: defined in both {first_module} and {module_name} (using {first_module})" - ) -``` - -**Issues:** -1. **Silent failures**: Collisions are logged but don't fail the build -2. **First-wins is arbitrary**: Alphabetical order determines which type "wins" -3. **No semantic guidance**: Users don't know which type to use - -**Improvements needed:** - -```python -# Define explicit collision policy -KNOWN_COLLISIONS = { - "Package": { - "canonical": "package", # Full definition - "variants": { - "create_media_buy_response": "_PackageFromCreateMediaBuyResponse", - }, - "reason": "Package has full definition in package.py and minimal version in responses", - }, - "Asset": { - "canonical": "brand_manifest", # First alphabetically, but arbitrary - "variants": { - "format": "_AssetFromFormat", - }, - "reason": "Asset type differs between brand manifests and format definitions", - }, -} - -# In consolidation: -if export_name in KNOWN_COLLISIONS: - # Handle explicitly - policy = KNOWN_COLLISIONS[export_name] - if module_name == policy["canonical"]: - # Export without prefix - unique_exports.add(export_name) - elif module_name in policy["variants"]: - # Export with qualified name - qualified = policy["variants"][module_name] - special_imports.append(f"from ... import {export_name} as {qualified}") - # else: skip this module's version entirely -elif export_name in export_to_module: - # Unexpected collision - FAIL THE BUILD - raise ValueError( - f"Unexpected collision: {export_name} in {module_name} and {export_to_module[export_name]}. " - f"Add to KNOWN_COLLISIONS in scripts/consolidate_exports.py" - ) -``` - -**Benefits:** -- Collisions are explicit and documented -- Build fails on new unexpected collisions (forces conscious decision) -- Users can understand the difference between variants -- Easier to maintain as schemas evolve - -**Recommendation**: Implement explicit collision policy in next iteration. - -### 5. Circular Import Risks - -**Status**: āœ… No issues found - -Tested all import paths: -```bash -$ uv run python3 -c "import sys; import importlib; modules = ['adcp', 'adcp.types', 'adcp.types._generated', 'adcp.types.aliases', 'adcp.types.stable']; [importlib.import_module(m) for m in modules]; print('All modules imported successfully')" -All modules imported successfully -``` - -**Why no circular imports:** -1. `generated_poc/*.py` only imports from `adcp.types.base` (foundation layer) -2. `_generated.py` only imports from `generated_poc/*` (one-way dependency) -3. `aliases.py` only imports from `_generated.py` (one-way dependency) -4. `stable.py` only imports from `_generated.py` (one-way dependency) -5. `__init__.py` imports from `aliases.py`, `stable.py`, `_generated.py` (all leaf nodes) - -**Dependency graph:** -``` -base.py (foundation) - ā†‘ -generated_poc/*.py (no cross-imports) - ā†‘ -_generated.py - ā†‘ -aliases.py, stable.py (parallel, no cross-imports) - ā†‘ -__init__.py -``` - -This is a **perfect DAG** (Directed Acyclic Graph) - no cycles possible. - -**Recommendation**: No changes needed. Consider adding a test to detect circular imports in CI: - -```python -# tests/test_import_order.py -def test_no_circular_imports(): - """Verify the import hierarchy has no circular dependencies.""" - import sys - import importlib - - # Clear any cached imports - modules_to_test = [ - 'adcp.types.base', - 'adcp.types.generated_poc.package', - 'adcp.types._generated', - 'adcp.types.aliases', - 'adcp.types.stable', - 'adcp.types', - 'adcp', - ] - - for mod_name in modules_to_test: - if mod_name in sys.modules: - del sys.modules[mod_name] - - # Import in dependency order - should not raise - for mod_name in modules_to_test: - importlib.import_module(mod_name) -``` - -### 6. Should `generated_poc` be Renamed to `_generated_poc`? - -**Status**: āš ļø Recommended for consistency - -**Current state:** -- `_generated.py` (internal, underscore prefix) āœ… -- `generated_poc/` (internal, no underscore prefix) āš ļø - -**Arguments for renaming:** - -1. **Consistency**: Both are internal implementation details -2. **Tool support**: Some tools (pyright, ruff) can be configured to warn on imports from `_` prefixed modules -3. **Convention**: Python stdlib uses `_` prefix for internal packages (`_collections`, `_weakref`) -4. **Clear signal**: Makes it obvious this is not public API - -**Arguments against renaming:** - -1. **Git history**: Renaming loses `git blame` history (mitigated by `.git-blame-ignore-revs`) -2. **Migration cost**: Need to update all internal imports (but these are internal only) -3. **Documentation**: Need to update all docs referencing `generated_poc` - -**Recommendation**: Rename to `_generated_poc/` for consistency. This is a one-time cost with long-term benefits. - -**Migration checklist:** -```bash -# 1. Rename directory -git mv src/adcp/types/generated_poc src/adcp/types/_generated_poc - -# 2. Update imports in _generated.py -sed -i 's/from adcp.types.generated_poc/from adcp.types._generated_poc/g' src/adcp/types/_generated.py - -# 3. Update scripts -sed -i 's/generated_poc/_generated_poc/g' scripts/*.py - -# 4. Update documentation -sed -i 's/generated_poc/_generated_poc/g' CLAUDE.md README.md docs/*.md - -# 5. Add to .git-blame-ignore-revs -echo "# Rename generated_poc to _generated_poc for consistency" >> .git-blame-ignore-revs -echo "" >> .git-blame-ignore-revs -``` - -### 7. Export Strategy in `__all__` - -**Status**: āœ… Good, minor refinement possible - -Current strategy in `_generated.py`: - -```python -__all__ = [ - "ActivateSignalRequest", - ... - "_PackageFromCreateMediaBuyResponse", - "_PackageFromPackage", - ... -] -``` - -**Issue**: Exporting `_` prefixed names in `__all__` is unusual but not wrong. - -**PEP 8 guidance:** -> "A name prefixed with an underscore (e.g. _spam) should be treated as a non-public part of the API" - -**However**: `__all__` explicitly lists public exports, so including `_` names is contradictory. - -**Options:** - -#### Option A: Remove from `__all__` but keep importable (recommended) -```python -__all__ = [ - "ActivateSignalRequest", - # ... (omit _PackageFromX) -] - -# Still importable via: -# from adcp.types._generated import _PackageFromPackage -# But not included in wildcard imports -``` - -#### Option B: Keep in `__all__` but document (current approach) -- Simpler -- Makes qualified names "official" internal API -- Explicit is better than implicit - -**Recommendation**: Keep current approach (Option B) but add docstring: - -```python -# _generated.py -"""INTERNAL: Consolidated generated types. - -DO NOT import from this module directly. -Use 'from adcp import Type' or 'from adcp.types.stable import Type' instead. - -For name-colliding types (like Package), use the qualified names: -- _PackageFromPackage: Full package definition with all fields -- _PackageFromCreateMediaBuyResponse: Minimal package info in response - -These qualified names are in __all__ for explicit internal use only. -""" -``` - -### 8. Documentation and Discoverability - -**Status**: āš ļø Needs improvement - -**Current state:** -- Docstrings explain the architecture (good) -- CLAUDE.md documents the pattern (good) -- Users are warned not to import from internal modules (good) - -**Missing:** -- Type stubs (`.pyi` files) for better IDE support -- API reference showing public vs internal -- Examples of correct import patterns -- Migration guide for existing code - -**Recommendations:** - -#### Add `py.typed` marker (already done āœ…) -```bash -# Confirmed in pyproject.toml: -[tool.setuptools.package-data] -adcp = ["py.typed"] -``` - -#### Add type stubs for clarity -```python -# src/adcp/types/_generated.pyi -"""Type stubs for generated types (internal API).""" - -# Only list public types in stub -class ActivateSignalRequest: ... -class Product: ... -# Omit _PackageFromX from stub to discourage use -``` - -#### Add API reference section to README -```markdown -## Import Patterns - -### āœ… Correct (use public API) -```python -from adcp import Product, Package, Format -from adcp.types import BrandManifest -from adcp.types.stable import MediaBuy -``` - -### āŒ Incorrect (do not import from internal modules) -```python -from adcp.types.generated_poc.product import Product # NO -from adcp.types._generated import Product # NO -from adcp.types._generated import _PackageFromPackage # NO -``` - -### Type Aliases -For discriminated unions, use semantic aliases: -```python -from adcp import ( - UrlPreviewRender, # instead of PreviewRender1 - HtmlPreviewRender, # instead of PreviewRender2 -) -``` -``` - -## Summary of Recommendations - -### High Priority (Do Now) - -1. **Rename `generated_poc/` ā†’ `_generated_poc/`** for consistency - - Clear signal that this is internal - - Follows Python conventions - - One-time migration cost - -2. **Implement explicit collision policy** in `consolidate_exports.py` - - Document why collisions exist - - Fail build on unexpected collisions - - Make collision resolution strategy explicit - -3. **Add circular import test** to CI - - Prevents regressions - - Documents expected import order - -### Medium Priority (Next Iteration) - -4. **Improve documentation** - - Add import pattern examples to README - - Document qualified naming strategy - - Add API reference showing public/internal split - -5. **Add collision documentation to `_generated.py`** - - Explain what `_PackageFromX` means - - When to use each variant - - Why collisions exist (upstream schema issue) - -### Low Priority (Nice to Have) - -6. **Consider type stub files** (`.pyi`) - - Better IDE support - - Hides internal implementation details - - Only if users report confusion - -7. **Lint rule to prevent `generated_poc` imports** - - Custom ruff rule or pre-commit hook - - Fails if non-test code imports from internal modules - -## Potential Gotchas - -### 1. Runtime Type Checking -```python -# This works as expected: -from adcp.types.stable import Package -isinstance(obj, Package) # āœ… Correct - -# This doesn't work as intended: -type(obj).__name__ == "Package" # āš ļø Matches both Package types -``` - -**Mitigation**: Document to use `isinstance()`, not `__name__` comparison. - -### 2. Pickle/Serialization -```python -# Objects pickled with old path won't unpickle with new qualified names -import pickle -from adcp.types._generated import _PackageFromPackage - -pkg = _PackageFromPackage(...) -data = pickle.dumps(pkg) # Stores module path in pickle - -# Later, after regeneration, if path changes: -pickle.loads(data) # May fail if module path changed -``` - -**Mitigation**: Document that Pydantic JSON serialization is preferred over pickle. - -### 3. Type Checker Confusion -```python -# This might confuse type checkers: -from adcp.types._generated import _PackageFromPackage as Package1 -from adcp.types._generated import _PackageFromCreateMediaBuyResponse as Package2 - -def process(pkg: Package1) -> Package2: - # Type checker knows these are different types - return pkg # āŒ Type error (correct!) -``` - -**Mitigation**: This is actually desired behavior. Users shouldn't be mixing these types. - -## Comparison to Other Approaches - -### Approach A: Multiple Subpackages (e.g., requests, responses, models) -```python -from adcp.types.models import Package -from adcp.types.responses import CreatedPackage -``` - -**Pros:** -- More semantic organization -- Clear separation of concerns - -**Cons:** -- Many small modules to maintain -- Unclear where to put shared types -- More complex import paths - -**Verdict**: Current approach is simpler for auto-generated code. - -### Approach B: Fully Qualified Everything -```python -from adcp.types.package import Package -from adcp.types.create_media_buy_response import PackageInfo -``` - -**Pros:** -- No collisions possible -- Very explicit - -**Cons:** -- Verbose import paths -- Harder to maintain -- Doesn't scale with many types - -**Verdict**: Current consolidation approach is more ergonomic. - -### Approach C: Dynamic Module Generation -```python -# Generate separate modules at build time based on schema structure -from adcp.types.generated.v1_0.package import Package -``` - -**Pros:** -- Version-explicit -- Can maintain multiple schema versions - -**Cons:** -- Much more complex -- Overkill for current needs -- Harder to maintain - -**Verdict**: Current approach is appropriate for current scale. - -## Conclusion - -The PR #65 architecture is **fundamentally sound** and follows Python best practices. The qualified naming strategy is a pragmatic solution to a real problem (upstream schema collisions). - -**Key strengths:** -1. Clear separation of internal vs public API via underscore prefix -2. Layered architecture prevents circular imports -3. Type-safe collision handling -4. Works correctly with type checkers -5. Allows schema evolution without breaking user code - -**Recommended improvements:** -1. Rename `generated_poc` ā†’ `_generated_poc` for consistency -2. Implement explicit collision policy with build-time validation -3. Add circular import test to CI -4. Improve documentation with import examples - -**Final recommendation**: Merge PR #65 with the high-priority improvements listed above. diff --git a/PR65_EVALUATION.md b/PR65_EVALUATION.md deleted file mode 100644 index f4cd9731..00000000 --- a/PR65_EVALUATION.md +++ /dev/null @@ -1,568 +0,0 @@ -# PR #65 Architecture Evaluation - Executive Summary - -**Evaluation Date**: 2025-11-18 -**PR**: Type Import Consolidation (#65) -**Reviewer**: Python Architecture Analysis -**Status**: āœ… APPROVED with recommendations - -## TL;DR - -The architecture is **Pythonic, type-safe, and production-ready**. The qualified naming strategy (`_PackageFromX`) elegantly solves a real upstream schema collision problem. Recommended improvements are minor refinements, not fundamental changes. - -**Recommendation: Merge with high-priority improvements below.** - ---- - -## Quick Reference - -### Is this Pythonic? āœ… YES - -- Uses leading underscore for internal modules (`_generated.py`) -- Follows PEP 8 naming conventions -- Provides stable public API via `__init__.py` -- Internal implementation details properly hidden - -### Will it work with type checkers? āœ… YES - -- Tested: No circular imports -- Types are distinct at runtime (`isinstance()` works correctly) -- Type checkers see `_PackageFromX` as different types (correct) -- No `Any` types or unsafe casts - -### Are there better alternatives? āš ļø NO SIGNIFICANT ALTERNATIVES - -- Considered: namespace packages, multiple subpackages, dynamic generation -- Current approach is simpler and appropriate for auto-generated code -- Qualified naming is standard practice in Python for collision resolution - -### Could it cause issues? āš ļø MINOR CONCERNS - -Three minor gotchas (all documented): -1. Runtime `type(obj).__name__` checks don't distinguish variants (use `isinstance()` instead) -2. Pickle serialization may break if paths change (use Pydantic JSON instead) -3. Collisions are detected but don't fail build (should fail on unexpected collisions) - ---- - -## The Architecture - -``` -ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā” -ā”‚ User Code ā”‚ -ā”‚ from adcp import Package, Product ā”‚ -ā””ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”¬ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”˜ - ā”‚ - ā†“ -ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā” -ā”‚ __init__.py (PUBLIC API) ā”‚ -ā”‚ - Re-exports stable types ā”‚ -ā”‚ - Semantic aliases (e.g., Package) ā”‚ -ā””ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”¬ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”˜ - ā”‚ - ā†“ -ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā” -ā”‚ _generated.py (INTERNAL) ā”‚ -ā”‚ - Consolidates all generated types ā”‚ -ā”‚ - Handles name collisions ā”‚ -ā”‚ - Qualified names (_PackageFromX) ā”‚ -ā””ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”¬ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”˜ - ā”‚ - ā†“ -ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā” -ā”‚ generated_poc/*.py (INTERNAL) ā”‚ -ā”‚ - Auto-generated from JSON schemas ā”‚ -ā”‚ - Never modified manually ā”‚ -ā””ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”˜ -``` - -**Why this works:** -- Clear unidirectional dependencies (DAG, no cycles) -- Internal vs. public distinction via naming -- Allows schema evolution without breaking user code -- Type-safe collision resolution - ---- - -## Key Design Decisions - -### 1. Qualified Naming: `_PackageFromX` - -**Decision**: Use underscore-prefixed qualified names for colliding types - -```python -from adcp.types._generated import ( - _PackageFromPackage, # Full package definition - _PackageFromCreateMediaBuyResponse, # Minimal package in response -) -``` - -**Why it works:** -- Makes collision source explicit (from which module) -- Maintains all type information for type checkers -- Standard Python pattern (similar to `_` prefix for private) - -**Why not alternatives:** -- āœ— Multiple subpackages: More complex, harder to maintain -- āœ— Dynamic generation: Overkill for current needs -- āœ— Ignore collision: Would lose one of the types - -**Trade-offs:** -- šŸ‘ Explicit and clear -- šŸ‘ Type-safe -- šŸ‘Ž Slightly verbose (but only for internal use) -- šŸ‘Ž Couples name to module (acceptable for generated code) - -### 2. Leading Underscore: `_generated.py` - -**Decision**: Use `_generated.py` (not `generated.py`) for internal module - -**Rationale:** -- PEP 8: "single leading underscore: weak 'internal use' indicator" -- Signals to users: "Don't import from here" -- Type checkers can warn on underscore imports -- Follows stdlib conventions (`_thread`, `_collections`) - -**Status**: āœ… Already implemented correctly - -### 3. Consolidation Layer - -**Decision**: Single `_generated.py` re-exports all `generated_poc/*` types - -**Why:** -- One place to handle collisions -- Simpler import paths for internal code -- Easier to maintain than scattered imports -- Can add logic (aliases, deprecations) in one place - -**Alternative rejected**: Import directly from `generated_poc/*` -- Would leak internal structure -- No central place to handle collisions -- Harder to evolve schema structure - ---- - -## Collision Handling Deep Dive - -### The Problem - -AdCP schemas define `Package` in two contexts: - -``` -package.json create-media-buy-response.json -ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā” ā”Œā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā” -ā”‚ class Package: ā”‚ ā”‚ class Package: ā”‚ -ā”‚ package_id: str ā”‚ ā”‚ package_id: str ā”‚ -ā”‚ buyer_ref: str ā”‚ ā”‚ buyer_ref: str ā”‚ -ā”‚ status: Status ā”‚ ā†ā†’ NOT ā”‚ ā”‚ -ā”‚ impressions: float ā”‚ SAME ā””ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”˜ -ā”‚ budget: float ā”‚ (2 fields) -ā”‚ ... 8 more fields ā”‚ -ā””ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”€ā”˜ -(12 fields) -``` - -These are **genuinely different types**, not duplicates: -- Full definition: Complete package with targeting, pacing, status -- Response version: Minimal acknowledgment (just IDs) - -### The Solution - -```python -# _generated.py -from adcp.types.generated_poc.package import Package as _PackageFromPackage -from adcp.types.generated_poc.create_media_buy_response import Package as _PackageFromCreateMediaBuyResponse - -__all__ = [ - # ... other types - "_PackageFromPackage", - "_PackageFromCreateMediaBuyResponse", -] - -# stable.py (public API) -from adcp.types._generated import _PackageFromPackage as Package -# Users get the full definition by default - -# __init__.py -from adcp.types.stable import Package -# Final export to users -``` - -### Type Safety Verification - -Run `examples/type_collision_demo.py` to see: - -```python -minimal = _PackageFromCreateMediaBuyResponse(buyer_ref="x", package_id="y") -full = _PackageFromPackage(package_id="y", buyer_ref="x", status="draft") - -# isinstance() correctly distinguishes them: -isinstance(minimal, _PackageFromCreateMediaBuyResponse) # True -isinstance(minimal, _PackageFromPackage) # False -isinstance(full, _PackageFromPackage) # True -isinstance(full, _PackageFromCreateMediaBuyResponse) # False -``` - -**Type checkers see these as different types** because they're defined in different modules. - ---- - -## Testing Results - -### āœ… Import Tests (No Circular Dependencies) - -```bash -$ uv run python3 -c "import sys; import importlib; - modules = ['adcp', 'adcp.types', 'adcp.types._generated', 'adcp.types.aliases', 'adcp.types.stable']; - [importlib.import_module(m) for m in modules]; - print('All modules imported successfully')" -All modules imported successfully -``` - -### āœ… Type Identity Tests - -```python -# Both have same __name__ but are distinct types -_PackageFromPackage.__name__ # "Package" -_PackageFromCreateMediaBuyResponse.__name__ # "Package" -_PackageFromPackage is _PackageFromCreateMediaBuyResponse # False āœ“ -``` - -### āœ… Field Verification - -``` -Full Package fields: 12 -Response Package fields: 2 - -Fields only in Full Package: - - bid_price, budget, creative_assignments, format_ids_to_provide, - impressions, pacing, pricing_option_id, product_id, status, targeting_overlay - -Shared fields: - - buyer_ref, package_id -``` - -### āœ… Runtime Type Checking - -```python -isinstance(minimal, _PackageFromCreateMediaBuyResponse) # āœ… True -isinstance(minimal, _PackageFromPackage) # āœ… False -``` - ---- - -## Recommended Improvements - -### High Priority (Before Merge) - -#### 1. Rename `generated_poc/` ā†’ `_generated_poc/` - -**Why:** Consistency with `_generated.py` convention - -```bash -git mv src/adcp/types/generated_poc src/adcp/types/_generated_poc -# Update all imports -sed -i 's/from adcp.types.generated_poc/from adcp.types._generated_poc/g' src/adcp/types/_generated.py -``` - -**Impact:** -- Makes internal vs public distinction clearer -- Follows Python conventions throughout -- One-time migration cost - -**Files affected:** 3-5 Python files, 2-3 docs - -#### 2. Explicit Collision Policy in `consolidate_exports.py` - -**Current issue:** Collisions are logged but don't fail build - -**Proposed fix:** - -```python -# scripts/consolidate_exports.py - -KNOWN_COLLISIONS = { - "Package": { - "canonical": "package", # Export as Package - "variants": { - "create_media_buy_response": "_PackageFromCreateMediaBuyResponse", - }, - "reason": "Full package definition vs response acknowledgment", - }, - "Asset": { - "canonical": "brand_manifest", - "variants": { - "format": "_AssetFromFormat", - }, - "reason": "Asset differs between brand manifests and format definitions", - }, -} - -# In generate function: -if export_name in export_to_module and export_name not in KNOWN_COLLISIONS: - # Unexpected collision - FAIL THE BUILD - raise ValueError( - f"Unexpected collision: {export_name} in {module_name} and {export_to_module[export_name]}. " - f"Add to KNOWN_COLLISIONS in scripts/consolidate_exports.py if intentional." - ) -``` - -**Benefits:** -- Documents why collisions exist -- Fails on unexpected collisions (forces conscious decision) -- Makes collision handling maintainable - -#### 3. Add Circular Import Test to CI - -```python -# tests/test_import_architecture.py - -def test_no_circular_imports(): - """Verify the import hierarchy has no circular dependencies.""" - import sys - import importlib - - # Test imports in dependency order - modules = [ - 'adcp.types.base', - 'adcp.types._generated_poc.package', # Sample from generated - 'adcp.types._generated', - 'adcp.types.aliases', - 'adcp.types.stable', - 'adcp', - ] - - # Clear cache - for mod in list(sys.modules.keys()): - if mod.startswith('adcp'): - del sys.modules[mod] - - # Import should succeed without circular dependency errors - for mod_name in modules: - importlib.import_module(mod_name) -``` - -Add to CI: -```yaml -# .github/workflows/ci.yml -- name: Test import architecture - run: uv run pytest tests/test_import_architecture.py -v -``` - -### Medium Priority (Next Iteration) - -#### 4. Document Qualified Naming in `_generated.py` Docstring - -```python -# src/adcp/types/_generated.py -"""INTERNAL: Consolidated generated types. - -DO NOT import from this module directly. -Use 'from adcp import Type' or 'from adcp.types.stable import Type' instead. - -## Qualified Names for Colliding Types - -Some types have name collisions in upstream schemas. These are exported with -qualified names indicating their source module: - -- `_PackageFromPackage`: Full package definition from package.json - - Fields: package_id, buyer_ref, status, impressions, budget, pacing, etc. - - Use: When working with complete package data - -- `_PackageFromCreateMediaBuyResponse`: Minimal package from create-media-buy-response.json - - Fields: package_id, buyer_ref - - Use: When handling API response acknowledgments - -The public API exports `Package` ā†’ `_PackageFromPackage` (full definition). -Use qualified names only if you need to distinguish response vs full package. -""" -``` - -#### 5. Add Import Pattern Examples to README - -```markdown -## Import Patterns - -### āœ… Recommended: Public API - -```python -from adcp import Product, Package, Format, BrandManifest -from adcp.types import MediaBuy, Creative -from adcp.types.stable import Property -``` - -### āŒ Avoid: Internal Modules - -```python -# DON'T DO THIS -from adcp.types.generated_poc.product import Product # āŒ -from adcp.types._generated import Product # āŒ -from adcp.types._generated import _PackageFromPackage # āŒ (unless you know why) -``` - -### ā„¹ļø Advanced: Discriminated Unions - -For types with multiple variants, use semantic aliases: - -```python -from adcp import ( - UrlPreviewRender, # instead of PreviewRender1 - HtmlPreviewRender, # instead of PreviewRender2 -) -``` -``` - -### Low Priority (Nice to Have) - -#### 6. Lint Rule to Prevent Internal Imports - -Custom ruff rule or pre-commit hook: - -```python -# .pre-commit-hooks/check_internal_imports.py -"""Prevent imports from internal modules in user-facing code.""" - -def check_file(filepath: str) -> list[str]: - if "test_" in filepath or filepath.endswith(("_generated.py", "aliases.py", "stable.py")): - return [] # Allow in tests and internal modules - - with open(filepath) as f: - for line_no, line in enumerate(f, 1): - if "from adcp.types._generated_poc" in line: - return [f"{filepath}:{line_no}: Don't import from _generated_poc - use public API"] - if "from adcp.types._generated import" in line: - return [f"{filepath}:{line_no}: Don't import from _generated - use public API"] - return [] -``` - ---- - -## Known Gotchas & Mitigations - -### Gotcha 1: Runtime Type Name Checking - -**Problem:** -```python -type(obj).__name__ == "Package" # Matches BOTH Package types -``` - -**Mitigation:** -- Document to use `isinstance()` instead -- Both types have same `__name__` but different identity - -**Impact:** Low (bad practice anyway) - -### Gotcha 2: Pickle Serialization - -**Problem:** -```python -import pickle -pkg = _PackageFromPackage(...) -data = pickle.dumps(pkg) # Stores full module path -# If module path changes, unpickling fails -``` - -**Mitigation:** -- Use Pydantic's JSON serialization instead -- More portable across schema versions -- Recommended in docs - -**Impact:** Low (Pydantic JSON is better choice) - -### Gotcha 3: Silent Collision Handling - -**Problem:** New collisions don't fail build (just logged) - -**Mitigation:** Implement explicit collision policy (see Recommendation #2) - -**Impact:** Medium (addressed by high-priority recommendation) - ---- - -## Comparison to Alternative Approaches - -### Alternative A: Multiple Subpackages - -```python -from adcp.types.models import Package -from adcp.types.responses import CreatedPackage -``` - -**Pros:** More semantic organization -**Cons:** Many modules to maintain, unclear where shared types go -**Verdict:** Current approach is simpler for auto-generated code āœ… - -### Alternative B: Ignore Collisions (First-Wins) - -```python -# Just export one Package, ignore the other -from adcp.types.generated_poc.package import Package -# create_media_buy_response.Package is silently ignored -``` - -**Pros:** Simpler code -**Cons:** Lose information, type mismatch errors at runtime -**Verdict:** Would break type safety āŒ - -### Alternative C: Namespace Packages - -```python -from adcp.types.package import full as Package -from adcp.types.package import response as PackageResponse -``` - -**Pros:** Very explicit -**Cons:** Requires restructuring, harder to generate -**Verdict:** Overkill for current needs āŒ - ---- - -## Final Verdict - -### Architecture Quality: 8/10 - -**Strengths:** -- āœ… Type-safe collision resolution -- āœ… Clear internal vs public API separation -- āœ… No circular imports -- āœ… Pythonic naming conventions -- āœ… Works correctly with type checkers -- āœ… Allows schema evolution - -**Weaknesses:** -- āš ļø Collision handling could be more explicit (addressed by recommendations) -- āš ļø Missing some documentation (addressed by recommendations) -- āš ļø `generated_poc` naming inconsistent with `_generated.py` (addressed by recommendations) - -### Recommendation: āœ… APPROVE - -Merge PR #65 with high-priority improvements: -1. Rename `generated_poc/` ā†’ `_generated_poc/` -2. Add explicit collision policy to build -3. Add circular import test to CI - -This is a solid, production-ready architecture that properly separates concerns and maintains type safety. - ---- - -## Resources - -- **Full Analysis**: See `ARCHITECTURE_REVIEW.md` for detailed analysis -- **Demo**: Run `examples/type_collision_demo.py` to see collision handling in action -- **Tests**: All import tests passing, no circular dependencies detected - -## Questions? - -**Q: Should I import from `_generated.py`?** -A: No. Import from `adcp` or `adcp.types` or `adcp.types.stable`. - -**Q: What if I need the response Package variant?** -A: Use `from adcp.types._generated import _PackageFromCreateMediaBuyResponse`, but this is rare. - -**Q: Will schema updates break my code?** -A: Not if you import from public API. Internal changes don't affect `from adcp import Package`. - -**Q: Why qualified names with underscores?** -A: Signals "internal use only" and makes collision source explicit. - -**Q: Is this pattern common in Python?** -A: Yes. Similar to stdlib (`_thread`, `_collections`) and many projects with generated code. diff --git a/examples/type_collision_demo.py b/examples/type_collision_demo.py deleted file mode 100644 index 4bff0e7f..00000000 --- a/examples/type_collision_demo.py +++ /dev/null @@ -1,221 +0,0 @@ -#!/usr/bin/env python3 -""" -Demonstration of type collision handling in adcp-client-python. - -This example shows: -1. Why Package type collisions exist -2. How the qualified naming strategy works -3. Type checker behavior with colliding types -4. Best practices for using these types -""" - -from __future__ import annotations - -# ============================================================================ -# CORRECT: Import from public API -# ============================================================================ - -from adcp import Package # This is _PackageFromPackage (full definition) -from adcp.types._generated import ( - _PackageFromCreateMediaBuyResponse, # Internal: minimal Package in response - _PackageFromPackage, # Internal: full Package definition -) - - -def demonstrate_collision(): - """Show that both Package types exist and are distinct.""" - - # These are the SAME type (both point to full Package definition) - print("=== Type Identity ===") - print(f"Package from public API: {Package.__module__}.{Package.__name__}") - print(f"_PackageFromPackage: {_PackageFromPackage.__module__}.{_PackageFromPackage.__name__}") - print(f"Same type? {Package is _PackageFromPackage}") - print() - - # These are DIFFERENT types (defined in different modules) - print("=== Type Collision ===") - print(f"Full Package: {_PackageFromPackage.__module__}.{_PackageFromPackage.__name__}") - print( - f"Response Package: {_PackageFromCreateMediaBuyResponse.__module__}.{_PackageFromCreateMediaBuyResponse.__name__}" - ) - print(f"Same type? {_PackageFromPackage is _PackageFromCreateMediaBuyResponse}") - print() - - # Both have same __name__ but are distinct types - print("=== Name vs. Identity ===") - print(f"Full Package.__name__: {_PackageFromPackage.__name__}") - print(f"Response Package.__name__: {_PackageFromCreateMediaBuyResponse.__name__}") - print("Both are named 'Package', but they're distinct types!") - print() - - -def demonstrate_field_differences(): - """Show the structural differences between Package types.""" - - full_fields = set(_PackageFromPackage.model_fields.keys()) - response_fields = set(_PackageFromCreateMediaBuyResponse.model_fields.keys()) - - print("=== Field Comparison ===") - print(f"Full Package fields: {len(full_fields)}") - print(f"Response Package fields: {len(response_fields)}") - print() - - print("Fields only in Full Package:") - for field in sorted(full_fields - response_fields): - print(f" - {field}") - print() - - print("Fields only in Response Package:") - for field in sorted(response_fields - full_fields): - print(f" - {field}") - print() - - print("Shared fields:") - for field in sorted(full_fields & response_fields): - print(f" - {field}") - print() - - -def demonstrate_type_safety(): - """Show how type checkers handle these distinct types.""" - - # Create minimal package (as returned in create response) - minimal = _PackageFromCreateMediaBuyResponse( - buyer_ref="buyer-123", - package_id="pkg-001", - ) - - # Create full package (note: needs required fields based on schema) - full = _PackageFromPackage( - package_id="pkg-001", - buyer_ref="buyer-123", - product_id="prod-001", - pricing_option_id="pricing-001", - status="draft", # Required field - impressions=10000, - budget=500.0, - # ... many more fields available - ) - - print("=== Type Safety ===") - print(f"Minimal package type: {type(minimal).__name__}") - print(f"Full package type: {type(full).__name__}") - print() - - # isinstance() works correctly (checks actual type, not name) - print("isinstance checks:") - print(f" minimal is Response Package? {isinstance(minimal, _PackageFromCreateMediaBuyResponse)}") - print(f" minimal is Full Package? {isinstance(minimal, _PackageFromPackage)}") - print(f" full is Full Package? {isinstance(full, _PackageFromPackage)}") - print(f" full is Response Package? {isinstance(full, _PackageFromCreateMediaBuyResponse)}") - print() - - -def demonstrate_best_practices(): - """Show the recommended way to work with Package types.""" - - print("=== Best Practices ===") - print() - - print("āœ… CORRECT: Import from public API") - print(" from adcp import Package") - print(" pkg = Package(package_id='...', ...)") - print() - - print("āœ… CORRECT: Use in type hints") - print(" def process_package(pkg: Package) -> None: ...") - print() - - print("āŒ WRONG: Import from internal modules") - print(" from adcp.types.generated_poc.package import Package # NO!") - print(" from adcp.types._generated import Package # NO!") - print() - - print("ā„¹ļø ADVANCED: If you need to distinguish response vs full package:") - print(" from adcp.types._generated import (") - print(" _PackageFromPackage as FullPackage,") - print(" _PackageFromCreateMediaBuyResponse as CreatedPackage,") - print(" )") - print(" def handle_response(pkg: CreatedPackage) -> None: ...") - print() - - -def demonstrate_pitfall(): - """Show a potential pitfall with runtime type checking.""" - - minimal = _PackageFromCreateMediaBuyResponse( - buyer_ref="buyer-123", package_id="pkg-001" - ) - - full = _PackageFromPackage( - package_id="pkg-001", - buyer_ref="buyer-123", - status="draft", # Required - ) - - print("=== Potential Pitfall ===") - print() - print("āŒ BAD: Checking type by __name__ (doesn't distinguish variants)") - print(f" type(minimal).__name__ == 'Package': {type(minimal).__name__ == 'Package'}") - print(f" type(full).__name__ == 'Package': {type(full).__name__ == 'Package'}") - print(" Both return True, even though they're different types!") - print() - - print("āœ… GOOD: Using isinstance() (correctly distinguishes variants)") - print(f" isinstance(minimal, _PackageFromCreateMediaBuyResponse): {isinstance(minimal, _PackageFromCreateMediaBuyResponse)}") - print(f" isinstance(full, _PackageFromPackage): {isinstance(full, _PackageFromPackage)}") - print() - - -def why_collisions_exist(): - """Explain the root cause of Package collision.""" - - print("=== Why Do Collisions Exist? ===") - print() - print("The AdCP JSON schemas define 'Package' in two different contexts:") - print() - print("1. package.json: Full package definition with all fields") - print(" - Used for complete package data") - print(" - Has fields like: impressions, budget, pacing, targeting, etc.") - print(" - This is what you work with in your application") - print() - print("2. create-media-buy-response.json: Minimal package info") - print(" - Used in API response to confirm package creation") - print(" - Only has: buyer_ref, package_id") - print(" - This is what the API returns when creating a media buy") - print() - print("The code generator creates both types with the name 'Package'") - print("because that's how they're named in the schemas.") - print() - print("Our solution: Import both, export full definition as 'Package',") - print("and provide qualified names for internal use if needed.") - print() - - -def main(): - """Run all demonstrations.""" - why_collisions_exist() - print("=" * 80) - print() - - demonstrate_collision() - print("=" * 80) - print() - - demonstrate_field_differences() - print("=" * 80) - print() - - demonstrate_type_safety() - print("=" * 80) - print() - - demonstrate_best_practices() - print("=" * 80) - print() - - demonstrate_pitfall() - - -if __name__ == "__main__": - main() From b5ca5f386fceb0accae5842ee3335184ae8953a6 Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Tue, 18 Nov 2025 20:43:10 -0500 Subject: [PATCH 5/6] fix: resolve PublisherProperties name collision with qualified exports The consolidation script now handles PublisherProperties name collisions between adagents and product modules by exporting both with qualified names: - _PublisherPropertiesFromProduct (used for semantic aliases) - _PublisherPropertiesFromAdagents - _PublisherProperties4FromProduct (by_id discriminator) - _PublisherProperties5FromProduct (by_tag discriminator) Updated aliases.py and tests to use qualified names from _generated.py instead of importing directly from generated_poc modules, enforcing proper import boundaries. All 295 tests pass. Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- scripts/consolidate_exports.py | 37 ++++++++++++++++--------- src/adcp/types/_generated.py | 21 +++++++++------ src/adcp/types/aliases.py | 16 +++++------ tests/test_discriminated_unions.py | 12 ++++++--- tests/test_type_aliases.py | 43 +++++++++++++----------------- 5 files changed, 72 insertions(+), 57 deletions(-) diff --git a/scripts/consolidate_exports.py b/scripts/consolidate_exports.py index 64c9375d..bb892a95 100644 --- a/scripts/consolidate_exports.py +++ b/scripts/consolidate_exports.py @@ -58,10 +58,17 @@ def generate_consolidated_exports() -> str: all_exports = set() collisions = [] - # Special handling for known collision: Package type - # We need BOTH Package types available, so import them with qualified names + # Special handling for known collisions + # We need BOTH versions of these types available, so import them with qualified names + KNOWN_COLLISIONS = { + "Package": {"package", "create_media_buy_response"}, + "PublisherProperties": {"product", "adagents"}, + "PublisherProperties4": {"product", "adagents"}, + "PublisherProperties5": {"product", "adagents"}, + } + special_imports = [] - package_modules_seen = set() + collision_modules_seen: dict[str, set[str]] = {name: set() for name in KNOWN_COLLISIONS} for module_path in modules: module_name = module_path.stem @@ -73,9 +80,9 @@ def generate_consolidated_exports() -> str: # Filter out names that collide with already-exported names unique_exports = set() for export_name in exports: - # Special case: Package collision - track all modules that define it - if export_name == "Package" and module_name in ("package", "create_media_buy_response"): - package_modules_seen.add(module_name) + # Special case: Known collisions - track all modules that define them + if export_name in KNOWN_COLLISIONS and module_name in KNOWN_COLLISIONS[export_name]: + collision_modules_seen[export_name].add(module_name) export_to_module[export_name] = module_name # Track that we've seen it continue # Don't add to unique_exports, we'll handle specially @@ -102,13 +109,17 @@ def generate_consolidated_exports() -> str: all_exports.update(unique_exports) - # Generate special imports for Package collision - if package_modules_seen: - collisions.append(f" Package: defined in {sorted(package_modules_seen)} (both exported with qualified names)") - for module_name in sorted(package_modules_seen): - qualified_name = f"_PackageFrom{module_name.replace('_', ' ').title().replace(' ', '')}" + # Generate special imports for all known collisions + for type_name, modules_seen in collision_modules_seen.items(): + if not modules_seen: + continue + collisions.append( + f" {type_name}: defined in {sorted(modules_seen)} (all exported with qualified names)" + ) + for module_name in sorted(modules_seen): + qualified_name = f"_{type_name}From{module_name.replace('_', ' ').title().replace(' ', '')}" special_imports.append( - f"from adcp.types.generated_poc.{module_name} import Package as {qualified_name}" + f"from adcp.types.generated_poc.{module_name} import {type_name} as {qualified_name}" ) all_exports.add(qualified_name) @@ -144,7 +155,7 @@ def generate_consolidated_exports() -> str: # Add special imports for name collisions if special_imports: - lines.extend(["", "# Special imports for name collisions (Package type)"]) + lines.extend(["", "# Special imports for name collisions (qualified names for types defined in multiple modules)"]) lines.extend(special_imports) # Add backward compatibility aliases (only if source exists) diff --git a/src/adcp/types/_generated.py b/src/adcp/types/_generated.py index a4fb7769..3610740f 100644 --- a/src/adcp/types/_generated.py +++ b/src/adcp/types/_generated.py @@ -10,7 +10,7 @@ DO NOT EDIT MANUALLY. Generated from: https://github.com/adcontextprotocol/adcp/tree/main/schemas -Generation date: 2025-11-19 01:13:16 UTC +Generation date: 2025-11-19 01:40:18 UTC """ # ruff: noqa: E501, I001 from __future__ import annotations @@ -19,7 +19,7 @@ from adcp.types.generated_poc.activate_signal_request import ActivateSignalRequest from adcp.types.generated_poc.activate_signal_response import ActivateSignalResponse, ActivateSignalResponse1, ActivateSignalResponse2 from adcp.types.generated_poc.activation_key import ActivationKey1, ActivationKey2 -from adcp.types.generated_poc.adagents import AuthorizedAgents, AuthorizedAgents1, AuthorizedAgents2, AuthorizedAgents3, AuthorizedSalesAgents, Contact, PropertyId, PropertyTag, PublisherProperties, PublisherProperties1, Tags +from adcp.types.generated_poc.adagents import AuthorizedAgents, AuthorizedAgents1, AuthorizedAgents2, AuthorizedAgents3, AuthorizedSalesAgents, Contact, PropertyId, PropertyTag, PublisherProperties1, Tags from adcp.types.generated_poc.asset_type import AssetTypeSchema, ContentLength, Dimensions, Duration, FileSize, Quality, Requirements, Type from adcp.types.generated_poc.audio_asset import AudioAsset from adcp.types.generated_poc.brand_manifest import Asset, AssetType, BrandManifest, Colors, Disclaimer, FeedFormat, Fonts, Logo, Metadata, ProductCatalog, UpdateFrequency @@ -80,7 +80,7 @@ from adcp.types.generated_poc.preview_creative_response import Input4, Preview, Preview1, Preview2, PreviewCreativeResponse, PreviewCreativeResponse1, PreviewCreativeResponse2, Response, Response1, Results, Results1 from adcp.types.generated_poc.preview_render import Embedding, PreviewRender, PreviewRender1, PreviewRender2, PreviewRender3 from adcp.types.generated_poc.pricing_model import PricingModel -from adcp.types.generated_poc.product import DeliveryMeasurement, Product, ProductCard, ProductCardDetailed, PublisherProperties4, PublisherProperties5 +from adcp.types.generated_poc.product import DeliveryMeasurement, Product, ProductCard, ProductCardDetailed from adcp.types.generated_poc.promoted_offerings import AssetSelectors, Offering, PromotedOfferings from adcp.types.generated_poc.promoted_products import PromotedProducts from adcp.types.generated_poc.property import Identifier, Property, PropertyType, Tag @@ -113,9 +113,13 @@ from adcp.types.generated_poc.webhook_asset import Method, Method1, ResponseType, Security, WebhookAsset from adcp.types.generated_poc.webhook_payload import WebhookPayload -# Special imports for name collisions (Package type) +# Special imports for name collisions (qualified names for types defined in multiple modules) from adcp.types.generated_poc.create_media_buy_response import Package as _PackageFromCreateMediaBuyResponse from adcp.types.generated_poc.package import Package as _PackageFromPackage +from adcp.types.generated_poc.adagents import PublisherProperties as _PublisherPropertiesFromAdagents +from adcp.types.generated_poc.product import PublisherProperties as _PublisherPropertiesFromProduct +from adcp.types.generated_poc.product import PublisherProperties4 as _PublisherProperties4FromProduct +from adcp.types.generated_poc.product import PublisherProperties5 as _PublisherProperties5FromProduct # Backward compatibility aliases for renamed types Channels = AdvertisingChannels @@ -161,9 +165,8 @@ "PropertyType", "ProtocolEnvelope", "ProtocolResponse", "ProvidePerformanceFeedbackRequest", "ProvidePerformanceFeedbackResponse", "ProvidePerformanceFeedbackResponse1", "ProvidePerformanceFeedbackResponse2", "PublisherDomain", "PublisherIdentifierTypes", - "PublisherProperties", "PublisherProperties1", "PublisherProperties4", "PublisherProperties5", - "PushNotificationConfig", "Quality", "QuartileData", "QuerySummary", "Render", - "ReportingCapabilities", "ReportingFrequency", "ReportingPeriod", "ReportingWebhook", + "PublisherProperties1", "PushNotificationConfig", "Quality", "QuartileData", "QuerySummary", + "Render", "ReportingCapabilities", "ReportingFrequency", "ReportingPeriod", "ReportingWebhook", "Request", "RequestedMetric", "Requirements", "Response", "Response1", "ResponseType", "Responsive", "Results", "Results1", "Scheme", "Security", "Signal", "SignalType", "Sort", "SortApplied", "StandardFormatIds", "Status", "StatusFilter", "StatusFilterEnum", @@ -176,5 +179,7 @@ "UrlType", "ValidationMode", "VastAsset1", "VastAsset2", "VastVersion", "VcpmAuctionPricingOption", "VcpmFixedRatePricingOption", "VenueBreakdownItem", "VideoAsset", "ViewThreshold", "ViewThreshold1", "WebhookAsset", "WebhookPayload", - "_PackageFromCreateMediaBuyResponse", "_PackageFromPackage" + "_PackageFromCreateMediaBuyResponse", "_PackageFromPackage", + "_PublisherProperties4FromProduct", "_PublisherProperties5FromProduct", + "_PublisherPropertiesFromAdagents", "_PublisherPropertiesFromProduct" ] diff --git a/src/adcp/types/aliases.py b/src/adcp/types/aliases.py index 126f762b..5a433dfc 100644 --- a/src/adcp/types/aliases.py +++ b/src/adcp/types/aliases.py @@ -85,23 +85,23 @@ VastAsset1, VastAsset2, ) + +# Import all generated types that need semantic aliases from adcp.types._generated import ( - PublisherProperties as PublisherPropertiesInternal, + # Package types (from name collision resolution) + _PackageFromCreateMediaBuyResponse as CreatedPackageInternal, ) from adcp.types._generated import ( - PublisherProperties4 as PublisherPropertiesByIdInternal, + _PackageFromPackage as FullPackageInternal, ) from adcp.types._generated import ( - PublisherProperties5 as PublisherPropertiesByTagInternal, + _PublisherProperties4FromProduct as PublisherPropertiesByIdInternal, ) - -# Import all generated types that need semantic aliases from adcp.types._generated import ( - # Package types (from name collision resolution) - _PackageFromCreateMediaBuyResponse as CreatedPackageInternal, + _PublisherProperties5FromProduct as PublisherPropertiesByTagInternal, ) from adcp.types._generated import ( - _PackageFromPackage as FullPackageInternal, + _PublisherPropertiesFromProduct as PublisherPropertiesInternal, ) # ============================================================================ diff --git a/tests/test_discriminated_unions.py b/tests/test_discriminated_unions.py index 3e05dc20..4a65e9e7 100644 --- a/tests/test_discriminated_unions.py +++ b/tests/test_discriminated_unions.py @@ -22,8 +22,8 @@ UrlVastAsset, ) -# Keep using generated names for authorization/deployment/destination variants -# since these don't have semantic aliases yet +# Keep using generated names for authorization variants +# Deployment and Destination now have semantic aliases from adcp.types._generated import ( AuthorizedAgents, # property_ids variant AuthorizedAgents1, # property_tags variant @@ -33,10 +33,14 @@ Deployment2, # Agent Destination1, # Platform Destination2, # Agent - PublisherProperties4, # selection_type='by_id' - PublisherProperties5, # selection_type='by_tag' + _PublisherProperties4FromProduct, # selection_type='by_id' (qualified name) + _PublisherProperties5FromProduct, # selection_type='by_tag' (qualified name) ) +# Use qualified names for local aliases in this test +PublisherProperties4 = _PublisherProperties4FromProduct +PublisherProperties5 = _PublisherProperties5FromProduct + class TestAuthorizationDiscriminatedUnions: """Test authorization_type discriminated unions in adagents.json. diff --git a/tests/test_type_aliases.py b/tests/test_type_aliases.py index a449a840..f2666b01 100644 --- a/tests/test_type_aliases.py +++ b/tests/test_type_aliases.py @@ -302,16 +302,16 @@ def test_package_type_aliases_imports(): def test_package_type_aliases_point_to_correct_modules(): """Test that Package aliases point to the correct generated types.""" from adcp import CreatedPackageReference, Package - from adcp.types.generated_poc.create_media_buy_response import ( - Package as ResponsePackage, + from adcp.types._generated import ( + _PackageFromCreateMediaBuyResponse, + _PackageFromPackage, ) - from adcp.types.generated_poc.package import Package as DomainPackage # Package should point to the full domain package - assert Package is DomainPackage + assert Package is _PackageFromPackage # CreatedPackageReference should point to the response package - assert CreatedPackageReference is ResponsePackage + assert CreatedPackageReference is _PackageFromCreateMediaBuyResponse # Verify they're different types assert Package is not CreatedPackageReference @@ -434,16 +434,16 @@ def test_publisher_properties_aliases_imports(): def test_publisher_properties_aliases_point_to_correct_types(): """Test that PublisherProperties aliases point to the correct generated types.""" from adcp import PublisherPropertiesAll, PublisherPropertiesById, PublisherPropertiesByTag - from adcp.types.generated_poc.product import ( - PublisherProperties, - PublisherProperties4, - PublisherProperties5, + from adcp.types._generated import ( + _PublisherProperties4FromProduct, + _PublisherProperties5FromProduct, + _PublisherPropertiesFromProduct, ) - # Verify aliases point to correct types - assert PublisherPropertiesAll is PublisherProperties - assert PublisherPropertiesById is PublisherProperties4 - assert PublisherPropertiesByTag is PublisherProperties5 + # Verify aliases point to correct types (from product module, not adagents) + assert PublisherPropertiesAll is _PublisherPropertiesFromProduct + assert PublisherPropertiesById is _PublisherProperties4FromProduct + assert PublisherPropertiesByTag is _PublisherProperties5FromProduct # Verify they're different types assert PublisherPropertiesAll is not PublisherPropertiesById @@ -455,12 +455,7 @@ def test_publisher_properties_aliases_have_correct_discriminators(): """Test that PublisherProperties aliases have the correct discriminator values.""" from adcp import PublisherPropertiesAll, PublisherPropertiesById, PublisherPropertiesByTag - # Check that discriminator field has correct literal type - all_selection_type = PublisherPropertiesAll.__annotations__["selection_type"] - by_id_selection_type = PublisherPropertiesById.__annotations__["selection_type"] - by_tag_selection_type = PublisherPropertiesByTag.__annotations__["selection_type"] - - # Verify the annotations contain Literal types + # Verify the annotations contain selection_type discriminator field assert "selection_type" in PublisherPropertiesAll.__annotations__ assert "selection_type" in PublisherPropertiesById.__annotations__ assert "selection_type" in PublisherPropertiesByTag.__annotations__ @@ -469,8 +464,6 @@ def test_publisher_properties_aliases_have_correct_discriminators(): def test_publisher_properties_aliases_can_instantiate(): """Test that PublisherProperties aliases can be used to create instances.""" from adcp import ( - PropertyId, - PropertyTag, PublisherPropertiesAll, PublisherPropertiesById, PublisherPropertiesByTag, @@ -484,20 +477,22 @@ def test_publisher_properties_aliases_can_instantiate(): assert props_all.selection_type == "all" # Create PublisherPropertiesById + # Note: property_ids should be plain strings (PropertyId is a constrained string type) props_by_id = PublisherPropertiesById( publisher_domain="example.com", selection_type="by_id", - property_ids=[PropertyId("homepage"), PropertyId("sports")], + property_ids=["homepage", "sports"], ) assert props_by_id.publisher_domain == "example.com" assert props_by_id.selection_type == "by_id" assert len(props_by_id.property_ids) == 2 # Create PublisherPropertiesByTag + # Note: property_tags should be plain strings (PropertyTag is a constrained string type) props_by_tag = PublisherPropertiesByTag( publisher_domain="example.com", selection_type="by_tag", - property_tags=[PropertyTag("premium"), PropertyTag("video")], + property_tags=["premium", "video"], ) assert props_by_tag.publisher_domain == "example.com" assert props_by_tag.selection_type == "by_tag" @@ -578,8 +573,8 @@ def test_deployment_aliases_point_to_correct_types(): def test_deployment_aliases_can_instantiate(): """Test that Deployment aliases can be used to create instances.""" + from adcp import AgentDeployment, PlatformDeployment - from datetime import datetime, timezone # Create PlatformDeployment platform_deployment = PlatformDeployment( From ee07efa9befcd96b4453efacc1080e4fcbd33dfb Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Tue, 18 Nov 2025 21:04:37 -0500 Subject: [PATCH 6/6] feat: adopt shared PublisherPropertySelector schema from upstream After upstream PR #219 merged, the PublisherProperties type is now defined in a shared publisher-property-selector.json schema with all 3 variants (all, by_id, by_tag). Changes: - Synced schemas from upstream (now includes publisher-property-selector.json) - Regenerated types (PublisherPropertySelector1/2/3 from shared schema) - Removed PublisherProperties from collision handling in consolidate_exports.py - Updated aliases.py to use PublisherPropertySelector types - Updated tests to reference shared types instead of module-specific variants Benefits: - Single source of truth for publisher property selection - No more name collisions between adagents and product modules - Simplified type system with cleaner exports - DRY schema definitions maintained upstream All 295 tests pass. Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- schemas/cache/.hashes.json | 7 +- schemas/cache/1.0.0/adagents.json | 67 +-- schemas/cache/1.0.0/index.json | 546 +++++++++--------- schemas/cache/1.0.0/product.json | 89 +-- .../1.0.0/publisher-property-selector.json | 94 +++ scripts/consolidate_exports.py | 3 - src/adcp/types/_generated.py | 18 +- src/adcp/types/aliases.py | 16 +- src/adcp/types/generated_poc/adagents.py | 111 ++-- src/adcp/types/generated_poc/product.py | 85 +-- .../publisher_property_selector.py | 81 +++ tests/test_discriminated_unions.py | 10 +- tests/test_type_aliases.py | 14 +- 13 files changed, 528 insertions(+), 613 deletions(-) create mode 100644 schemas/cache/1.0.0/publisher-property-selector.json create mode 100644 src/adcp/types/generated_poc/publisher_property_selector.py diff --git a/schemas/cache/.hashes.json b/schemas/cache/.hashes.json index cbb29428..abd7830f 100644 --- a/schemas/cache/.hashes.json +++ b/schemas/cache/.hashes.json @@ -1,6 +1,6 @@ { - "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/index.json": "eed524f6ca3b7b8981035a74f3eaf339da6fadab85e4af9196c1c7c4d5150095", - "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/adagents.json": "bd0e7cd9189b191d827a3ab7fb9d4f9ef4913377c816c5aa27af30bfd20d3451", + "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/index.json": "46f68f0e55d63361c0eda1cc7ffff4f25c9416467dd3a04db2cca0973f086b7d", + "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/adagents.json": "f32e3778a454b7ae65f42952906f06466008bfd1208b83729e14d5f2164902d5", "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/core/activation-key.json": "bb9c20c6200b651ce6db89f7160be60b9845dbbb4390a13363ea5b82c1c3c786", "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/core/asset-type.json": "a61ac8e14a61adef64d10d6ab39c204d703c087f29efa45c69c527988c93cd3d", "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/core/assets/audio-asset.json": "e25d688d22c8b130d9d1a2a09f9aa462cb7a5c83db6eea6f328cd937f8625a3f", @@ -34,11 +34,12 @@ "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/core/performance-feedback.json": "80384474042b6cda08b1128859143ec5822d6dcc907ba1fa3ecf81719e7644a7", "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/core/placement.json": "ea814df6d878232bfdb1249fe199a1e32ec18598b7d3e3c57324d6e6120d9cf8", "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/core/pricing-option.json": "cfaeff3d4fc49e0d3ae76364e246b3b7a772ef12cbda65b1cff400ab1f841bfa", - "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/core/product.json": "f3ef04e850cb61c2ba86e05da1d5a352b63031ddbb42fbdffbbbd6c8432ad5c5", + "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/core/product.json": "e1b4faa9029bd06baa537fbf534993e7830b1bdc2241279dea4806c134cea50d", "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/core/promoted-offerings.json": "d8b4b92db0e2debc5c0ddbc0a8ff673f258f0bbc0348737df61be20a25827077", "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/core/promoted-products.json": "77773b1dce91b219ec5043c091eb2977a82ba301e03aead3868ba704e625379e", "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/core/property.json": "510458c96a93deb90d9fa3a4dfc11b63c113755dbec3de386690f6838213bc84", "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/core/protocol-envelope.json": "c6096b4ed4330c5e2045989bfd5cdc64fa6587cf8b0d1d2c19e33c7434cdacb8", + "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/core/publisher-property-selector.json": "3e4870d0446a5825c16365a99d49932517223c1d9d3d46a4fbf413d377ed43dd", "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/core/push-notification-config.json": "be2af5dbf7d398c958e59c70ab61a845e4a7d1f1e076412589d06d53454b64b0", "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/core/reporting-capabilities.json": "c463c8d512c17b8ac7afde34d782b5e5f700ed9cf73a52992a328f85ad24d568", "https://raw.githubusercontent.com/adcontextprotocol/adcp/main/static/schemas/v1/core/response.json": "0ac624a30da08e1aa90d2a9379f8c1ed29b704c3f5399224b9684672d3df9723", diff --git a/schemas/cache/1.0.0/adagents.json b/schemas/cache/1.0.0/adagents.json index 7e142bac..c50e6a85 100644 --- a/schemas/cache/1.0.0/adagents.json +++ b/schemas/cache/1.0.0/adagents.json @@ -318,7 +318,7 @@ "properties": { "description": "Specific properties this agent is authorized for (alternative to property_ids/property_tags)", "items": { - "$ref": "property.json" + "$ref": "/schemas/v1/core/property.json" }, "minItems": 1, "type": "array" @@ -354,68 +354,7 @@ "publisher_properties": { "description": "Properties from other publisher domains this agent is authorized for. Each entry specifies a publisher domain and which of their properties this agent can sell", "items": { - "oneOf": [ - { - "additionalProperties": false, - "properties": { - "property_ids": { - "description": "Specific property IDs from the publisher's adagents.json properties array", - "items": { - "pattern": "^[a-z0-9_]+$", - "type": "string" - }, - "minItems": 1, - "type": "array" - }, - "publisher_domain": { - "description": "Domain where the publisher's adagents.json is hosted (e.g., 'cnn.com')", - "pattern": "^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$", - "type": "string" - }, - "selection_type": { - "const": "by_id", - "description": "Discriminator indicating selection by specific property IDs", - "type": "string" - } - }, - "required": [ - "publisher_domain", - "selection_type", - "property_ids" - ], - "type": "object" - }, - { - "additionalProperties": false, - "properties": { - "property_tags": { - "description": "Property tags from the publisher's adagents.json tags. Agent is authorized for all properties with these tags", - "items": { - "pattern": "^[a-z0-9_]+$", - "type": "string" - }, - "minItems": 1, - "type": "array" - }, - "publisher_domain": { - "description": "Domain where the publisher's adagents.json is hosted (e.g., 'cnn.com')", - "pattern": "^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$", - "type": "string" - }, - "selection_type": { - "const": "by_tag", - "description": "Discriminator indicating selection by property tags", - "type": "string" - } - }, - "required": [ - "publisher_domain", - "selection_type", - "property_tags" - ], - "type": "object" - } - ] + "$ref": "/schemas/v1/core/publisher-property-selector.json" }, "minItems": 1, "type": "array" @@ -487,7 +426,7 @@ "properties": { "description": "Array of all properties covered by this adagents.json file. Same structure as list_authorized_properties response.", "items": { - "$ref": "property.json" + "$ref": "/schemas/v1/core/property.json" }, "minItems": 1, "type": "array" diff --git a/schemas/cache/1.0.0/index.json b/schemas/cache/1.0.0/index.json index 62c1a1fa..03c248d4 100644 --- a/schemas/cache/1.0.0/index.json +++ b/schemas/cache/1.0.0/index.json @@ -1,17 +1,16 @@ { - "$schema": "http://json-schema.org/draft-07/schema#", "$id": "/schemas/v1/index.json", - "title": "AdCP Schema Registry v1", - "version": "1.0.0", - "description": "Registry of all AdCP JSON schemas for validation and discovery", + "$schema": "http://json-schema.org/draft-07/schema#", "adcp_version": "2.4.0", - "standard_formats_version": "2.0.0", - "versioning": { - "note": "AdCP uses path-based versioning. The schema URL path (/schemas/v1/) indicates the version. Individual request/response schemas do NOT include adcp_version fields. Compatibility follows semantic versioning rules." - }, + "baseUrl": "/schemas/v1", "changelog": { + "1.8.0": { + "changes": [ + "Previous version with string-based dimensions" + ], + "date": "2025-10-13" + }, "2.0.0": { - "date": "2025-10-14", "breaking_changes": [ "Added renders array to Format schema with role and dimensions fields (replaces top-level dimensions)", "Formats now specify rendered outputs via renders array - enables companion ads, adaptive formats, and multi-placement", @@ -22,422 +21,427 @@ "Removed hints object from preview renders (use format lookup for render specifications instead)", "Changed preview render field names: output_id\u2192render_id, output_role\u2192role, added dimensions field" ], + "date": "2025-10-14", "rationale": "Renders array with structured dimensions eliminates parsing ambiguity and uniformly supports single and multi-render formats. Multi-render preview support enables companion ads and adaptive formats. Terminology changes prevent confusion between preview rendering and generative format outputs. Simplified schema removes redundant fields." - }, - "1.8.0": { - "date": "2025-10-13", - "changes": [ - "Previous version with string-based dimensions" - ] } }, + "description": "Registry of all AdCP JSON schemas for validation and discovery", + "examples": [ + { + "code": "const Ajv = require('ajv'); const ajv = new Ajv(); const schema = require('./schemas/v1/core/product.json'); const validate = ajv.compile(schema);", + "description": "JavaScript validation example", + "language": "javascript" + }, + { + "code": "import jsonschema; schema = {...}; jsonschema.validate(data, schema)", + "description": "Python validation example", + "language": "python" + }, + { + "code": "// Use everit-org/json-schema or similar library", + "description": "Java validation example", + "language": "java" + } + ], "lastUpdated": "2025-10-31", - "baseUrl": "/schemas/v1", "schemas": { + "adagents": { + "$ref": "/schemas/v1/adagents.json", + "description": "Authorized sales agents file format specification", + "file_location": "/.well-known/adagents.json", + "purpose": "Declares which sales agents are authorized to sell a publisher's advertising inventory" + }, "core": { "description": "Core data models used throughout AdCP", "schemas": { - "product": { - "$ref": "product.json", - "description": "Represents available advertising inventory" - }, - "media-buy": { - "$ref": "media-buy.json", - "description": "Represents a purchased advertising campaign" + "brand-manifest": { + "$ref": "/schemas/v1/core/brand-manifest.json", + "description": "Standardized brand information manifest for creative generation and media buying" }, - "package": { - "$ref": "package.json", - "description": "A specific product within a media buy (line item)" + "brand-manifest-ref": { + "$ref": "/schemas/v1/core/brand-manifest-ref.json", + "description": "Brand manifest reference (inline object or URL)" }, "creative-asset": { - "$ref": "creative-asset.json", + "$ref": "/schemas/v1/core/creative-asset.json", "description": "Creative asset for upload to library - supports static assets, generative formats, and third-party ad serving (VAST, DAAST, HTML, JavaScript)" }, - "targeting": { - "$ref": "targeting.json", - "description": "Audience targeting criteria" - }, - "frequency-cap": { - "$ref": "frequency-cap.json", - "description": "Frequency capping settings" + "creative-assignment": { + "$ref": "/schemas/v1/core/creative-assignment.json", + "description": "Assignment of a creative asset to a package" }, - "format": { - "$ref": "format.json", - "description": "Represents a creative format with its requirements" + "creative-manifest": { + "$ref": "/schemas/v1/core/creative-manifest.json", + "description": "Complete specification of a creative with all assets needed for rendering" }, - "measurement": { - "$ref": "measurement.json", - "description": "Measurement capabilities included with a product" + "creative-policy": { + "$ref": "/schemas/v1/core/creative-policy.json", + "description": "Creative requirements and restrictions for a product" }, "delivery-metrics": { - "$ref": "delivery-metrics.json", + "$ref": "/schemas/v1/core/delivery-metrics.json", "description": "Standard delivery metrics for reporting" }, - "creative-policy": { - "$ref": "creative-policy.json", - "description": "Creative requirements and restrictions for a product" + "deployment": { + "$ref": "/schemas/v1/core/deployment.json", + "description": "A signal deployment to a specific destination platform with activation status and key" }, - "response": { - "$ref": "response.json", - "description": "Standard response structure (MCP)" + "destination": { + "$ref": "/schemas/v1/core/destination.json", + "description": "A destination platform where signals can be activated (DSP, sales agent, etc.)" }, "error": { - "$ref": "error.json", + "$ref": "/schemas/v1/core/error.json", "description": "Standard error structure" }, - "sub-asset": { - "$ref": "sub-asset.json", - "description": "Sub-asset for multi-asset creative formats" + "format": { + "$ref": "/schemas/v1/core/format.json", + "description": "Represents a creative format with its requirements" }, - "creative-assignment": { - "$ref": "creative-assignment.json", - "description": "Assignment of a creative asset to a package" + "frequency-cap": { + "$ref": "/schemas/v1/core/frequency-cap.json", + "description": "Frequency capping settings" }, - "creative-manifest": { - "$ref": "creative-manifest.json", - "description": "Complete specification of a creative with all assets needed for rendering" + "measurement": { + "$ref": "/schemas/v1/core/measurement.json", + "description": "Measurement capabilities included with a product" + }, + "media-buy": { + "$ref": "/schemas/v1/core/media-buy.json", + "description": "Represents a purchased advertising campaign" + }, + "package": { + "$ref": "/schemas/v1/core/package.json", + "description": "A specific product within a media buy (line item)" }, "performance-feedback": { - "$ref": "performance-feedback.json", + "$ref": "/schemas/v1/core/performance-feedback.json", "description": "Performance feedback data for a media buy or package" }, - "property": { - "$ref": "property.json", - "description": "An advertising property that can be validated via adagents.json" + "placement": { + "$ref": "/schemas/v1/core/placement.json", + "description": "Represents a specific ad placement within a product's inventory" }, - "brand-manifest": { - "$ref": "brand-manifest.json", - "description": "Standardized brand information manifest for creative generation and media buying" + "pricing-option": { + "$ref": "/schemas/v1/core/pricing-option.json", + "description": "A pricing model option offered by a publisher for a product" }, - "brand-manifest-ref": { - "$ref": "brand-manifest-ref.json", - "description": "Brand manifest reference (inline object or URL)" + "product": { + "$ref": "/schemas/v1/core/product.json", + "description": "Represents available advertising inventory" }, "promoted-products": { - "$ref": "promoted-products.json", + "$ref": "/schemas/v1/core/promoted-products.json", "description": "Product or offering selection for campaigns with multiple selection methods" }, - "start-timing": { - "$ref": "start-timing.json", - "description": "Campaign start timing: 'asap' or ISO 8601 date-time" - }, - "pricing-option": { - "$ref": "pricing-option.json", - "description": "A pricing model option offered by a publisher for a product" + "property": { + "$ref": "/schemas/v1/core/property.json", + "description": "An advertising property that can be validated via adagents.json" }, "protocol-envelope": { - "$ref": "protocol-envelope.json", + "$ref": "/schemas/v1/core/protocol-envelope.json", "description": "Standard envelope structure added by protocol layer (MCP, A2A, REST) that wraps task response payloads with protocol-level fields like status, context_id, task_id, and message" }, - "placement": { - "$ref": "placement.json", - "description": "Represents a specific ad placement within a product's inventory" + "publisher-property-selector": { + "$ref": "/schemas/v1/core/publisher-property-selector.json", + "description": "Selects properties from a publisher's adagents.json - supports three patterns: all properties, specific IDs, or by tags" + }, + "response": { + "$ref": "/schemas/v1/core/response.json", + "description": "Standard response structure (MCP)" + }, + "start-timing": { + "$ref": "/schemas/v1/core/start-timing.json", + "description": "Campaign start timing: 'asap' or ISO 8601 date-time" + }, + "sub-asset": { + "$ref": "/schemas/v1/core/sub-asset.json", + "description": "Sub-asset for multi-asset creative formats" + }, + "targeting": { + "$ref": "/schemas/v1/core/targeting.json", + "description": "Audience targeting criteria" }, "webhook-payload": { - "$ref": "webhook-payload.json", + "$ref": "/schemas/v1/core/webhook-payload.json", "description": "Webhook payload structure sent when async task status changes - protocol-level fields at top-level (operation_id, task_type, status, etc.) and task-specific payload nested under 'result'" + } + } + }, + "creative": { + "asset_types": { + "$ref": "/schemas/v1/creative/asset-types/index.json", + "description": "Asset type definitions for creative manifests" + }, + "description": "Creative protocol task request/response schemas and asset type definitions", + "tasks": { + "build-creative": { + "request": { + "$ref": "/schemas/v1/media-buy/build-creative-request.json", + "description": "Request parameters for AI-powered creative generation" + }, + "response": { + "$ref": "/schemas/v1/media-buy/build-creative-response.json", + "description": "Response payload for build_creative task" + } }, - "destination": { - "$ref": "destination.json", - "description": "A destination platform where signals can be activated (DSP, sales agent, etc.)" + "list-creative-formats": { + "request": { + "$ref": "/schemas/v1/creative/list-creative-formats-request.json", + "description": "Request parameters for discovering creative formats from this creative agent" + }, + "response": { + "$ref": "/schemas/v1/creative/list-creative-formats-response.json", + "description": "Response payload with full format definitions - this is the authoritative source for format specifications" + } }, - "deployment": { - "$ref": "deployment.json", - "description": "A signal deployment to a specific destination platform with activation status and key" + "preview-creative": { + "request": { + "$ref": "/schemas/v1/creative/preview-creative-request.json", + "description": "Request parameters for generating creative previews" + }, + "response": { + "$ref": "/schemas/v1/creative/preview-creative-response.json", + "description": "Response payload for preview_creative task" + } } } }, "enums": { "description": "Enumerated types and constants", "schemas": { - "pricing-model": { - "$ref": "pricing-model.json", - "description": "Supported pricing models for advertising products" + "channels": { + "$ref": "/schemas/v1/enums/channels.json", + "description": "Advertising channels (display, video, dooh, ctv, audio, etc.)" + }, + "creative-status": { + "$ref": "/schemas/v1/enums/creative-status.json", + "description": "Status of a creative asset" }, "delivery-type": { - "$ref": "delivery-type.json", + "$ref": "/schemas/v1/enums/delivery-type.json", "description": "Type of inventory delivery" }, - "media-buy-status": { - "$ref": "media-buy-status.json", - "description": "Status of a media buy" + "frequency-cap-scope": { + "$ref": "/schemas/v1/enums/frequency-cap-scope.json", + "description": "Scope for frequency cap application" }, - "package-status": { - "$ref": "package-status.json", - "description": "Status of a package" + "identifier-types": { + "$ref": "/schemas/v1/enums/identifier-types.json", + "description": "Valid identifier types for property identification across different media types" }, - "creative-status": { - "$ref": "creative-status.json", - "description": "Status of a creative asset" + "media-buy-status": { + "$ref": "/schemas/v1/enums/media-buy-status.json", + "description": "Status of a media buy" }, "pacing": { - "$ref": "pacing.json", + "$ref": "/schemas/v1/enums/pacing.json", "description": "Budget pacing strategy" }, - "frequency-cap-scope": { - "$ref": "frequency-cap-scope.json", - "description": "Scope for frequency cap application" - }, - "standard-format-ids": { - "$ref": "standard-format-ids.json", - "description": "Enumeration of all standard creative format identifiers" + "package-status": { + "$ref": "/schemas/v1/enums/package-status.json", + "description": "Status of a package" }, - "identifier-types": { - "$ref": "identifier-types.json", - "description": "Valid identifier types for property identification across different media types" + "pricing-model": { + "$ref": "/schemas/v1/enums/pricing-model.json", + "description": "Supported pricing models for advertising products" }, "publisher-identifier-types": { - "$ref": "publisher-identifier-types.json", + "$ref": "/schemas/v1/enums/publisher-identifier-types.json", "description": "Valid identifier types for publisher/legal entity identification (TAG ID, DUNS, LEI, seller_id, GLN)" }, - "channels": { - "$ref": "channels.json", - "description": "Advertising channels (display, video, dooh, ctv, audio, etc.)" + "standard-format-ids": { + "$ref": "/schemas/v1/enums/standard-format-ids.json", + "description": "Enumeration of all standard creative format identifiers" }, "task-status": { - "$ref": "task-status.json", + "$ref": "/schemas/v1/enums/task-status.json", "description": "Standardized task status values based on A2A TaskState enum" }, "task-type": { - "$ref": "task-type.json", + "$ref": "/schemas/v1/enums/task-type.json", "description": "Valid AdCP task types across all domains (create_media_buy, update_media_buy, sync_creatives, activate_signal, get_signals)" } } }, - "pricing-options": { - "description": "Individual pricing model schemas with model-specific validation. CPM and vCPM support both fixed and auction pricing; all other models are fixed-rate only.", - "schemas": { - "cpm-fixed-option": { - "$ref": "cpm-fixed-option.json", - "description": "Cost Per Mille (CPM) fixed-rate pricing for direct/guaranteed deals" - }, - "cpm-auction-option": { - "$ref": "cpm-auction-option.json", - "description": "Cost Per Mille (CPM) auction-based pricing for programmatic/non-guaranteed inventory" - }, - "vcpm-fixed-option": { - "$ref": "vcpm-fixed-option.json", - "description": "Viewable Cost Per Mille (vCPM) fixed-rate pricing for viewability-guaranteed deals" - }, - "vcpm-auction-option": { - "$ref": "vcpm-auction-option.json", - "description": "Viewable Cost Per Mille (vCPM) auction-based pricing for programmatic inventory with viewability guarantee" - }, - "cpc-option": { - "$ref": "cpc-option.json", - "description": "Cost Per Click (CPC) fixed-rate pricing for performance campaigns" - }, - "cpcv-option": { - "$ref": "cpcv-option.json", - "description": "Cost Per Completed View (CPCV) fixed-rate pricing for video/audio" - }, - "cpv-option": { - "$ref": "cpv-option.json", - "description": "Cost Per View (CPV) fixed-rate pricing with threshold" - }, - "cpp-option": { - "$ref": "cpp-option.json", - "description": "Cost Per Point (CPP) fixed-rate pricing for TV/audio with demographic measurement" - }, - "flat-rate-option": { - "$ref": "flat-rate-option.json", - "description": "Flat rate pricing for DOOH and sponsorships" - } - } - }, "media-buy": { "description": "Media buy task request/response schemas", "supporting-schemas": { "package-request": { - "$ref": "package-request.json", + "$ref": "/schemas/v1/media-buy/package-request.json", "description": "Package configuration for media buy creation - used within create_media_buy request" } }, "tasks": { - "get-products": { - "request": { - "$ref": "get-products-request.json", - "description": "Request parameters for discovering available advertising products" - }, - "response": { - "$ref": "get-products-response.json", - "description": "Response payload for get_products task" - } - }, - "list-creative-formats": { - "request": { - "$ref": "list-creative-formats-request.json", - "description": "Request parameters for discovering format IDs and creative agents supported by this sales agent" - }, - "response": { - "$ref": "list-creative-formats-response.json", - "description": "Response payload with format_ids and creative_agents list. Sales agent returns which formats it supports and which creative agents provide those formats. Buyers query creative agents for full format specifications." - } - }, "create-media-buy": { "request": { - "$ref": "create-media-buy-request.json", + "$ref": "/schemas/v1/media-buy/create-media-buy-request.json", "description": "Request parameters for creating a media buy" }, "response": { - "$ref": "create-media-buy-response.json", + "$ref": "/schemas/v1/media-buy/create-media-buy-response.json", "description": "Response payload for create_media_buy task" } }, - "sync-creatives": { + "get-media-buy-delivery": { "request": { - "$ref": "sync-creatives-request.json", - "description": "Request parameters for syncing creative assets with upsert semantics" + "$ref": "/schemas/v1/media-buy/get-media-buy-delivery-request.json", + "description": "Request parameters for retrieving comprehensive delivery metrics" }, "response": { - "$ref": "sync-creatives-response.json", - "description": "Response payload for sync_creatives task" + "$ref": "/schemas/v1/media-buy/get-media-buy-delivery-response.json", + "description": "Response payload for get_media_buy_delivery task" } }, - "list-creatives": { + "get-products": { "request": { - "$ref": "list-creatives-request.json", - "description": "Request parameters for querying creative library with filtering and pagination" + "$ref": "/schemas/v1/media-buy/get-products-request.json", + "description": "Request parameters for discovering available advertising products" }, "response": { - "$ref": "list-creatives-response.json", - "description": "Response payload for list_creatives task" + "$ref": "/schemas/v1/media-buy/get-products-response.json", + "description": "Response payload for get_products task" } }, - "update-media-buy": { + "list-authorized-properties": { "request": { - "$ref": "update-media-buy-request.json", - "description": "Request parameters for updating campaign and package settings" + "$ref": "/schemas/v1/media-buy/list-authorized-properties-request.json", + "description": "Request parameters for discovering all properties this agent is authorized to represent" }, "response": { - "$ref": "update-media-buy-response.json", - "description": "Response payload for update_media_buy task" + "$ref": "/schemas/v1/media-buy/list-authorized-properties-response.json", + "description": "Response payload for list_authorized_properties task" } }, - "get-media-buy-delivery": { + "list-creative-formats": { "request": { - "$ref": "get-media-buy-delivery-request.json", - "description": "Request parameters for retrieving comprehensive delivery metrics" + "$ref": "/schemas/v1/media-buy/list-creative-formats-request.json", + "description": "Request parameters for discovering format IDs and creative agents supported by this sales agent" }, "response": { - "$ref": "get-media-buy-delivery-response.json", - "description": "Response payload for get_media_buy_delivery task" + "$ref": "/schemas/v1/media-buy/list-creative-formats-response.json", + "description": "Response payload with format_ids and creative_agents list. Sales agent returns which formats it supports and which creative agents provide those formats. Buyers query creative agents for full format specifications." } }, - "list-authorized-properties": { + "list-creatives": { "request": { - "$ref": "list-authorized-properties-request.json", - "description": "Request parameters for discovering all properties this agent is authorized to represent" + "$ref": "/schemas/v1/media-buy/list-creatives-request.json", + "description": "Request parameters for querying creative library with filtering and pagination" }, "response": { - "$ref": "list-authorized-properties-response.json", - "description": "Response payload for list_authorized_properties task" + "$ref": "/schemas/v1/media-buy/list-creatives-response.json", + "description": "Response payload for list_creatives task" } }, "provide-performance-feedback": { "request": { - "$ref": "provide-performance-feedback-request.json", + "$ref": "/schemas/v1/media-buy/provide-performance-feedback-request.json", "description": "Request parameters for sharing performance outcomes with publishers" }, "response": { - "$ref": "provide-performance-feedback-response.json", + "$ref": "/schemas/v1/media-buy/provide-performance-feedback-response.json", "description": "Response payload for provide_performance_feedback task" } - } - } - }, - "creative": { - "description": "Creative protocol task request/response schemas and asset type definitions", - "tasks": { - "build-creative": { - "request": { - "$ref": "build-creative-request.json", - "description": "Request parameters for AI-powered creative generation" - }, - "response": { - "$ref": "build-creative-response.json", - "description": "Response payload for build_creative task" - } }, - "preview-creative": { + "sync-creatives": { "request": { - "$ref": "preview-creative-request.json", - "description": "Request parameters for generating creative previews" + "$ref": "/schemas/v1/media-buy/sync-creatives-request.json", + "description": "Request parameters for syncing creative assets with upsert semantics" }, "response": { - "$ref": "preview-creative-response.json", - "description": "Response payload for preview_creative task" + "$ref": "/schemas/v1/media-buy/sync-creatives-response.json", + "description": "Response payload for sync_creatives task" } }, - "list-creative-formats": { + "update-media-buy": { "request": { - "$ref": "list-creative-formats-request.json", - "description": "Request parameters for discovering creative formats from this creative agent" + "$ref": "/schemas/v1/media-buy/update-media-buy-request.json", + "description": "Request parameters for updating campaign and package settings" }, "response": { - "$ref": "list-creative-formats-response.json", - "description": "Response payload with full format definitions - this is the authoritative source for format specifications" + "$ref": "/schemas/v1/media-buy/update-media-buy-response.json", + "description": "Response payload for update_media_buy task" } } - }, - "asset_types": { - "$ref": "index.json", - "description": "Asset type definitions for creative manifests" + } + }, + "pricing-options": { + "description": "Individual pricing model schemas with model-specific validation. CPM and vCPM support both fixed and auction pricing; all other models are fixed-rate only.", + "schemas": { + "cpc-option": { + "$ref": "/schemas/v1/pricing-options/cpc-option.json", + "description": "Cost Per Click (CPC) fixed-rate pricing for performance campaigns" + }, + "cpcv-option": { + "$ref": "/schemas/v1/pricing-options/cpcv-option.json", + "description": "Cost Per Completed View (CPCV) fixed-rate pricing for video/audio" + }, + "cpm-auction-option": { + "$ref": "/schemas/v1/pricing-options/cpm-auction-option.json", + "description": "Cost Per Mille (CPM) auction-based pricing for programmatic/non-guaranteed inventory" + }, + "cpm-fixed-option": { + "$ref": "/schemas/v1/pricing-options/cpm-fixed-option.json", + "description": "Cost Per Mille (CPM) fixed-rate pricing for direct/guaranteed deals" + }, + "cpp-option": { + "$ref": "/schemas/v1/pricing-options/cpp-option.json", + "description": "Cost Per Point (CPP) fixed-rate pricing for TV/audio with demographic measurement" + }, + "cpv-option": { + "$ref": "/schemas/v1/pricing-options/cpv-option.json", + "description": "Cost Per View (CPV) fixed-rate pricing with threshold" + }, + "flat-rate-option": { + "$ref": "/schemas/v1/pricing-options/flat-rate-option.json", + "description": "Flat rate pricing for DOOH and sponsorships" + }, + "vcpm-auction-option": { + "$ref": "/schemas/v1/pricing-options/vcpm-auction-option.json", + "description": "Viewable Cost Per Mille (vCPM) auction-based pricing for programmatic inventory with viewability guarantee" + }, + "vcpm-fixed-option": { + "$ref": "/schemas/v1/pricing-options/vcpm-fixed-option.json", + "description": "Viewable Cost Per Mille (vCPM) fixed-rate pricing for viewability-guaranteed deals" + } } }, "signals": { "description": "Signals protocol task request/response schemas", "tasks": { - "get-signals": { + "activate-signal": { "request": { - "$ref": "get-signals-request.json", - "description": "Request parameters for discovering signals based on description" + "$ref": "/schemas/v1/signals/activate-signal-request.json", + "description": "Request parameters for activating a signal on a specific platform/account" }, "response": { - "$ref": "get-signals-response.json", - "description": "Response payload for get_signals task" + "$ref": "/schemas/v1/signals/activate-signal-response.json", + "description": "Response payload for activate_signal task" } }, - "activate-signal": { + "get-signals": { "request": { - "$ref": "activate-signal-request.json", - "description": "Request parameters for activating a signal on a specific platform/account" + "$ref": "/schemas/v1/signals/get-signals-request.json", + "description": "Request parameters for discovering signals based on description" }, "response": { - "$ref": "activate-signal-response.json", - "description": "Response payload for activate_signal task" + "$ref": "/schemas/v1/signals/get-signals-response.json", + "description": "Response payload for get_signals task" } } } - }, - "adagents": { - "description": "Authorized sales agents file format specification", - "$ref": "adagents.json", - "file_location": "/.well-known/adagents.json", - "purpose": "Declares which sales agents are authorized to sell a publisher's advertising inventory" } }, + "standard_formats_version": "2.0.0", + "title": "AdCP Schema Registry v1", "usage": { - "validation": "Use these schemas to validate AdCP requests and responses", "codeGeneration": "Generate client SDKs using these schemas", "documentation": "Reference schemas for API documentation", - "testing": "Validate test fixtures and examples" + "testing": "Validate test fixtures and examples", + "validation": "Use these schemas to validate AdCP requests and responses" }, - "examples": [ - { - "language": "javascript", - "description": "JavaScript validation example", - "code": "const Ajv = require('ajv'); const ajv = new Ajv(); const schema = require('./schemas/v1/core/product.json'); const validate = ajv.compile(schema);" - }, - { - "language": "python", - "description": "Python validation example", - "code": "import jsonschema; schema = {...}; jsonschema.validate(data, schema)" - }, - { - "language": "java", - "description": "Java validation example", - "code": "// Use everit-org/json-schema or similar library" - } - ] + "version": "1.0.0", + "versioning": { + "note": "AdCP uses path-based versioning. The schema URL path (/schemas/v1/) indicates the version. Individual request/response schemas do NOT include adcp_version fields. Compatibility follows semantic versioning rules." + } } \ No newline at end of file diff --git a/schemas/cache/1.0.0/product.json b/schemas/cache/1.0.0/product.json index d569b5bb..9f762d9f 100644 --- a/schemas/cache/1.0.0/product.json +++ b/schemas/cache/1.0.0/product.json @@ -126,94 +126,7 @@ "publisher_properties": { "description": "Publisher properties covered by this product. Buyers fetch actual property definitions from each publisher's adagents.json and validate agent authorization. Selection patterns mirror the authorization patterns in adagents.json for consistency.", "items": { - "discriminator": { - "propertyName": "selection_type" - }, - "oneOf": [ - { - "additionalProperties": false, - "description": "Select all properties from the publisher domain", - "properties": { - "publisher_domain": { - "description": "Domain where publisher's adagents.json is hosted (e.g., 'cnn.com')", - "pattern": "^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$", - "type": "string" - }, - "selection_type": { - "const": "all", - "description": "Discriminator indicating all properties from this publisher are included", - "type": "string" - } - }, - "required": [ - "publisher_domain", - "selection_type" - ], - "type": "object" - }, - { - "additionalProperties": false, - "description": "Select specific properties by ID", - "properties": { - "property_ids": { - "description": "Specific property IDs from the publisher's adagents.json", - "items": { - "pattern": "^[a-z0-9_]+$", - "type": "string" - }, - "minItems": 1, - "type": "array" - }, - "publisher_domain": { - "description": "Domain where publisher's adagents.json is hosted (e.g., 'cnn.com')", - "pattern": "^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$", - "type": "string" - }, - "selection_type": { - "const": "by_id", - "description": "Discriminator indicating selection by specific property IDs", - "type": "string" - } - }, - "required": [ - "publisher_domain", - "selection_type", - "property_ids" - ], - "type": "object" - }, - { - "additionalProperties": false, - "description": "Select properties by tag membership", - "properties": { - "property_tags": { - "description": "Property tags from the publisher's adagents.json. Product covers all properties with these tags", - "items": { - "pattern": "^[a-z0-9_]+$", - "type": "string" - }, - "minItems": 1, - "type": "array" - }, - "publisher_domain": { - "description": "Domain where publisher's adagents.json is hosted (e.g., 'cnn.com')", - "pattern": "^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$", - "type": "string" - }, - "selection_type": { - "const": "by_tag", - "description": "Discriminator indicating selection by property tags", - "type": "string" - } - }, - "required": [ - "publisher_domain", - "selection_type", - "property_tags" - ], - "type": "object" - } - ] + "$ref": "/schemas/v1/core/publisher-property-selector.json" }, "minItems": 1, "type": "array" diff --git a/schemas/cache/1.0.0/publisher-property-selector.json b/schemas/cache/1.0.0/publisher-property-selector.json new file mode 100644 index 00000000..228e6b8d --- /dev/null +++ b/schemas/cache/1.0.0/publisher-property-selector.json @@ -0,0 +1,94 @@ +{ + "$id": "/schemas/v1/core/publisher-property-selector.json", + "$schema": "http://json-schema.org/draft-07/schema#", + "description": "Selects properties from a publisher's adagents.json. Used for both product definitions and agent authorization. Supports three selection patterns: all properties, specific IDs, or by tags.", + "discriminator": { + "propertyName": "selection_type" + }, + "oneOf": [ + { + "additionalProperties": false, + "description": "Select all properties from the publisher domain", + "properties": { + "publisher_domain": { + "description": "Domain where publisher's adagents.json is hosted (e.g., 'cnn.com')", + "pattern": "^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$", + "type": "string" + }, + "selection_type": { + "const": "all", + "description": "Discriminator indicating all properties from this publisher are included", + "type": "string" + } + }, + "required": [ + "publisher_domain", + "selection_type" + ], + "type": "object" + }, + { + "additionalProperties": false, + "description": "Select specific properties by ID", + "properties": { + "property_ids": { + "description": "Specific property IDs from the publisher's adagents.json", + "items": { + "pattern": "^[a-z0-9_]+$", + "type": "string" + }, + "minItems": 1, + "type": "array" + }, + "publisher_domain": { + "description": "Domain where publisher's adagents.json is hosted (e.g., 'cnn.com')", + "pattern": "^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$", + "type": "string" + }, + "selection_type": { + "const": "by_id", + "description": "Discriminator indicating selection by specific property IDs", + "type": "string" + } + }, + "required": [ + "publisher_domain", + "selection_type", + "property_ids" + ], + "type": "object" + }, + { + "additionalProperties": false, + "description": "Select properties by tag membership", + "properties": { + "property_tags": { + "description": "Property tags from the publisher's adagents.json. Selector covers all properties with these tags", + "items": { + "pattern": "^[a-z0-9_]+$", + "type": "string" + }, + "minItems": 1, + "type": "array" + }, + "publisher_domain": { + "description": "Domain where publisher's adagents.json is hosted (e.g., 'cnn.com')", + "pattern": "^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$", + "type": "string" + }, + "selection_type": { + "const": "by_tag", + "description": "Discriminator indicating selection by property tags", + "type": "string" + } + }, + "required": [ + "publisher_domain", + "selection_type", + "property_tags" + ], + "type": "object" + } + ], + "title": "Publisher Property Selector" +} \ No newline at end of file diff --git a/scripts/consolidate_exports.py b/scripts/consolidate_exports.py index bb892a95..755de5c1 100644 --- a/scripts/consolidate_exports.py +++ b/scripts/consolidate_exports.py @@ -62,9 +62,6 @@ def generate_consolidated_exports() -> str: # We need BOTH versions of these types available, so import them with qualified names KNOWN_COLLISIONS = { "Package": {"package", "create_media_buy_response"}, - "PublisherProperties": {"product", "adagents"}, - "PublisherProperties4": {"product", "adagents"}, - "PublisherProperties5": {"product", "adagents"}, } special_imports = [] diff --git a/src/adcp/types/_generated.py b/src/adcp/types/_generated.py index 3610740f..f2271134 100644 --- a/src/adcp/types/_generated.py +++ b/src/adcp/types/_generated.py @@ -10,7 +10,7 @@ DO NOT EDIT MANUALLY. Generated from: https://github.com/adcontextprotocol/adcp/tree/main/schemas -Generation date: 2025-11-19 01:40:18 UTC +Generation date: 2025-11-19 02:03:09 UTC """ # ruff: noqa: E501, I001 from __future__ import annotations @@ -19,7 +19,7 @@ from adcp.types.generated_poc.activate_signal_request import ActivateSignalRequest from adcp.types.generated_poc.activate_signal_response import ActivateSignalResponse, ActivateSignalResponse1, ActivateSignalResponse2 from adcp.types.generated_poc.activation_key import ActivationKey1, ActivationKey2 -from adcp.types.generated_poc.adagents import AuthorizedAgents, AuthorizedAgents1, AuthorizedAgents2, AuthorizedAgents3, AuthorizedSalesAgents, Contact, PropertyId, PropertyTag, PublisherProperties1, Tags +from adcp.types.generated_poc.adagents import AuthorizedAgents, AuthorizedAgents1, AuthorizedAgents2, AuthorizedAgents3, AuthorizedSalesAgents, Contact, PropertyId, PropertyTag, Tags from adcp.types.generated_poc.asset_type import AssetTypeSchema, ContentLength, Dimensions, Duration, FileSize, Quality, Requirements, Type from adcp.types.generated_poc.audio_asset import AudioAsset from adcp.types.generated_poc.brand_manifest import Asset, AssetType, BrandManifest, Colors, Disclaimer, FeedFormat, Fonts, Logo, Metadata, ProductCatalog, UpdateFrequency @@ -88,6 +88,7 @@ from adcp.types.generated_poc.provide_performance_feedback_request import ProvidePerformanceFeedbackRequest from adcp.types.generated_poc.provide_performance_feedback_response import ProvidePerformanceFeedbackResponse, ProvidePerformanceFeedbackResponse1, ProvidePerformanceFeedbackResponse2 from adcp.types.generated_poc.publisher_identifier_types import PublisherIdentifierTypes +from adcp.types.generated_poc.publisher_property_selector import PublisherPropertySelector1, PublisherPropertySelector2, PublisherPropertySelector3 from adcp.types.generated_poc.push_notification_config import Authentication, PushNotificationConfig, Scheme from adcp.types.generated_poc.reporting_capabilities import AvailableMetric, AvailableReportingFrequency, ReportingCapabilities from adcp.types.generated_poc.response import ProtocolResponse @@ -116,10 +117,6 @@ # Special imports for name collisions (qualified names for types defined in multiple modules) from adcp.types.generated_poc.create_media_buy_response import Package as _PackageFromCreateMediaBuyResponse from adcp.types.generated_poc.package import Package as _PackageFromPackage -from adcp.types.generated_poc.adagents import PublisherProperties as _PublisherPropertiesFromAdagents -from adcp.types.generated_poc.product import PublisherProperties as _PublisherPropertiesFromProduct -from adcp.types.generated_poc.product import PublisherProperties4 as _PublisherProperties4FromProduct -from adcp.types.generated_poc.product import PublisherProperties5 as _PublisherProperties5FromProduct # Backward compatibility aliases for renamed types Channels = AdvertisingChannels @@ -165,8 +162,9 @@ "PropertyType", "ProtocolEnvelope", "ProtocolResponse", "ProvidePerformanceFeedbackRequest", "ProvidePerformanceFeedbackResponse", "ProvidePerformanceFeedbackResponse1", "ProvidePerformanceFeedbackResponse2", "PublisherDomain", "PublisherIdentifierTypes", - "PublisherProperties1", "PushNotificationConfig", "Quality", "QuartileData", "QuerySummary", - "Render", "ReportingCapabilities", "ReportingFrequency", "ReportingPeriod", "ReportingWebhook", + "PublisherPropertySelector1", "PublisherPropertySelector2", "PublisherPropertySelector3", + "PushNotificationConfig", "Quality", "QuartileData", "QuerySummary", "Render", + "ReportingCapabilities", "ReportingFrequency", "ReportingPeriod", "ReportingWebhook", "Request", "RequestedMetric", "Requirements", "Response", "Response1", "ResponseType", "Responsive", "Results", "Results1", "Scheme", "Security", "Signal", "SignalType", "Sort", "SortApplied", "StandardFormatIds", "Status", "StatusFilter", "StatusFilterEnum", @@ -179,7 +177,5 @@ "UrlType", "ValidationMode", "VastAsset1", "VastAsset2", "VastVersion", "VcpmAuctionPricingOption", "VcpmFixedRatePricingOption", "VenueBreakdownItem", "VideoAsset", "ViewThreshold", "ViewThreshold1", "WebhookAsset", "WebhookPayload", - "_PackageFromCreateMediaBuyResponse", "_PackageFromPackage", - "_PublisherProperties4FromProduct", "_PublisherProperties5FromProduct", - "_PublisherPropertiesFromAdagents", "_PublisherPropertiesFromProduct" + "_PackageFromCreateMediaBuyResponse", "_PackageFromPackage" ] diff --git a/src/adcp/types/aliases.py b/src/adcp/types/aliases.py index 5a433dfc..437221f0 100644 --- a/src/adcp/types/aliases.py +++ b/src/adcp/types/aliases.py @@ -85,23 +85,23 @@ VastAsset1, VastAsset2, ) - -# Import all generated types that need semantic aliases from adcp.types._generated import ( - # Package types (from name collision resolution) - _PackageFromCreateMediaBuyResponse as CreatedPackageInternal, + PublisherPropertySelector1 as PublisherPropertiesInternal, ) from adcp.types._generated import ( - _PackageFromPackage as FullPackageInternal, + PublisherPropertySelector2 as PublisherPropertiesByIdInternal, ) from adcp.types._generated import ( - _PublisherProperties4FromProduct as PublisherPropertiesByIdInternal, + PublisherPropertySelector3 as PublisherPropertiesByTagInternal, ) + +# Import all generated types that need semantic aliases from adcp.types._generated import ( - _PublisherProperties5FromProduct as PublisherPropertiesByTagInternal, + # Package types (from name collision resolution) + _PackageFromCreateMediaBuyResponse as CreatedPackageInternal, ) from adcp.types._generated import ( - _PublisherPropertiesFromProduct as PublisherPropertiesInternal, + _PackageFromPackage as FullPackageInternal, ) # ============================================================================ diff --git a/src/adcp/types/generated_poc/adagents.py b/src/adcp/types/generated_poc/adagents.py index b0549acc..6833985b 100644 --- a/src/adcp/types/generated_poc/adagents.py +++ b/src/adcp/types/generated_poc/adagents.py @@ -1,6 +1,6 @@ # generated by datamodel-codegen: # filename: adagents.json -# timestamp: 2025-11-18T03:35:10+00:00 +# timestamp: 2025-11-19T02:02:39+00:00 from __future__ import annotations @@ -9,7 +9,7 @@ from adcp.types.base import AdCPBaseModel from pydantic import AnyUrl, AwareDatetime, ConfigDict, EmailStr, Field, RootModel -from . import property +from . import property, publisher_property_selector class PropertyId(RootModel[str]): @@ -72,81 +72,6 @@ class AuthorizedAgents1(AdCPBaseModel): url: Annotated[AnyUrl, Field(description="The authorized agent's API endpoint URL")] -class PublisherProperties(AdCPBaseModel): - model_config = ConfigDict( - extra='forbid', - ) - property_ids: Annotated[ - list[PropertyId], - Field( - description="Specific property IDs from the publisher's adagents.json properties array", - min_length=1, - ), - ] - publisher_domain: Annotated[ - str, - Field( - description="Domain where the publisher's adagents.json is hosted (e.g., 'cnn.com')", - pattern='^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$', - ), - ] - selection_type: Annotated[ - Literal['by_id'], - Field(description='Discriminator indicating selection by specific property IDs'), - ] - - -class PublisherProperties1(AdCPBaseModel): - model_config = ConfigDict( - extra='forbid', - ) - property_tags: Annotated[ - list[PropertyTag], - Field( - description="Property tags from the publisher's adagents.json tags. Agent is authorized for all properties with these tags", - min_length=1, - ), - ] - publisher_domain: Annotated[ - str, - Field( - description="Domain where the publisher's adagents.json is hosted (e.g., 'cnn.com')", - pattern='^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$', - ), - ] - selection_type: Annotated[ - Literal['by_tag'], Field(description='Discriminator indicating selection by property tags') - ] - - -class AuthorizedAgents3(AdCPBaseModel): - model_config = ConfigDict( - extra='forbid', - ) - authorization_type: Annotated[ - Literal['publisher_properties'], - Field( - description='Discriminator indicating authorization for properties from other publisher domains' - ), - ] - authorized_for: Annotated[ - str, - Field( - description='Human-readable description of what this agent is authorized to sell', - max_length=500, - min_length=1, - ), - ] - publisher_properties: Annotated[ - list[PublisherProperties | PublisherProperties1], - Field( - description='Properties from other publisher domains this agent is authorized for. Each entry specifies a publisher domain and which of their properties this agent can sell', - min_length=1, - ), - ] - url: Annotated[AnyUrl, Field(description="The authorized agent's API endpoint URL")] - - class Contact(AdCPBaseModel): model_config = ConfigDict( extra='forbid', @@ -226,6 +151,38 @@ class AuthorizedAgents2(AdCPBaseModel): url: Annotated[AnyUrl, Field(description="The authorized agent's API endpoint URL")] +class AuthorizedAgents3(AdCPBaseModel): + model_config = ConfigDict( + extra='forbid', + ) + authorization_type: Annotated[ + Literal['publisher_properties'], + Field( + description='Discriminator indicating authorization for properties from other publisher domains' + ), + ] + authorized_for: Annotated[ + str, + Field( + description='Human-readable description of what this agent is authorized to sell', + max_length=500, + min_length=1, + ), + ] + publisher_properties: Annotated[ + list[ + publisher_property_selector.PublisherPropertySelector1 + | publisher_property_selector.PublisherPropertySelector2 + | publisher_property_selector.PublisherPropertySelector3 + ], + Field( + description='Properties from other publisher domains this agent is authorized for. Each entry specifies a publisher domain and which of their properties this agent can sell', + min_length=1, + ), + ] + url: Annotated[AnyUrl, Field(description="The authorized agent's API endpoint URL")] + + class AuthorizedSalesAgents(AdCPBaseModel): model_config = ConfigDict( extra='forbid', diff --git a/src/adcp/types/generated_poc/product.py b/src/adcp/types/generated_poc/product.py index 474e6b03..35145efc 100644 --- a/src/adcp/types/generated_poc/product.py +++ b/src/adcp/types/generated_poc/product.py @@ -1,13 +1,13 @@ # generated by datamodel-codegen: # filename: product.json -# timestamp: 2025-11-18T05:05:53+00:00 +# timestamp: 2025-11-19T02:02:39+00:00 from __future__ import annotations -from typing import Annotated, Any, Literal +from typing import Annotated, Any from adcp.types.base import AdCPBaseModel -from pydantic import AwareDatetime, ConfigDict, Field, RootModel +from pydantic import AwareDatetime, ConfigDict, Field from . import cpc_option, cpcv_option, cpm_auction_option, cpm_fixed_option, cpp_option, cpv_option from . import creative_policy as creative_policy_1 @@ -15,7 +15,7 @@ from . import flat_rate_option from . import format_id as format_id_1 from . import measurement as measurement_1 -from . import placement +from . import placement, publisher_property_selector from . import reporting_capabilities as reporting_capabilities_1 from . import vcpm_auction_option, vcpm_fixed_option @@ -69,77 +69,6 @@ class ProductCardDetailed(AdCPBaseModel): ] -class PublisherProperties(AdCPBaseModel): - model_config = ConfigDict( - extra='forbid', - ) - publisher_domain: Annotated[ - str, - Field( - description="Domain where publisher's adagents.json is hosted (e.g., 'cnn.com')", - pattern='^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$', - ), - ] - selection_type: Annotated[ - Literal['all'], - Field( - description='Discriminator indicating all properties from this publisher are included' - ), - ] - - -class PropertyId(RootModel[str]): - root: Annotated[str, Field(pattern='^[a-z0-9_]+$')] - - -class PublisherProperties4(AdCPBaseModel): - model_config = ConfigDict( - extra='forbid', - ) - property_ids: Annotated[ - list[PropertyId], - Field(description="Specific property IDs from the publisher's adagents.json", min_length=1), - ] - publisher_domain: Annotated[ - str, - Field( - description="Domain where publisher's adagents.json is hosted (e.g., 'cnn.com')", - pattern='^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$', - ), - ] - selection_type: Annotated[ - Literal['by_id'], - Field(description='Discriminator indicating selection by specific property IDs'), - ] - - -class PropertyTag(PropertyId): - pass - - -class PublisherProperties5(AdCPBaseModel): - model_config = ConfigDict( - extra='forbid', - ) - property_tags: Annotated[ - list[PropertyTag], - Field( - description="Property tags from the publisher's adagents.json. Product covers all properties with these tags", - min_length=1, - ), - ] - publisher_domain: Annotated[ - str, - Field( - description="Domain where publisher's adagents.json is hosted (e.g., 'cnn.com')", - pattern='^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$', - ), - ] - selection_type: Annotated[ - Literal['by_tag'], Field(description='Discriminator indicating selection by property tags') - ] - - class Product(AdCPBaseModel): model_config = ConfigDict( extra='forbid', @@ -212,7 +141,11 @@ class Product(AdCPBaseModel): ] = None product_id: Annotated[str, Field(description='Unique identifier for the product')] publisher_properties: Annotated[ - list[PublisherProperties | PublisherProperties4 | PublisherProperties5], + list[ + publisher_property_selector.PublisherPropertySelector1 + | publisher_property_selector.PublisherPropertySelector2 + | publisher_property_selector.PublisherPropertySelector3 + ], Field( description="Publisher properties covered by this product. Buyers fetch actual property definitions from each publisher's adagents.json and validate agent authorization. Selection patterns mirror the authorization patterns in adagents.json for consistency.", min_length=1, diff --git a/src/adcp/types/generated_poc/publisher_property_selector.py b/src/adcp/types/generated_poc/publisher_property_selector.py new file mode 100644 index 00000000..798d964f --- /dev/null +++ b/src/adcp/types/generated_poc/publisher_property_selector.py @@ -0,0 +1,81 @@ +# generated by datamodel-codegen: +# filename: publisher-property-selector.json +# timestamp: 2025-11-19T02:02:39+00:00 + +from __future__ import annotations + +from typing import Annotated, Literal + +from adcp.types.base import AdCPBaseModel +from pydantic import ConfigDict, Field, RootModel + + +class PublisherPropertySelector1(AdCPBaseModel): + model_config = ConfigDict( + extra='forbid', + ) + publisher_domain: Annotated[ + str, + Field( + description="Domain where publisher's adagents.json is hosted (e.g., 'cnn.com')", + pattern='^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$', + ), + ] + selection_type: Annotated[ + Literal['all'], + Field( + description='Discriminator indicating all properties from this publisher are included' + ), + ] + + +class PropertyId(RootModel[str]): + root: Annotated[str, Field(pattern='^[a-z0-9_]+$')] + + +class PublisherPropertySelector2(AdCPBaseModel): + model_config = ConfigDict( + extra='forbid', + ) + property_ids: Annotated[ + list[PropertyId], + Field(description="Specific property IDs from the publisher's adagents.json", min_length=1), + ] + publisher_domain: Annotated[ + str, + Field( + description="Domain where publisher's adagents.json is hosted (e.g., 'cnn.com')", + pattern='^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$', + ), + ] + selection_type: Annotated[ + Literal['by_id'], + Field(description='Discriminator indicating selection by specific property IDs'), + ] + + +class PropertyTag(PropertyId): + pass + + +class PublisherPropertySelector3(AdCPBaseModel): + model_config = ConfigDict( + extra='forbid', + ) + property_tags: Annotated[ + list[PropertyTag], + Field( + description="Property tags from the publisher's adagents.json. Selector covers all properties with these tags", + min_length=1, + ), + ] + publisher_domain: Annotated[ + str, + Field( + description="Domain where publisher's adagents.json is hosted (e.g., 'cnn.com')", + pattern='^[a-z0-9]([a-z0-9-]*[a-z0-9])?(\\.[a-z0-9]([a-z0-9-]*[a-z0-9])?)*$', + ), + ] + selection_type: Annotated[ + Literal['by_tag'], Field(description='Discriminator indicating selection by property tags') + ] diff --git a/tests/test_discriminated_unions.py b/tests/test_discriminated_unions.py index 4a65e9e7..834cc6fd 100644 --- a/tests/test_discriminated_unions.py +++ b/tests/test_discriminated_unions.py @@ -33,13 +33,13 @@ Deployment2, # Agent Destination1, # Platform Destination2, # Agent - _PublisherProperties4FromProduct, # selection_type='by_id' (qualified name) - _PublisherProperties5FromProduct, # selection_type='by_tag' (qualified name) + PublisherPropertySelector2, # selection_type='by_id' (shared schema) + PublisherPropertySelector3, # selection_type='by_tag' (shared schema) ) -# Use qualified names for local aliases in this test -PublisherProperties4 = _PublisherProperties4FromProduct -PublisherProperties5 = _PublisherProperties5FromProduct +# Use shorter names for local aliases in this test +PublisherProperties4 = PublisherPropertySelector2 +PublisherProperties5 = PublisherPropertySelector3 class TestAuthorizationDiscriminatedUnions: diff --git a/tests/test_type_aliases.py b/tests/test_type_aliases.py index f2666b01..044244c1 100644 --- a/tests/test_type_aliases.py +++ b/tests/test_type_aliases.py @@ -435,15 +435,15 @@ def test_publisher_properties_aliases_point_to_correct_types(): """Test that PublisherProperties aliases point to the correct generated types.""" from adcp import PublisherPropertiesAll, PublisherPropertiesById, PublisherPropertiesByTag from adcp.types._generated import ( - _PublisherProperties4FromProduct, - _PublisherProperties5FromProduct, - _PublisherPropertiesFromProduct, + PublisherPropertySelector1, + PublisherPropertySelector2, + PublisherPropertySelector3, ) - # Verify aliases point to correct types (from product module, not adagents) - assert PublisherPropertiesAll is _PublisherPropertiesFromProduct - assert PublisherPropertiesById is _PublisherProperties4FromProduct - assert PublisherPropertiesByTag is _PublisherProperties5FromProduct + # Verify aliases point to correct types (from shared publisher_property_selector module) + assert PublisherPropertiesAll is PublisherPropertySelector1 + assert PublisherPropertiesById is PublisherPropertySelector2 + assert PublisherPropertiesByTag is PublisherPropertySelector3 # Verify they're different types assert PublisherPropertiesAll is not PublisherPropertiesById