fix: apply selection_type inference to inventory profile publisher_properties#1174
Conversation
…n inventory profile path TDD red phase: 6 unit tests proving Product.effective_properties returns raw inventory profile publisher_properties without selection_type inference. 5 tests fail (KeyError: selection_type), 1 passes (passthrough when selection_type already present). Also adds InventoryProfileFactory.
…ction_type inference 6 scenarios covering product discovery when inventory profile publisher_properties lack the selection_type discriminator: - by_id inference from property_ids - by_tag inference from property_tags - all inference when no IDs/tags - passthrough when selection_type present - fallback to all when property_ids invalid - legacy field stripping
Add call_a2a, call_mcp, build_rest_body, parse_rest_response, and REST_ENDPOINT to ProductEnv. Override call_impl with sync wrapper (asyncio.run) so ImplDispatcher works with the async ProductMixin. Enables multi-transport BDD dispatch for product discovery scenarios.
…enarios 6 scenarios x 4 transports = 24 test cases. 20 fail proving the bug (Unable to extract tag using discriminator 'selection_type'), 4 pass (passthrough when selection_type already present). Steps use ProductEnv harness — real DB, real _get_products_impl, real convert_product_model_to_schema, all 4 transports (IMPL/A2A/MCP/REST).
…operties (prebid#1162) Extract ensure_selection_type() helper and apply it to both code paths in Product.effective_properties. Inventory profile publisher_properties that lack the selection_type discriminator now get it inferred: - valid property_ids -> "by_id" - valid property_tags -> "by_tag" - neither -> "all" Legacy extra fields (property_name, property_type, identifiers) are stripped. Invalid property_ids (e.g. domain-style "weather.com") fall back to "all". BDD: 24/24 pass (6 scenarios x 4 transports). Unit: 6 regression tests pass.
tests/harness/product.py
Outdated
| try: | ||
| asyncio.get_running_loop() | ||
| # Already in async context — return the coroutine for the caller to await | ||
| return coro # type: ignore[return-value] | ||
| except RuntimeError: | ||
| # No running loop — bridge to sync | ||
| return asyncio.run(coro) |
There was a problem hiding this comment.
See here and below there is a sync/async bridge. This is a new pattern when compared to the others. Not 100% sure the benefit or difference between having a sync/async bridge and not though? Just noting for project consistency
There was a problem hiding this comment.
Good catch — aligned to the AccountSyncEnv pattern: explicit call_impl() (sync via asyncio.run) + call_impl_async() (native async). Same for call_a2a/call_a2a_async. The get_running_loop() auto-detection was clever but inconsistent with the rest of the harness layer.
tests/bdd/conftest.py
Outdated
| request.getfixturevalue("integration_db") | ||
| from tests.harness.product import ProductEnv | ||
|
|
||
| with ProductEnv(tenant_id="inv-profile-test") as env: |
There was a problem hiding this comment.
Hardcoded tenant_id
UC-004 and UC-005 both use ProductEnv() / DeliveryPollEnv() / CreativeFormatsEnv() with no explicit tenant_id. They rely on the default "test_tenant" from the base class.
There was a problem hiding this comment.
Fixed — removed the explicit tenant_id, now uses the default "test_tenant" from BaseTestEnv like all other harnesses. No isolation requirement justified the custom value.
| def given_tenant(ctx: dict) -> None: | ||
| """Create a tenant with required config for get_products.""" | ||
| tenant = TenantFactory( | ||
| tenant_id="inv-profile-test", |
There was a problem hiding this comment.
Hardcoded tenant_id
UC-004 and UC-005 both use ProductEnv() / DeliveryPollEnv() / CreativeFormatsEnv() with no explicit tenant_id. They rely on the default "test_tenant" from the base class.
There was a problem hiding this comment.
Fixed — updated the TenantFactory call to use "test_tenant" to match the harness default.
| def _make_product_with_profile(publisher_properties: list[dict]) -> MagicMock: | ||
| """Build a mock Product whose effective_properties calls the real property logic. | ||
|
|
||
| Uses a MagicMock with the real Product.effective_properties descriptor | ||
| to test the actual inference logic without SQLAlchemy instrumentation. | ||
| """ | ||
| profile = MagicMock() | ||
| profile.publisher_properties = publisher_properties | ||
|
|
||
| product = MagicMock(spec=Product) | ||
| product.inventory_profile_id = 1 | ||
| product.inventory_profile = profile | ||
| product.properties = None | ||
| product.property_ids = None | ||
| product.property_tags = None | ||
| product.tenant = None | ||
|
|
||
| # Wire the real property descriptor so we test actual production code | ||
| type(product).effective_properties = PropertyMock(side_effect=lambda: Product.effective_properties.fget(product)) | ||
| return product |
There was a problem hiding this comment.
This PropertyMock(side_effect=lambda: Product.effective_properties.fget(product)) is unique in testing of the project. Don't know if we want to make an issue to expand this pattern across other testing in the project?
There was a problem hiding this comment.
This is the standard Python idiom for mocking `@property` — you can't override a property via instance attribute assignment (no setter), so `PropertyMock` on the class is the only option. It's the sole usage in the codebase, and I don't think it needs expanding — the integration tests for `effective_properties` use a real DB with real `InventoryProfile` relationships, which is the right approach for testing the property's own logic. This unit test just needs a surgical override to reach the error path in `convert_product_model_to_schema`.
…ference-inventory-profile # Conflicts: # tests/bdd/conftest.py
- ProductEnv: replace get_running_loop() auto-detection with explicit call_impl/call_impl_async + call_a2a/call_a2a_async pairs, matching AccountSyncEnv pattern for project consistency - Remove hardcoded "inv-profile-test" tenant_id from conftest.py and step definitions — use default "test_tenant" like all other harnesses
2 participants
Problem
`get_products` fails with a `VALIDATION_ERROR` when a product references an inventory profile whose `publisher_properties` JSON lacks the `selection_type` discriminator required by AdCP 2.13.0+:
```
Unable to extract tag using discriminator 'selection_type'
[type=union_tag_not_found, input_value={'property_ids': ['weather.com'], ...}]
```
Any inventory profile created via the admin UI's "full JSON" mode stores arbitrary JSON with no validation, so legacy data (missing `selection_type`, containing domain-style `property_ids`, extra fields like `property_name`) passes through to Pydantic and crashes.
Closes #1162
Root Cause
`Product.effective_properties` has two code paths:
Path 1 skipped all inference. The AdCP `PublisherPropertySelector` discriminated union requires `selection_type` to be present for Pydantic to parse it.
Solution
Extract the inference logic into a shared helper `ensure_selection_type()` and apply it to both code paths (DRY). The function is non-destructive — it adds `selection_type` when missing but preserves all other fields on the dict:
No database migration needed — normalization happens on read.
Testing Approach
BDD-first. Gherkin scenarios define the expected behavior, step definitions exercise real production code through the `ProductEnv` harness (real DB, real `_get_products_impl`, real `convert_product_model_to_schema`). Every scenario runs through all 4 transports (IMPL, A2A, MCP, REST) — 6 scenarios × 4 transports = 24 BDD test cases.
Scenarios cover:
Test results (all green, merged with latest main)