Repository navigation
Emit a clear error instead of crashing on duplicate generated schema names - #914
adityasingh2400 wants to merge 3 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.
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.
|
|
||
| let nodes = decls.compactMap(DeclarationRecursionDetector.Node.init) | ||
| let nodeLookup = Dictionary(uniqueKeysWithValues: nodes.map { ($0.name, $0) }) | ||
| var duplicateNames: Set<String> = [] |
There was a problem hiding this comment.
This PR as-is is a worthwhile, incremental improvement, so we can move forward with this approach. However, if you're motivated to improve it further in a follow up, we can enhance this to keep track of the schema location and even be able to emit diagnostics that show up in the editor, pointing to the duplicate entries in the OpenAPI document.
| @@ -25,7 +25,31 @@ extension TypesFileTranslator { | |||
| func boxRecursiveTypes(_ decls: [Declaration]) throws -> [Declaration] { | |||
There was a problem hiding this comment.
Regardless of whether we enhance to provide a richer diagnostic including schema location, this is not the right place for this check. This function should do only what it says (boxing recursive types).
Further down in this file you'll find translateSchemas(_:multipartSchemasNames:). This logic should move there either as a pass over the computed [Declaration] list, or as part of the flatMap.
We should also check to see if TypeAssigner is a logical place to put this. I can't remember how stateful that is, but if it is stateful, it seems like the best place to keep track of which types have been assigned and emit the diagnostics.
Per review, translateBoxedTypes should only box recursive types, so the duplicate generated schema name check now runs in translateSchemas as a pass over the computed declarations before boxing. boxRecursiveTypes keeps the first node on a name collision so it never traps if a non-throwing diagnostic collector lets execution continue past the error. The clear error message and context keys are unchanged.
|
Thanks for the review. I moved the duplicate-name check out of translateBoxedTypes and into translateSchemas, where it now runs as a pass over the computed declarations before boxing. translateBoxedTypes is back to only boxing recursive types, and it keeps the first node on a name collision so it cannot trap if a non-throwing diagnostic collector lets execution continue past the error. The clear error message is unchanged and the existing tests still pass. I left the richer schema-location diagnostic and the TypeAssigner idea as possible follow ups. |
simonjbeaumont
left a comment
There was a problem hiding this comment.
Thanks. This is nearly ready to land. Just needs a couple more tests.
| // Duplicate generated names are detected and reported earlier, in | ||
| // `translateSchemas`. Keep the first node on a collision here so this | ||
| // step never traps even if a non-throwing diagnostic collector lets | ||
| // execution continue past that error. |
There was a problem hiding this comment.
| // Duplicate generated names are detected and reported earlier, in | |
| // `translateSchemas`. Keep the first node on a collision here so this | |
| // step never traps even if a non-throwing diagnostic collector lets | |
| // execution continue past that error. |
| if sortedNames.count == 1 { | ||
| let duplicateName = sortedNames[0] | ||
| 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] | ||
| } else { | ||
| let nameList = sortedNames.map { "'\($0)'" }.joined(separator: ", ") | ||
| message = | ||
| "Multiple schemas in '#/components/schemas' map to the same generated Swift type names \(nameList), which is not supported. This usually happens when the naming strategy collapses distinct OpenAPI names into one. Switch the namingStrategy to 'defensive', or add 'nameOverrides' entries to give the schemas different names." | ||
| context = ["names": nameList] | ||
| } |
There was a problem hiding this comment.
Seems over the top to special case the single entry list. Let's just use one path.
| if sortedNames.count == 1 { | |
| let duplicateName = sortedNames[0] | |
| 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] | |
| } else { | |
| let nameList = sortedNames.map { "'\($0)'" }.joined(separator: ", ") | |
| message = | |
| "Multiple schemas in '#/components/schemas' map to the same generated Swift type names \(nameList), which is not supported. This usually happens when the naming strategy collapses distinct OpenAPI names into one. Switch the namingStrategy to 'defensive', or add 'nameOverrides' entries to give the schemas different names." | |
| context = ["names": nameList] | |
| } | |
| let nameList = sortedNames.map { "'\($0)'" }.joined(separator: ", ") | |
| message = | |
| "Multiple schemas in '#/components/schemas' map to the same generated Swift type names \(nameList), which is not supported. This usually happens when the naming strategy collapses distinct OpenAPI names into one. Switch the namingStrategy to 'defensive', or add 'nameOverrides' entries to give the schemas different names." | |
| context = ["names": nameList] |
There was a problem hiding this comment.
Please add a test that shows that duplicate basenames in different namespaces do not produce an error. E.g. A.T and B.T are fine. The tests you added show only top-level types in the Schemas dictionary.
…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.Note: this supersedes #907, which I closed by accident during a bulk cleanup of my stale forks and cannot reopen because the original fork was deleted. The branch is identical to the reviewed state of #907 including the uniquingKeysWith rework discussed there.