Skip to content

fix(api-sync): make --apply idempotent per (class, field/enum value) #39

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
ericviana merged 1 commit into from
Aug 4, 2026
Merged

fix(api-sync): make --apply idempotent per (class, field/enum value) #39

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions scripts/api-sync.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,6 +725,14 @@ function reconcileEnums(array $map, array $newSpec, array $reachable, array $cla
{
$applicable = [];
$needsHuman = [];
// Same idempotency concern as reconcileTypes()' $scheduledInsertions: more than one
// spec-map.json enums entry can legitimately point at the same SDK enum class (e.g. the same
// LimitIncreaseRequestSupportingDocumentType class is mapped from both
// CustomerLimitIncreaseIn.supporting_document_type and
// GetCustomerLimitIncreaseOut.supporting_document_type). If a new spec value is missing from
// that class, both entries independently report it -- dedupe per (class, value) so --apply
// inserts the case at most once per run.
$scheduledCases = [];

foreach ($map['enums'] as $entry) {
$s = $entry['spec'];
Expand Down Expand Up @@ -752,6 +760,11 @@ function reconcileEnums(array $map, array $newSpec, array $reachable, array $cla
if (isset($divergenceEnumIndex["{$className}|{$value}"])) {
continue; // recorded known divergence, not pending drift
}
$caseKey = "{$className}|{$value}";
if (isset($scheduledCases[$caseKey])) {
continue; // same class+value already scheduled by an earlier enums entry this run
}
$scheduledCases[$caseKey] = true;
$applicable[] = [
'kind' => 'enum-member-added',
'enum' => $className,
Expand Down Expand Up @@ -960,6 +973,16 @@ function reconcileTypes(array $map, array $newSpec, array $reachable, array $cla
{
$applicable = [];
$needsHuman = [];
// Insertion is per (class, field), not per (schema, class, field): a multi-schema entry
// (e.g. spec: ["PayinOut", "CreatePayinOut"]) or two separate entries mapping the same SDK
// class (e.g. TrackingTransaction reused across the payin and payout tracking_transaction
// paths) can independently find the SAME missing field on the SAME class -- once per schema
// that lacks it. Without this dedupe, --apply would insert that promoted property/fromArray
// line/toArray line more than once into one file in a single run, corrupting the source
// (duplicate constructor parameters). This index makes insertion idempotent per run: the
// first schema to report a given (class, field) as missing wins, later reports of the exact
// same gap are dropped as already-scheduled, not re-emitted.
$scheduledInsertions = [];

foreach ($map['types'] as $entry) {
$schemas = typeSpecSchemas($entry);
Expand Down Expand Up @@ -1045,6 +1068,11 @@ function reconcileTypes(array $map, array $newSpec, array $reachable, array $cla
// is unambiguous; for a fan-out with multiple sdk sites we apply to ALL of them, since
// any of them might legitimately accept/return the new field.
foreach ($entry['sdk'] as $site) {
$insertionKey = "{$site['class']}|{$field}";
if (isset($scheduledInsertions[$insertionKey])) {
continue; // same class+field already scheduled by an earlier schema/entry this run
}
$scheduledInsertions[$insertionKey] = true;
$applicable[] = [
'kind' => 'field-added',
'schema' => $schemaName,
Expand Down
136 changes: 136 additions & 0 deletions tests/ApiSync/ApiSyncTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -396,6 +396,142 @@ public function applying_a_field_twice_only_inserts_it_once(): void
$this->assertSame(3, substr_count($source, 'description'));
}

#[Test]
public function a_multi_schema_entry_missing_the_same_field_on_both_schemas_is_only_scheduled_once(): void
{
// Reproduces the exact double-apply bug: a single spec-map.json entry with TWO schemas
// (e.g. spec: ["PayinOut", "CreatePayinOut"]) sharing one SDK class. If both schemas are
// independently missing the same field, reconcileTypes() used to emit one 'field-added'
// per schema -- two entries for the same (class, field) -- and --apply inserted the
// promoted property/fromArray line/toArray line twice into one file in a single run.
$map = $this->widgetMap();
$map['types'][0]['spec'] = ['WidgetOut', 'WidgetOutAlias'];

$classIndex = scanAllClasses($this->fixtureRoot);
$newSpec = $this->baseSpec([
'components' => ['schemas' => [
'WidgetOut' => ['properties' => ['description' => ['type' => ['string', 'null']]]],
'WidgetOutAlias' => [
'type' => 'object',
'properties' => [
'id' => ['type' => 'string'],
'name' => ['type' => 'string'],
'color' => ['type' => 'string', 'enum' => ['red', 'blue']],
'description' => ['type' => ['string', 'null']],
],
],
]],
]);
// WidgetOutAlias is only reachable if something references it -- webhooks is a
// convenient root for a schema with no operation of its own.
$newSpec['webhooks'] = ['widget.alias' => ['post' => ['requestBody' => ['content' => ['application/json' => ['schema' => ['$ref' => '#/components/schemas/WidgetOutAlias']]]]]]];
$reachable = computeReachable($newSpec);

[$applicable, $needsHuman] = reconcileTypes($map, $newSpec, $reachable, $classIndex, []);

$this->assertEmpty($needsHuman);
$this->assertCount(1, $applicable, 'the same (class, field) gap reported by two schemas in one entry must be scheduled only once');
$this->assertSame('description', $applicable[0]['field']);
$this->assertSame('WidgetResponse', $applicable[0]['class']);

foreach ($applicable as $change) {
applyFieldInsertion($this->fixtureRoot, $change['file'], $change['class'], $change['field'], $change['propSchema']);
}

$source = file_get_contents("{$this->fixtureRoot}/{$applicable[0]['file']}");
$this->assertSame(3, substr_count($source, 'description'), 'exactly one constructor param + one fromArray line + one toArray line, never two of each');

// a second --apply-equivalent pass over the now-patched source finds nothing left to do.
$classIndex = scanAllClasses($this->fixtureRoot);
[$applicableAgain, $needsHumanAgain] = reconcileTypes($map, $newSpec, $reachable, $classIndex, []);
$this->assertEmpty($applicableAgain);
$this->assertEmpty($needsHumanAgain);
}

#[Test]
public function two_separate_entries_mapping_the_same_class_missing_the_same_field_are_only_scheduled_once(): void
{
// Same bug, different trigger shape: TWO separate spec-map.json `types` entries (not one
// multi-schema entry) independently point at the same SDK class -- this is exactly how
// TrackingTransaction is reused across the payin and payout tracking_transaction paths.
$map = $this->widgetMap();
$map['types'][] = [
'spec' => 'WidgetOutAlias',
'sdk' => [['file' => 'src/Resources/Widgets/Widgets.php', 'class' => 'WidgetResponse']],
];

$classIndex = scanAllClasses($this->fixtureRoot);
$newSpec = $this->baseSpec([
'components' => ['schemas' => [
'WidgetOut' => ['properties' => ['description' => ['type' => ['string', 'null']]]],
'WidgetOutAlias' => [
'type' => 'object',
'properties' => [
'id' => ['type' => 'string'],
'name' => ['type' => 'string'],
'color' => ['type' => 'string', 'enum' => ['red', 'blue']],
'description' => ['type' => ['string', 'null']],
],
],
]],
]);
$newSpec['webhooks'] = ['widget.alias' => ['post' => ['requestBody' => ['content' => ['application/json' => ['schema' => ['$ref' => '#/components/schemas/WidgetOutAlias']]]]]]];
$reachable = computeReachable($newSpec);

[$applicable, $needsHuman] = reconcileTypes($map, $newSpec, $reachable, $classIndex, []);

$this->assertEmpty($needsHuman);
$this->assertCount(1, $applicable, 'two entries mapping the same class must not double-schedule the same missing field');

foreach ($applicable as $change) {
applyFieldInsertion($this->fixtureRoot, $change['file'], $change['class'], $change['field'], $change['propSchema']);
}

$source = file_get_contents("{$this->fixtureRoot}/{$applicable[0]['file']}");
$this->assertSame(3, substr_count($source, 'description'));
}

#[Test]
public function two_enum_entries_mapping_the_same_class_missing_the_same_value_are_only_scheduled_once(): void
{
// Mirrors the field-insertion bug on the enum side: two spec-map.json `enums` entries
// (e.g. the same LimitIncreaseRequestSupportingDocumentType class mapped from both
// CustomerLimitIncreaseIn and GetCustomerLimitIncreaseOut) independently report the same
// missing case, which used to insert it twice into the enum file in one --apply run.
$map = $this->widgetMap();
$map['enums'][] = [
'spec' => ['schema' => 'WidgetOutAlias', 'property' => 'color'],
'sdk' => ['file' => 'src/Types/WidgetColor.php', 'class' => 'WidgetColor'],
];

$classIndex = scanAllClasses($this->fixtureRoot);
$newSpec = $this->baseSpec([
'components' => ['schemas' => [
'WidgetOut' => ['properties' => ['color' => ['enum' => ['red', 'blue', 'green']]]],
'WidgetOutAlias' => [
'type' => 'object',
'properties' => [
'color' => ['type' => 'string', 'enum' => ['red', 'blue', 'green']],
],
],
]],
]);
$newSpec['webhooks'] = ['widget.alias' => ['post' => ['requestBody' => ['content' => ['application/json' => ['schema' => ['$ref' => '#/components/schemas/WidgetOutAlias']]]]]]];
$reachable = computeReachable($newSpec);

[$applicable, $needsHuman] = reconcileEnums($map, $newSpec, $reachable, $classIndex, []);

$this->assertEmpty($needsHuman);
$this->assertCount(1, $applicable, 'two enum entries reporting the same missing case on the same class must not double-schedule it');
$this->assertSame('green', $applicable[0]['value']);

$specValuesByEnum = ['WidgetColor' => ['red', 'blue', 'green']];
applyEnumCaseInsertion($this->fixtureRoot, $applicable[0]['file'], 'WidgetColor', 'green', $newSpec, $specValuesByEnum);

$source = file_get_contents("{$this->fixtureRoot}/src/Types/WidgetColor.php");
$this->assertSame(1, substr_count($source, "case GREEN = 'green';"));
}

// ---- NEEDS_HUMAN: removals ----

#[Test]
Expand Down
Loading