TR-7946 Stop discarding domain types the generator can actually build - #135
Conversation
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
56ef9c5 to
bfe758a
Compare
|
@mSprunskas , please review. Thank you. |
dzmitry-starastsenka-paysera
changed the title
mSprunskas
left a comment
mSprunskas
left a comment
There was a problem hiding this comment.
- Changes should be reflected in tests
- See
1. High — Array item types that point to a named scalar type stay unchanged, thus generation continues to fail
File: src/Paysera/Bundle/CodeGeneratorBundle/Service/TypeDefinitionBuilder.php:117-131
Problem: The new loop changes only properties that have the type PropertyDefinition::TYPE_REFERENCE. An array property keeps its item type in ArrayPropertyDefinition::$itemsType. Its own type is array. Therefore the loop does not touch it.
DefinitionValidator.php:28-30 validates the item type:
if ($property instanceof ArrayPropertyDefinition) {
$this->validateType($property->getItemsType(), $api);
}
The named scalar type is not in $api->getTypes(). Thus validateType() throws UnrecognizedTypeException.
Verification: Output of the test script:
currency type='string' reference=NULL items=- constants=0
currencies type='array' reference=NULL items='Currency' constants=0
The property currency is corrected. The array property currencies keeps 'Currency'.
Impact: The RAML pattern items: { type: <NamedScalarType> } is common. Test fixtures use this pattern (for example tests/JavascriptGeneratorBundle/Fixtures/raml/account/api.raml:21-22). Generation still stops with Did not found defined type "Currency". The
fix does not do what the CHANGELOG promises.
Fix: Add the item type to the same loop:
if ($property instanceof ArrayPropertyDefinition && isset($scalarAliases[$property->getItemsType()])) {
$property->setItemsType($scalarAliases[$property->getItemsType()]);
}
---
2. Medium — The alias list comes from the raw RAML data, not from the types that no builder made, thus a class can be generated and bypassed at the same time
File: src/Paysera/Bundle/CodeGeneratorBundle/Service/TypeDefinitionBuilder.php:97-111
Problem: The method collects aliases from $apiTypes. It does not check whether a builder made a TypeDefinition for that name. FilterTypeDefinitionBuilder::supports() returns true for each name that contains the text Filter
(FilterTypeDefinition::BASE_FILTER = 'Filter'). Its buildTypeDefinition() does not need properties. It returns a FilterTypeDefinition with an empty property list.
Verification: Output of the test script:
mode type='string' reference=NULL
FilterTypeDefinitionBuilder->supports('FilterMode') = true
buildTypeDefinition('FilterMode') returns ...\FilterTypeDefinition
Impact: For a scalar type with a name such as FilterMode, CurrencyFilter or StatusFilter, the generator makes an empty class. At the same time all references to that class become string. The generated class is dead code. The two decisions do not agree.
Fix: Make the alias list from the names that gave no TypeDefinition. Collect the built names first, then keep only the aliases that are not in that list.
---
3. Medium — Named scalar types whose name contains "Result" stop generation before the new method starts
File: src/Paysera/Bundle/CodeGeneratorBundle/Service/TypeDefinitionBuilder.php:47-64
Problem: resolveScalarAliases() runs at line 64. The builder loop runs before it, at lines 47-60. ResultTypeBuilder has position 20 and thus runs before SimpleTypeBuilder (position 100). It supports each name that contains Result and throws when the
definition has no properties key.
Verification:
ResultTypeBuilder->supports('ScanResult', ['type' => 'string']) = true
InvalidDefinitionException: ResultType definition must contain "properties" list
Impact: A scalar type with a name such as ScanResult, ResultStatus or PaymentResultCode still stops generation. The exception occurs before the new code can help.
Fix: Add a properties test to ResultTypeBuilder::supports(), or find the aliases before the builder loop and remove them from $apiTypes.
---
4. Medium — Enumeration values are lost when an alias becomes a primitive type
File: src/Paysera/Bundle/CodeGeneratorBundle/Service/TypeDefinitionBuilder.php:125-128
Problem: The method writes the primitive type and clears the reference. It does not copy the enum values of the alias into the property. PropertyDefinitionBuilder.php:56-58 makes constants only from the enum key of the property itself.
Verification: The test script gives constants=0 for the property currency, although the alias Currency has enum: ['EUR', 'USD'].
Impact: A RAML enumeration alias is the most usual named scalar type. The generated client loses all constants and all value control. The generated code compiles, but it accepts each string.
Fix: Copy the enumeration values with ConstantBuilder when the alias has an enum key.
---
5. Medium — Neither change has a test
Files: src/Paysera/.../TypeDefinitionBuilder.php, src/Paysera/.../MetadataTypeBuilder.php
Problem: The change adds 55 lines of new logic and changes the match rule of a builder. The change list holds no test file and no RAML fixture. A search of tests/ finds no fixture with a named scalar type. It also finds no unit test for
TypeDefinitionBuilder or MetadataTypeBuilder.
Impact: The gaps in findings #1, #2, #3 and #4 are not detected. A later change can break the new logic without a signal.
Fix: Add a RAML fixture with a named scalar type. Use it as a property, as an array item and as a body type.
---
6. Medium — .phpunit.result.cache is a build product and enters the repository
File: .phpunit.result.cache
Problem: git check-ignore gives exit code 1 for this file. Thus .gitignore does not exclude it. The file has 6034 bytes and holds local test results.
Impact: The file changes with each test run. It causes noise in each merge request and can cause merge conflicts.
Fix: Add .phpunit.result.cache to .gitignore. Do not commit the file.
---
7. Medium — release.sh holds personal paths, has no interpreter line and cannot run
File: release.sh
Problems:
- Line 1 and line 5 use paths of one developer: ~/Projects/api-spec/app-mokejimai/... and ~/Projects/js-lib-paysera-clients/.... Other developers do not have these directories.
- The file has no #!/usr/bin/env bash line. Byte inspection with od -c shows that the file starts with bin/console.
- The file permission is -rw-rw-r--. Thus the file is not executable. The command ./release.sh fails.
- The file has no set -euo pipefail. If the first command fails, the second command still runs.
- The name release.sh is not correct. The script does not make a release. It generates two JavaScript client packages.
Impact: The script is a local helper file. It is not usable by other persons. It gives wrong information about its function.
Fix: Do not commit this file, or make the paths parameters, add the interpreter line, add set -e, set the execute permission and use a correct name such as generate-js-clients.sh.
---
8. Low — Bodies of requests and responses that use a named scalar type are still not accepted
File: src/Paysera/Bundle/CodeGeneratorBundle/Service/TypeDefinitionBuilder.php:117-131
Problem: DefinitionValidator::validateResource() (lines 39-70) reads the body types from the raw RAML data with $body->getType(). The new method changes only the properties of the decorated types. It does not change the raw RAML data.
Impact: A method that has a named scalar type as its body or its 200 response still causes UnrecognizedTypeException. This case is less common than the case in finding #1.
Fix: Let validateType() accept the names that are in the alias list, or resolve the aliases in the raw RAML data.
---
9. Low — MetadataTypeBuilder no longer removes names that use a separator character other than a space
File: src/Paysera/Bundle/CodeGeneratorBundle/Service/TypeDefinitionBuilder/MetadataTypeBuilder.php:21
Problem: The old code found the text metadata at any position. Thus it matched Result_Metadata, because that name contains metadata. The new code removes only the space character before the comparison.
Verification:
supports(ResultMetadata ) = TRUE (dropped)
supports(rest.ResultMetadata ) = TRUE (dropped)
supports(Result Meta Data ) = TRUE (dropped)
supports(Result_Metadata ) = false (generated)
Impact: If an API specification uses Result_Metadata or Result-Metadata, the generator now makes a class for it. The REST client runtime already supplies this class. This is a small risk, because the usual name is ResultMetadata.
Fix: Remove _ and - also, for example with preg_replace('/[\s_-]+/', '', ...).
---
10. Low — Two different classes supply the same constant values in one code block
File: src/Paysera/Bundle/CodeGeneratorBundle/Service/TypeDefinitionBuilder.php:105-107
Problem: Line 105 calls TypeHelper::isPrimitiveType(). Lines 106 and 107 use PropertyDefinition::TYPE_OBJECT and PropertyDefinition::TYPE_ARRAY. The class TypeHelper has the same constants (TypeHelper::TYPE_OBJECT, TypeHelper::TYPE_ARRAY).
Impact: The code is more difficult to read. The values are equal now, thus there is no defect.
Fix: Use TypeHelper constants at this position, because the test on line 105 uses TypeHelper.
---
11. Note — A non-ASCII character is in a source comment
File: src/Paysera/Bundle/CodeGeneratorBundle/Service/TypeDefinitionBuilder.php:85
Problem: The comment holds an em dash (U+2014). All other files in src/ use ASCII characters only.
Fix: Replace the em dash with a hyphen or a comma.
---
12. Note — The version number is a patch increment, but the generated output changes
File: CHANGELOG.md:8
Problem: The new version is 11.11.6. The last tag is 11.11.5. The change to MetadataTypeBuilder causes the generator to make new classes for each type whose name contains metadata (for example TransferMetadata, DocumentMetaData). Before this change, the
generator did not make these classes.
Impact: Users who regenerate a client get new files. Their build can change. The project states that it follows Semantic Versioning (CHANGELOG.md:5).
Fix: Use 11.12.0, or add a note in the CHANGELOG that the generated output changes.
| class MetadataTypeBuilder implements TypeDefinitionBuilderInterface | ||
| { | ||
| /** | ||
| * Only the result envelope's own metadata type is skipped: it is supplied by the REST client |
| } | ||
|
|
||
| /** | ||
| * A named scalar type is an alias for a primitive with extra constraints, e.g. |
MetadataTypeBuilder claimed every type whose name merely contains "metadata" and returned null for it, so domain types such as AccountingMetadata were silently dropped and every type referencing them failed validation. Only the result envelope's own ResultMetadata is supplied by the REST client runtime, so only that name is skipped now. A named scalar type - a RAML DataType that is a primitive plus constraints, with no properties - was claimed by no builder and never became a TypeDefinition, which failed validation for every type referencing it. Such an alias has no entity to generate, so it is taken out of the type list before the builders run and the properties pointing at it become the underlying primitive instead, keeping the enum values as constants on the referencing property. Resolving the aliases before the builder loop also keeps them away from the builders that match on a name substring: ResultTypeBuilder used to abort on a scalar named ScanResult, and FilterTypeDefinitionBuilder used to emit an empty class for a scalar named FilterMode. Covered by the named-scalar fixture in the Javascript and PHP REST client suites, exercising an alias as a property, as an array item type, with and without enum values, and a domain type named PaymentMetadata.
bfe758a to
918e380
Compare
|
Thanks for the review — reworked and force-pushed as Tests now exist, and the suite runsThe reason the first version had none: PHPUnit cannot start on a stock Ubuntu 24.04 PHP 8.3, because Running it inside a container that has the extension works and needs nothing installed on the host: docker run --rm -v "$PWD":/app -w /app -u $(id -u):$(id -g) \
gitlab.paysera.net:5050/paysera/developer-environment/mokejimai:dev php bin/phpunitThat image is PHP 7.4.3, which matches New fixture Point by point
On #2 and #3 — one change instead of twoRather than filtering the alias list against the names that produced no $scalarAliases = $this->collectScalarAliases($apiTypes);
$apiTypes = array_diff_key($apiTypes, $scalarAliases);A named scalar has no entity to generate, so the honest model is that it never reaches the builders at all. That makes the contradiction in #2 structurally impossible — nothing can be both generated as a class and rewritten to a primitive — and it fixes #3 without adding a On #8 — deliberately out of scopeA named scalar used as a request or response body would need On #6 and #7 — these are not in this branchBoth findings describe files this PR adds. It does not add them.
One supporting detail in #1 is also off: Evidence that each defect was realRunning the new fixture against the older revisions, same fixture, same command:
|
mSprunskas
left a comment
mSprunskas
left a comment
There was a problem hiding this comment.
1. Medium — A named scalar used as a Result item type is not resolved
File: src/Paysera/Bundle/CodeGeneratorBundle/Service/TypeDefinitionBuilder.php:130-135
Problem: resolveScalarAliases() reads only $type->getProperties(). ResultTypeDefinition keeps its item type in a different field, itemsType (Entity/Definition/ResultTypeDefinition.php:15). ResultTypeBuilder fills this field
from $definition['properties'][$dataKey]['items']['type'] (ResultTypeBuilder.php:31). The new code does not change this field. The guard at line 152 tests instanceof ArrayPropertyDefinition, and a ResultTypeDefinition is not
a property. Thus the alias name stays.
Impact: For this RAML:
CurrencyResult:
type: Paysera.Result
properties:
currencies:
type: array
items:
type: Currency
Result.php.twig:9 writes return new Currency($data);. The Currency class is never generated, because the alias is removed at line 52. UsedTypesResolver::filterOutTypes() also drops Currency, so no import is written. The
generated client has a fatal error at run time. DefinitionValidator does not check ResultTypeDefinition::itemsType, so the generator gives no error message.
Fix: In resolveScalarAliases(), also test each type for ResultTypeDefinition and replace itemsType with the primitive type, in the same way as line 154.
---
2. Medium — Metadata types with "Result" in the name now go to ResultTypeBuilder
File: src/Paysera/Bundle/CodeGeneratorBundle/Service/TypeDefinitionBuilder/MetadataTypeBuilder.php:15-22
Problem: supports() now accepts only the exact short name resultmetadata. A domain type such as SearchResultMetadata or TransferResultMetadata gives searchresultmetadata, which is not equal. The type then goes to the next
builder. ResultTypeBuilder::supports() (position 20) accepts every name that contains Result (ResultTypeBuilder.php:14).
Impact: Two failure modes:
- The type has properties. The generator makes a ResultTypeDefinition, and dataKey becomes the first property name. The output is a wrong Result subclass, not an entity. There is no error message.
- The type has no properties. ResultTypeBuilder::buildTypeDefinition() throws InvalidDefinitionException and all generation stops.
Before this change, these types were removed and were not generated. The change makes the output wrong instead of absent.
Fix: Test the name against ResultTypeBuilder before MetadataTypeBuilder releases it, or make ResultTypeBuilder::supports() more exact (for example, a name that ends with Result).
---
3. Low — Named scalars still stop generation when used as a body type
File: src/Paysera/Bundle/CodeGeneratorBundle/Service/DefinitionValidator.php:70-104
Problem: The fix resolves aliases only in type properties. A body type is validated from the RAML tree:
body:
application/json:
type: Currency
validateType() resolves the proxy to the name Currency, then calls $api->getType('Currency'), which returns null because line 52 removed the alias. The method throws UnrecognizedTypeException.
Impact: The CHANGELOG says that named scalar types "no longer break generation for every type referencing them". This is not true for request bodies, response bodies, and inline nested object properties. The user gets the
message Did not found defined type "Currency", which does not identify the cause.
Fix: Resolve aliases for body types too, or limit the CHANGELOG text to properties and array item types.
---
4. Low — release.sh is a personal scratch file and is not ignored
File: release.sh:1-5
Problem: The file holds two bin/console js-generator:package commands with absolute personal paths (~/Projects/api-spec/..., ~/Projects/js-lib-paysera-clients/...). It has no shebang line, no set -e, and no execute
permission (-rw-rw-r--). The name says "release", but the file does not release anything. git check-ignore shows that .gitignore does not cover it.
Impact: A git add . puts this file in the repository. Other developers get a broken script with paths that do not exist on their machines.
Fix: Delete the file, or add it to .gitignore.
---
5. Low — The SymfonyBundle generator has no test for the new behaviour
File: tests/PhpGeneratorBundle/RestClient/GenerateRestClientCommandTest.php:86, tests/JavascriptGeneratorBundle/GeneratePackageCommandTest.php:93
Problem: The named-scalar case was added to the REST client test and to the JavaScript test. The Symfony bundle generator (tests/PhpGeneratorBundle/SymfonyBundle/GenerateBundleCommandTest.php) uses the same
TypeDefinitionBuilder service, but it has no named-scalar fixture.
Impact: The alias removal and the metadata change also change the entities and the normalizers of the Symfony bundle generator. No test protects that output.
Fix: Add a named-scalar fixture to the SymfonyBundle test data provider.
---
6. Note — Dead condition in collectScalarAliases()
File: src/Paysera/Bundle/CodeGeneratorBundle/Service/TypeDefinitionBuilder.php:104
Problem: The test is_array($definition) is always true. TypeCollection::toArray() returns name => array for each item (vendor/.../src/TypeCollection.php:197-205), and TypeDefinitionBuilderInterface::supports() already
declares array $definition.
Fix: Remove the condition.
---
7. Note — Documentation is not complete for the new private methods
File: src/Paysera/Bundle/CodeGeneratorBundle/Service/TypeDefinitionBuilder.php:20, 137, 163
Problem: The new property $constantBuilder has no @var tag, but $builders above it has one. resolveScalarAlias() and applyAliasConstants() have no @param tags, but collectScalarAliases() and resolveScalarAliases() have them.
The tag @return array name => definition at line 97 is not a correct docblock format.
Fix: Use the same documentation style in all methods of the file.
---
8. Note — The CHANGELOG entry does not show the behaviour change
File: CHANGELOG.md:8-11
File: CHANGELOG.md:8-11
Problem: Both entries are under ### Fixed. The first entry says that a new client generation "can add files". This is a change of behaviour, not only a correction.
Fix: Move the metadata entry to a ### Changed section. Keep a Changelog uses Changed for this type of entry.
Follow-up to review of 918e380. - Resolve a named scalar used as the item type of a result envelope. ResultTypeDefinition keeps that type on the type itself rather than on a property, so the property walk never reached it and createItem() referenced a class that is never generated. Both Result templates already return the payload untouched for a primitive, so the rewrite is enough. - Stop ResultTypeBuilder claiming names that end in "Metadata". Its match is a loose substring, so releasing SearchResultMetadata from MetadataTypeBuilder handed it to the result builder, which shaped it as an envelope instead of an entity. No api-spec type is affected today: the only name holding both words is ResultMetadata, still skipped at position 10. - Drop the always-true is_array() guard, align docblocks, and split the behaviour change out of "Fixed" into "Changed" in the changelog. The named scalar entry now states that request and response bodies stay unsupported. - Cover both cases in the named-scalar fixture and register that fixture with the Symfony bundle suite as well. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
@mSprunskas , Thanks for the second pass. Pushed as Before the per-item replies, the measurement that shaped my triage, because it changes how several of these should be weighted. Regression sweep over the whole spec corpusI generated a PHP REST client from every one of the 263
Reproduce with Fixed#1 — named scalar as a result item type. Confirmed and fixed. One correction on the framing, since it affects how the rest of the review should be read: this is not caused by this PR. On #2 — metadata types whose name contains "Result". Real, and the one item here I'd call a genuine regression, so thank you for catching it. Reproduced both modes: with no array property it dies with a Fixed on the return strpos($name, 'Result') !== false
&& preg_match(self::PATTERN_METADATA_SUFFIX, $name) !== 1
&& !array_key_exists(self::ANNOTATION_ENTITY, $definition);I deliberately did not take the other option you offered — making #5 — Symfony bundle coverage. Added; the Two notes. The file is #6 — dead #7 — docblocks. Added the missing One thing I'd like a ruling on, because it points the other way from your two inline comments on the previous round — "Redundant comments" on the #8 — changelog. The metadata entry moved to a Scoped rather than implemented#3 — named scalar as a body type. Confirmed: I took the second option from your own comment and limited the changelog wording instead of widening the fix, for two reasons. Supporting it means resolving aliases in the raw RAML tree that Couldn't action#4 —
A local worktree or a different checkout would explain it. No action needed from you — just flagging why that one is unaddressed, in case it looks skipped. Happy to fix it immediately if you can point me at where it lives. The neighbouring round-1 item that was real — Also from round oneVersion is Verification
|
2 participants
TR-7946
Client generation from
paysera/api-specis currently impossible — this affects every developer, not one team. The failure mode is silent and points at the wrong place, so it reads as your own mistake rather than a tool defect.Concretely:
@paysera/public-transfers-clientcannot be regenerated at all. It is stuck at 4.13.0 and missing fields that shipped long ago (vop_check.idfrom CORE-5433,vop_check.error_codefrom TR-7778), so frontend consumers have been reading raw payload keys instead of using the client.The mechanism
TypeDefinitionBuilder::buildTypeDefinitions()asks each buildersupports()and takes the first match. If the winning builder returnsnull, or if no builder claims the type, nothing is registered.DefinitionValidator::validateType()then looks the type up with$api->getType($name), getsnull, and throws:The name in that message is the consumer of the missing type, never the type that was dropped. That is what makes this so hard to diagnose — fix one, the next appears, and it looks like a moving target.
What this PR fixes
1.
MetadataTypeBuilderdiscarded any type whose name contained "metadata"…paired with
buildTypeDefinition()returningnull. That is correct forResultMetadata, the pagination envelope supplied by the REST client runtime rather than generated. But matching a substring of the name also swallows unrelated domain types.Real casualty:
AccountingMetadatainapp-evpbank/public-transfers(added by TSP-148). The type is well-formed; only its name is unlucky. Demonstrated by A/B — identical content namedAccountingMetagenerates cleanly, whileFooMetadataandMetadataFooboth fail.Now matches
ResultMetadataexactly. The comparison is made against the short name, because a typecoming from a library arrives qualified —
supports()is called withPaysera.ResultMetadata, notResultMetadata. Spaces and case are still normalised, preserving the old "meta data" tolerance for thatone name.
2. Named scalar types were claimed by no builder at all
SimpleTypeBuilder::supports()requiresisset($definition['properties']) || isset($definition['queryParameters']). A named scalar — a RAML DataType that is a primitive plus constraints, in its own fragment:…has neither, and no other builder claims it. It is never registered, so every property referencing it fails validation.
Real casualties:
ResolvedStatus(TR-7156) andTimelineMessageCode(TR-7561), both correctly declared inraml/types.raml. RAML 1.0 permits named scalars; the generator only understood object-shaped types.Such an alias has no entity of its own to generate — properties pointing at it are the underlying primitive.
resolveScalarAliases()rewrites those properties to the primitive and leaves the alias out of the generated output entirely.Test plan
Regression — 27 fixture APIs, byte-identical output
Generated a JS client from every
api.ramlundertests/on unmodifiedmaster, then again with this change, and compared the full output trees:This is the property that matters most: the two type categories being rescued are ones that previously aborted generation, so nothing that used to generate can change.
ResultMetadatais the only*Metadata*-named type in the entire fixture set (914 files), so narrowing the match is provably inert here.Forward — the real spec now gets past both defects
Against
app-evpbank/public-transfers/api.raml:Did not found defined type "AccountingMetadata"AccountingMetadata, passesResolvedStatus/TimelineMessageCode, stops atDid not found defined type "string | nil"That last one is union support — deliberately not in this PR, see below.
The repo's own PHPUnit suite was not executed. The PHP available to me has no
mbstring, which PHPUnit requires, and the only other runtimes on hand are PHP 7.4 containers that cannot run this codebase. The 27-API before/after comparison above is my substitute, but CI must confirm the unit suite. Please treat that as a required check rather than a formality.No fixture in the repo currently covers a named scalar type, so the new path has no golden-master coverage. Happy to add a fixture if you would like one — I left it out rather than hand-roll expected output I could not validate against a green suite.
Not in this PR
Two further blockers exist, both tracked under TR-7946:
X | nil) are unsupported —DefinitionValidator.php:83throws'UnionType currently is not supported', and in practice the type arrives unparsed and falls through to the same "Did not found defined type" message.api-specuses| nilpervasively for nullable fields, so this alone still blockspublic-transfers. Previously reported as INV-257 and closed "Won't Do" in Feb 2025. Kept separate because it is a feature, not a patch, and deserves its own decision. A separate PR will follow.api-spec— fixed inpaysera/api-spec!1789.Note on direction
PLT-1813 records an intent to replace this generator with OpenAPI Generator. If that migration is close, the sensible outcome may be to take these two low-risk fixes (which unblock everyone today) and leave unions permanently unsupported. Flagging it so the call is explicit.