Repository navigation
Emit a clear error instead of crashing on duplicate generated schema names - #907
adityasingh2400 wants to merge 2 commits into
Conversation
…names
When the idiomatic naming strategy maps two distinct OpenAPI schema names
(for example NullTime and nullTime) to the same Swift type name, the
generator trapped while boxing recursive types. The boxing code built a
name-to-node lookup with Dictionary(uniqueKeysWithValues:), which calls
fatalError on duplicate keys, surfacing as
Fatal error: Duplicate values for key: 'NullTime'
with no indication of which document or strategy caused it.
Detect the collision before constructing the lookup and emit an error
diagnostic that names the conflicting type and points at the documented
workarounds (switch namingStrategy to defensive, or add a nameOverrides
entry). The lookup now also tolerates duplicate keys so the diagnostic is
the failure path rather than a trap, regardless of the diagnostic
collector in use.
Adds a regression test that translates two schemas which collide under the
idiomatic strategy and asserts the error is emitted; it crashed before this
change. Also covers that distinct idiomatic names emit no error.
simonjbeaumont
left a comment
simonjbeaumont
left a comment
There was a problem hiding this comment.
Thanks for contributing! I added a comment about a direction that would support the better diagnostics, without sacrificing the performance.
| if let duplicateName = nodes.firstDuplicateName { | ||
| try diagnostics.emit( | ||
| .error( | ||
| message: | ||
| "Multiple schemas in '#/components/schemas' map to the same generated Swift type name '\(duplicateName)', which is not supported. This usually happens when the naming strategy collapses two distinct OpenAPI names into one. Switch the namingStrategy to 'defensive', or add a 'nameOverrides' entry to give one of the schemas a different name.", | ||
| context: ["name": duplicateName] | ||
| ) | ||
| ) | ||
| } | ||
| let nodeLookup = Dictionary(nodes.map { ($0.name, $0) }, uniquingKeysWith: { first, _ in first }) |
There was a problem hiding this comment.
I like the motivation of surfacing better errors to users but I don't love that we're now iterating over the nodes twice. Can we instead use the uniquingKeysWith: closure to capture the duplicates and then surface the diagnostic errors afterwards if duplicates are found.
Given the closure will be called for each duplicate this will also support providing a richer diagnostic when there is more than one duplicate.
There was a problem hiding this comment.
Good call, done in 6aef8fb. Duplicate detection now lives in the uniquingKeysWith: closure that builds the lookup, so we no longer scan the nodes a second time. The closure records each colliding name into a set, and afterward a single error reports all of them, so two distinct collisions surface together instead of stopping at the first. I added a test covering the multi-collision case alongside the existing single-collision one.
Per review, fold duplicate detection into the uniquingKeysWith: closure that builds the node lookup, instead of a separate firstDuplicateName scan. This removes the second iteration over the nodes and lets a single diagnostic report every colliding generated name when more than one pair collapses.
|
Thanks, that is a much cleaner approach. I reworked it to build the lookup with |
|
@adityasingh2400 did you mean to close this? Sorry for the delays in reviewing but I'll get to it soon. |
|
Sorry about that @simonjbeaumont, that was my mistake. I was bulk cleaning up stale forks on my profile and this got caught in the sweep, and since the fork was deleted I cannot reopen this one. I have restored the exact same branch including the uniquingKeysWith rework and opened #914 to replace it. No changes beyond what you see here, and no rush on the review. Thanks for picking it up. |
…names (#939) ## Summary When the idiomatic naming strategy maps distinct OpenAPI component keys to the same generated Swift type name, schema translation currently reaches `Dictionary(uniqueKeysWithValues:)` in recursive-type boxing and traps. This change detects duplicate generated top-level schema names before boxing and emits a deterministic error diagnostic with the documented `defensive` naming and `nameOverrides` resolution paths. The defensive lookup in `translateBoxedTypes` keeps execution from reaching the duplicate-key dictionary initializer when a non-throwing diagnostic collector continues after the error. Duplicate basenames in different namespaces remain valid. Fixes #854. ## Prior work and attribution This continues the work from #907/#914 by Aditya Singh and incorporates the review feedback left there. The original issue and reproduction were reported by @marknefedov in #854. Aditya Singh (`@adityasingh2400`) implemented the substantial prior repair in #907 and #914. The placement in `translateSchemas`, keeping `translateBoxedTypes` focused on boxing, the single plural diagnostic path, and the `A.T` / `B.T` namespace countercase follow review direction from @simonjbeaumont on #914. The bug report and repair lineage are not original to this patch; this submission does not claim discovery or sole authorship. ## Verification - Apple Swift 6.3.2 / macOS SDK 26.5 build 25F70: frozen baseline collision probe RED with exit 133 and `Fatal error: Duplicate values for key: 'NullTime'`. - The identical collision probe after applying this exact patch completes with one error diagnostic and exit 0. - `A.T` / `B.T` repository-native generated identities `A.TPayload` / `B.TPayload`: exit 0, no diagnostic. - `Test_translateSchemas`: throwing collector, non-throwing collector, deterministic multiple collisions, and namespace countercase coverage. - Previously verified affected offline surface: 312 tests, 1 expected pre-existing skip, 0 failures. - Strict `swift-format lint` and `git diff --check`: exit 0. ## Scope Exactly three files change: two `_OpenAPIGeneratorCore` implementation files and one focused test file. No public API or new architecture is introduced. --------- Co-authored-by: Aditya Singh <adisin650@gmail.com>
When the idiomatic naming strategy maps two distinct OpenAPI schema names to the same generated Swift type name, the generator crashes instead of reporting the problem. This is the crash tracked in #854: a document with both
NullTimeandnullTimeunder#/components/schemasproducesRoot cause is in
boxRecursiveTypes, which builds a name-to-node lookup for recursion detection withDictionary(uniqueKeysWithValues:). That initializer traps on duplicate keys, and the idiomatic strategy can produce two declarations sharing a name even though the OpenAPI names were distinct. The defensive strategy keeps them distinct, which is why the crash only shows up with idiomatic naming.The fix detects the collision before building the lookup and emits an error diagnostic that names the conflicting type and points at the documented workarounds (switch
namingStrategytodefensive, or add anameOverridesentry). TheConfiguring-the-generatorarticle already notes that idiomatic naming "might produce name conflicts (in that case, switch back to defensive)", so this turns an undiagnosed trap into the guidance that already exists. The lookup construction now also tolerates duplicate keys, so the error diagnostic is the failure path rather than a fatal trap, independent of which diagnostic collector is in use.Verification: running the generator with
namingStrategy: idiomaticon a spec containingNullTimeandnullTimepreviously crashed; it now exits non-zero withI added a regression test in
Test_translateSchemasthat translates two schemas colliding under the idiomatic strategy and asserts the error is emitted. With the previous code that test aborts with theDuplicate values for keytrap (signal 5); with the fix it passes. A second test confirms that distinct idiomatic names emit no error. The fullOpenAPIGeneratorCoreTeststarget (115 tests) and the reference tests (114 tests) pass, so generated output for valid documents is unchanged, including recursive-type boxing.