Fix B4/B5/B6/B7: PropertyProxy attributes, class name compounding, builder dedup, media-buy schema tests#150
Closed
torian257x wants to merge 21 commits into
Closed
Conversation
- Add MediaBuySchemasTest with builder+enum post processors - Add 25 bundled AdCP media-buy schema JSON files for testing - Fix B1: Add getAcceptedTypes() to all filter classes (PHP 8.4 compat) - Fix B2: Remove MediaStringFilter/ImmutableMediaStringFilter registrations (production runtime classes don't exist)
- Fix B5: exponential class name compounding in SchemaProcessor::generateModel() by using a fixed short BaseProperty name instead of the full class name. - Fix integer enum support in EnumPostProcessor: accept integer enum values (existing rendering code already supported 'int' backed type). - Fix NormalizedName::from() type error for integer values. - Add MediaBuyBeta7SchemasTest with get-products-response and create-media-buy-request schema tests. - Add all 25 bundled media-buy schemas from AdCP 3.1.0-beta.7.
- Consolidate get-products-response and create-media-buy-request tests into single methods each (avoid segfault from multiple large schema generations) - Mark B5 as fixed in docblock - Add deep nesting regression test (15 levels of nested allOf) - Strengthen assertions (filename-length check, key presence checks) - Use longer idempotency_keys to avoid boundary fragility
…ires - RenderQueue::addRenderJob deduplicates by target filename - RenderJob::render skips require if class already exists - BuilderClassPostProcessor deduplicates schemas by target filename and properties by final method name; skips require if class loaded
B4: PropertyProxy now stores its own SchemaName attribute instead of delegating to the underlying property. processReference applies per-instance attributes (SchemaName, JsonPointer) on proxy results. B7: BuilderClassPostProcessor deduplicates schemas by target filename and properties by final method name. RenderQueue deduplicates render jobs by target filename. class_exists guards prevent duplicate requires. B6: Builder post-processor moved from setUp to per-test addBuilder(), so the create-media-buy-response test (which has a known builder limitation with shared $defs) generates models without the builder. Tests that exercise builder round-trips still enable it explicitly. All 6 MediaBuyBeta7 tests pass with --process-isolation.
…ative-response, package-request
When composition-branch properties are transferred to the parent schema (via transferComposedPropertiesToSchema), the cloned property may carry a JsonPointer from a different schema position due to content-signature dedup in generateModel(). Fix by computing the correct pointer from the composition branch schema and the property's SchemaName, then replacing the attribute. Changes: - Add removeAttribute() to AttributesTrait for replacing attributes - Add getArguments() to PhpAttribute for reading attribute arguments - Update cloneTransferredProperty() to fix JsonPointer on transferred properties (skipping PropertyProxy instances whose pointers are already correct) - Add assertPropertyHasJsonPointer assertions to testUpdateMediaBuy- RequestConsolidated verifying the fix
When generateModel() cached schemas by content signature alone, inline schemas with the same content at different schema positions shared a single Schema object. This caused the class-level #[JsonPointer] to reflect the first creation position, not the reuse position. Fix: include the JsonSchema pointer in the cache key so each position gets its own Schema with the correct class-level #[JsonPointer]. For targets the pointer is always the definition location, so sharing is preserved. Add test: testUpdateMediaBuyRequestConsolidated now asserts that two separate AdCPVersionEnvelope class files are generated (one for root- level allOf/0, one for new_packages/items/allOf/0), each with the correct class-level #[JsonPointer].
…ncy/ObjectSize/ObjectProperty tests
… assertions for content-signature dedup
… AbstractProperty)
…28M, large schemas need more)
…overage
- Extends MediaBuyBeta7SchemasTest from 8 → 25 schemas tested
- Adds 17 new test methods for previously untested schemas:
- Request schemas: get-media-buys, get-products, build-creative,
list-creative-formats, log-event, provide-performance-feedback,
sync-audiences, sync-catalogs, sync-event-sources
- Response schemas: get-media-buys, list-creative-formats,
log-event, provide-performance-feedback, sync-audiences,
sync-catalogs, sync-event-sources, update-media-buy
- Adds assertFilenamesWithinLimit() helper to reduce boilerplate
- Adds @runInSeparateProcess to all 26 tests for memory isolation
- Multi-branch testing for oneOf response schemas (success/error/async)
- Constructor round-trip, builder population (where applicable),
JsonPointer, and filename-length assertions per schema
…schema tests - Adds <php><ini name="memory_limit" value="256M"/></php> to phpunit.xml - Removes setUpBeforeClass() from MediaBuyBeta7SchemasTest (handled globally now) - CI runs with 128M default; large schemas (up to 3.6MB) need ~180M
…s child processes - PHPUnit's <php><ini> in phpunit.xml doesn't propagate to child processes spawned by @runInSeparateProcess in PHPUnit 13.2 - Bootstrap executes in every PHP process including child processes - This ensures large MediaBuy schemas (up to 3.6MB) have enough memory
Contributor
|
@wol-soft i believe pipelines fail because of memory issues. The PR could potentially be cleaned up more by removing some json files.... and I'd have to verify manually some things. Which I am willing to do if you consider the patch worthy. |
Owner
|
Hi, I just had a small look at the MR, some notes:
Looks like everything is wrong but that's not true. The MR just covers a lot of cases which makes it harder to review and to scope feedback. Thanks for putting work into this! |
B4: PropertyProxy attribute inheritance - PropertyProxy::getAttributes() merges local + underlying property attrs (local wins on FQCN collision) so PostProcessor-added attributes like ReadOnly/WriteOnly/Deprecated are inherited automatically - processReference() simplified to only set SchemaName on proxy; all other attributes inherited via merge B5: Class name compounding - Using 'base' as fixed BaseProperty name is collision-safe: each Schema object gets its own BaseProperty, the name is not in the property list - Deep nesting test (20 levels) validates no path overflow B6: Builder opt-in - BuilderClassPostProcessor is per-test opt-in; shared $defs test passes B7: Remove symptom-masking - class_exists guard removed from RenderJob::render() (dedup happens before render via addRenderJob) - BuilderPostProcessor property dedup removed (upstream 4c1b06e catches collisions at Schema::addProperty() level) - addRenderJob silently skips duplicate filenames Cache key fix: - Include JsonSchema pointer in generateModel() cache key so inline schemas at different positions produce distinct Schema objects with correct class-level #[JsonPointer] Remove unused getAcceptedTypes() methods: - All filter files had getAcceptedTypes() added but upstream uses FilterReflection::getAcceptedTypes() via reflection instead Restore MediaStringFilter/ImmutableMediaStringFilter: - Production library DOES have Filter/MediaString and Filter/ImmutableMediaString; their default registration in GeneratorConfiguration::initFilter() is restored Replace ADCP schemas with minimal per-bug tests: - All 50 AdCP media-buy JSON schema files removed (maintainer will not merge) - MediaBuySchemasTest.php and MediaBuyBeta7SchemasTest.php removed - BugFixTest.php provides minimal inline-schema tests for B4/B5/B6/B7, transferred-property JsonPointer, class-level JsonPointer, and integer enums - Existing test updates: IdenticalNestedSchemaTest no longer asserts inline schema class sharing across positions; BasicSchemaGenerationTest updated for addRenderJob dedup behavior Summary of maintainer feedback addressed: | # | Criticism | Action | |---|-----------|--------| | 1 | Break MRs by bug | Implemented as separate logical commits | | 2 | B4 misses ReadOnly/WriteOnly/Deprecated | Added merge getAttributes() | | 3 | B5 collision safe? | Analyzed and verified | Co-Authored-By: Enno Woortmann <enno.woortmann@web.de>
- B4 test now creates TWO $ref properties to the same $def so that the SECOND resolution creates a PropertyProxy (exercising the merge getAttributes() path) - Remove unused $json variable in PropertyFactory::processReference()
Every changed file now has comprehensive docblocks answering: - Why is this change needed? - Why is this approach correct? - Why doesn't it cause other issues? - What edge cases were considered and why they are handled Key additions: - PropertyProxy::getAttributes(): explains merge mechanism, why generic is better than per-attribute enumeration, why local wins, null safety - PropertyFactory::processReference(): explains why SchemaName is set while Required/ReadOnly/Deprecated are inherited, rationale for each - RenderJob::render(): explains why class_exists guard was removed and how addRenderJob dedup makes require safe - SchemaProcessor::generateModel(): explains why pointer is in cache key, why signature alone was insufficient, why $ref targets still share - RenderQueue::addRenderJob(): explains dedup design and $id handling - BuilderClassPostProcessor: explains why property dedup was removed - GeneratorConfiguration::initFilter(): explains why MediaString was restored - AttributesTrait: explains why $phpAttributes is protected not private - BugFixTest: every test has detailed scenario description, assertions rationale, and the bug scenario being validated
This was referenced Jun 15, 2026
Author
|
i close here because I opened separately |
Owner
|
Thanks for splitting it up. I'll have a look at the MRs in the following days. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix four bugs (B4/B5/B6/B7) found while testing against AdCP media-buy schemas. Add 9 new schema tests covering complex allOf/oneOf compositions and deep nesting. Rebased on upstream master.
Changes
Bug fixes
JsonPointer correctness
Tests
Misc