fix: more fixes to duplicates and collisions (#80)

Author: dotkas

crates/oapi-codegen/src/console.rs

@@ -235,6 +235,7 @@ fn hints_for(err: &Error) -> Vec<String> {
         }
         Error::TypeNameCollision { hint, .. }
         | Error::SchemaNameCollision { hint, .. }
+        | Error::OperationNameCollision { hint, .. }
         | Error::InvalidTypeNameSuffix { hint, .. } => {
             return vec![hint.clone()];
         }

crates/oapi-codegen/src/error.rs

@@ -155,6 +155,26 @@ pub enum Error {
         hint: String,
     },
 
+    /// Two operations collapsed onto one Rust method name.
+    ///
+    /// Every artifact of an operation derives from this one name, so the file
+    /// holds a duplicate trait method, response enum, and handler, and the router
+    /// points both routes at one handler. The generator will not choose which
+    /// operation keeps the plain name, because that choice belongs to the spec
+    /// author.
+    OperationNameCollision {
+        /// The Rust method name that both operations produce.
+        ident: String,
+        /// `method path` of the operation that claimed the name first, in
+        /// document order.
+        first: String,
+        /// `method path` of the operation that collided with `first`.
+        second: String,
+        /// How to resolve the clash. Rendered by the console as a hint, and not
+        /// by `Display`, so the console does not print it twice.
+        hint: String,
+    },
+
     /// `output-options.type-name-suffix` holds no identifier characters.
     ///
     /// Casing drops punctuation and separators, so a suffix such as `-` or `_`
@@ -256,6 +276,14 @@ impl std::fmt::Display for Error {
                     "component schemas `{first}` and `{second}` both produce the Rust type name `{ident}`"
                 );
             }
+            Error::OperationNameCollision {
+                ident, first, second, ..
+            } => {
+                return write!(
+                    f,
+                    "operations `{first}` and `{second}` both produce the Rust method name `{ident}`"
+                );
+            }
             Error::InvalidTypeNameSuffix { suffix, .. } => {
                 return write!(
                     f,
@@ -297,6 +325,7 @@ impl std::error::Error for Error {
             | Error::SchemaDepthExceeded { .. }
             | Error::TypeNameCollision { .. }
             | Error::SchemaNameCollision { .. }
+            | Error::OperationNameCollision { .. }
             | Error::InvalidTypeNameSuffix { .. }
             | Error::InvalidPathParameter { .. }
             | Error::UndeclaredPathParameter { .. }

crates/oapi-codegen/src/lower/paths.rs

@@ -85,6 +85,7 @@ use crate::lower::schema::string_format_type;
 use crate::lower::security;
 use crate::naming::Case;
 use crate::naming::RustIdent;
+use crate::naming::X_RUST_NAME;
 use crate::naming::operations;
 use crate::naming::to_ident;
 
@@ -169,6 +170,13 @@ impl Lowerer<'_> {
         let catalogue = security::scheme_catalogue(self.spec);
         let mut operations = Vec::new();
         let mut used_schemes: Vec<String> = Vec::new();
+        // Maps a claimed method name back to the `method path` that claimed it, so
+        // a collision error can name the earlier operation and not the identifier
+        // alone. Filtering runs before lowering (see `crate::filter`), and nothing
+        // prunes an operation afterwards, so every name claimed here reaches the
+        // file and every collision found here is real.
+        let mut claimed: BTreeMap<String, String> = BTreeMap::new();
+        let mut collisions = crate::lower::validate::Diagnostics::new();
         for (path, entry) in self.spec.paths().iter() {
             let item = match entry {
                 ReferenceOr::Item(item) => item,
@@ -182,6 +190,24 @@ impl Lowerer<'_> {
             };
             for (method, operation) in item.iter() {
                 let mut lowered = self.lower_operation(path, method, operation, &item.parameters)?;
+                let route = format!("{method} {path}");
+                match claimed.get(lowered.name.logical()) {
+                    Some(first) => {
+                        collisions.push(Error::OperationNameCollision {
+                            ident: lowered.name.logical().to_owned(),
+                            first: first.clone(),
+                            second: route,
+                            hint: operation_collision_hint(operation),
+                        });
+                        // Do not lower this operation into the service. Its
+                        // artifacts all derive from the colliding name, so keeping
+                        // it would emit the duplicates this check exists to stop.
+                        continue;
+                    }
+                    None => {
+                        claimed.insert(lowered.name.logical().to_owned(), route);
+                    }
+                }
                 lowered.security = self.operation_security(operation);
                 for key in &lowered.security {
                     if !used_schemes.iter().any(|existing| return existing == key) {
@@ -191,6 +217,7 @@ impl Lowerer<'_> {
                 operations.push(lowered);
             }
         }
+        collisions.into_result()?;
         let security_schemes = catalogue
             .into_iter()
             .filter(|scheme| return used_schemes.iter().any(|key| return *key == scheme.key))
@@ -1444,16 +1471,58 @@ impl Lowerer<'_> {
     }
 }
 
-/// Derive the trait method name: the `operationId` if present, else a name
-/// synthesised from the method and path (for example `get /v1/widgets` -> `get_v1_widgets`).
+/// Derive the trait method name: an explicit `x-rust-name`, else the
+/// `operationId`, else a name synthesised from the method and path (for example
+/// `get /v1/widgets` -> `get_v1_widgets`).
+///
+/// `x-rust-name` is the escape hatch for two `operationId`s that collapse onto one
+/// Rust name. It takes precedence over `operationId`, the same way it does for a
+/// schema, so the author names the method and the generator does not.
 fn operation_name(path: &str, method: &str, operation: &OasOperation) -> crate::naming::RustIdent {
+    if let Some(name) = operation
+        .extensions
+        .get(X_RUST_NAME)
+        .and_then(|value| return value.as_str())
+    {
+        return operations::operation_method_name(name);
+    }
     if let Some(id) = &operation.operation_id {
         return operations::operation_method_name(id);
     }
     let synthesised = format!("{method} {path}");
     return operations::operation_method_name(&synthesised);
 }
 
+/// Build the remedy text for an operation-name collision.
+///
+/// `operation` is the one that collided, and the advice depends on what it already
+/// declares. An operation that sets `x-rust-name` needs a different name and not
+/// the extension it already uses. An operation with no `operationId` derives its
+/// name from the method and path, so adding an `operationId` is the natural fix.
+///
+/// There is no suffix option for operations, unlike `type-name-suffix` for
+/// schemas. A method name appears in the trait a consumer implements, so every
+/// name a consumer writes stays the author's choice.
+fn operation_collision_hint(operation: &OasOperation) -> String {
+    if operation.extensions.contains_key(X_RUST_NAME) {
+        return format!(
+            "This operation already sets `{X_RUST_NAME}`, and that name collides too. \
+             Give it a name that no other operation uses."
+        );
+    }
+    if operation.operation_id.is_some() {
+        return format!(
+            "Two `operationId`s that differ only in case or in punctuation produce one Rust name. \
+             Give one of the two operations a different `operationId`, or set `{X_RUST_NAME}` on it \
+             to name the generated method directly."
+        );
+    }
+    return format!(
+        "This operation declares no `operationId`, so its name comes from the method and the path. \
+         Add an `operationId`, or set `{X_RUST_NAME}` on it to name the generated method directly."
+    );
+}
+
 /// The operation's doc comment, preferring `summary` over `description`.
 fn operation_doc(operation: &OasOperation) -> Option<String> {
     if let Some(summary) = &operation.summary

crates/oapi-codegen/src/naming.rs

@@ -4,9 +4,11 @@
 use proc_macro2::Ident;
 use proc_macro2::Span;
 
-/// The `x-rust-name` extension key: override a generated identifier (of a type,
-/// field, or server) with a caller-supplied name. Shared so every consumer and
-/// any user-facing message names the same key.
+/// The `x-rust-name` extension key: override a generated identifier with a
+/// caller-supplied name. It applies to a top-level schema (the type name), a
+/// property (the field name), an operation (the method name, and so the name of
+/// every artifact derived from it), and a `servers:` entry (the server URL name).
+/// Shared so every consumer and any user-facing message names the same key.
 pub(crate) const X_RUST_NAME: &str = "x-rust-name";
 
 /// `snake_case` / `UpperCamelCase` conversion used by [`to_ident`].

crates/oapi-codegen/tests/coverage.rs

@@ -459,19 +459,27 @@ const COMBINED_UNSUPPORTED_FIXTURES: &[&str] = &[
     "combined_reserved_name_client_error",
 ];
 
-/// Model-only fixtures for schema-name collisions. The golden-file tests do not
-/// cover these, because one must fail and the other needs a config option.
+/// Fixtures for name collisions. The golden-file tests do not cover these,
+/// because some must fail and others need a config option or an extension.
 ///
 /// `type_name_collision_error` must fail. Two schema names collapse onto one Rust
 /// identifier, and the spec offers no remedy. `type_name_collision_suffix` must
-/// succeed, because `output-options.type-name-suffix` resolves the collision. The
-/// tests below exercise both.
+/// succeed, because `output-options.type-name-suffix` resolves the collision.
+///
+/// The `operation_name_collision_*` fixtures do the same for a method name.
+/// `_error` and `_synthesised` must fail, `_rust_name` succeeds through
+/// `x-rust-name`, and `_filtered` succeeds because a filter removes one of the two
+/// operations before lowering. The tests below exercise each one.
 const NAMING_COLLISION_FIXTURES: &[&str] = &[
     "type_name_collision_error",
     "type_name_collision_suffix",
     "type_name_collision_pruned",
     "type_name_collision_reachable",
     "type_name_collision_inline_overlap",
+    "operation_name_collision_error",
+    "operation_name_collision_rust_name",
+    "operation_name_collision_synthesised",
+    "operation_name_collision_filtered",
 ];
 
 /// Absolute path to the crate's `tests` directory.
@@ -1377,6 +1385,120 @@ fn collision_between_unused_schemas_fails_for_models_only() {
     assert_eq!(problems.len(), 2, "expected both collisions, got: {problems:?}");
 }
 
+/// Two `operationId`s that collapse onto one Rust method name must stop
+/// generation, and one run must report every collision.
+///
+/// Every artifact of an operation derives from its method name, so a collision
+/// emitted a duplicate trait method, response enum, and handler, and pointed both
+/// routes at one handler. `rustc` rejects that with `E0428`.
+#[test]
+fn colliding_operation_ids_fail() {
+    let fixture = tests_dir().join("fixtures").join("operation_name_collision_error.yaml");
+    let err = oapi_codegen::generate(&fixture, &server_config())
+        .expect_err("two operations that produce one method name must stop generation");
+    let oapi_codegen::Error::Validation { problems } = &err else {
+        panic!("expected an aggregated Validation error, got: {err:?}");
+    };
+    assert_eq!(problems.len(), 2, "expected both collisions, got: {problems:?}");
+    let report = problems
+        .iter()
+        .map(|problem| {
+            return problem.to_string();
+        })
+        .collect::<Vec<String>>()
+        .join("\n");
+    assert!(
+        report.contains("list_widgets") && report.contains("get_thing"),
+        "the report names both colliding method names, got: {report}",
+    );
+    assert!(
+        report.contains("get /widgets") && report.contains("get /gadgets"),
+        "the report names the route of each colliding operation, got: {report}",
+    );
+}
+
+/// The client emitter derives its names from the same lowered operation, so it
+/// must reject the same collision. One check in the shared lowering pass covers
+/// both emitters.
+#[test]
+fn colliding_operation_ids_fail_for_the_client() {
+    let fixture = tests_dir().join("fixtures").join("operation_name_collision_error.yaml");
+    let err = oapi_codegen::generate(&fixture, &client_config())
+        .expect_err("a client run derives the same names, so the collision must stop it");
+    let oapi_codegen::Error::Validation { problems } = &err else {
+        panic!("expected an aggregated Validation error, got: {err:?}");
+    };
+    assert_eq!(problems.len(), 2, "expected both collisions, got: {problems:?}");
+}
+
+/// `x-rust-name` is the only escape hatch for an operation, and it names every
+/// artifact of that operation and not the trait method alone.
+#[test]
+fn x_rust_name_resolves_an_operation_collision() {
+    let fixture = tests_dir()
+        .join("fixtures")
+        .join("operation_name_collision_rust_name.yaml");
+    let code = oapi_codegen::generate(&fixture, &server_config())
+        .expect("`x-rust-name` must resolve the collision it exists for");
+    for expected in [
+        "fn list_widgets",
+        "fn list_gadgets",
+        "enum ListWidgetsResponse",
+        "enum ListGadgetsResponse",
+        "list_gadgets_handler",
+    ] {
+        assert!(
+            code.contains(expected),
+            "the override names every artifact, and `{expected}` is missing from: {code}",
+        );
+    }
+}
+
+/// An operation with no `operationId` takes its name from the method and the path,
+/// so two such paths can collide as well. The remedy differs, because there is no
+/// `operationId` to change.
+#[test]
+fn colliding_synthesised_operation_names_fail() {
+    let fixture = tests_dir()
+        .join("fixtures")
+        .join("operation_name_collision_synthesised.yaml");
+    let err = oapi_codegen::generate(&fixture, &server_config())
+        .expect_err("two synthesised names that collide must stop generation");
+    let oapi_codegen::Error::OperationNameCollision { ident, hint, .. } = &err else {
+        panic!("expected a single OperationNameCollision, got: {err:?}");
+    };
+    assert_eq!(ident, "get_widgets_list");
+    assert!(
+        hint.contains("declares no `operationId`"),
+        "the remedy must address the missing `operationId`, got: {hint}",
+    );
+}
+
+/// Filtering runs before lowering, so a collision that a filter removes must not
+/// stop generation. This is the trap the schema-name check fell into: reporting a
+/// collision between an item the file holds and one that it never emits.
+#[test]
+fn operation_collision_a_filter_removes_still_generates() {
+    let fixture = tests_dir()
+        .join("fixtures")
+        .join("operation_name_collision_filtered.yaml");
+    let mut config = server_config();
+    config
+        .output_options
+        .exclude_operation_ids
+        .push("listWidgets".to_owned());
+    let code = oapi_codegen::generate(&fixture, &config)
+        .expect("only one operation survives the filter, so no collision remains");
+    assert!(
+        code.contains("fn list_widgets"),
+        "the operation that survives is emitted: {code}",
+    );
+    assert!(
+        !code.contains("/gadgets"),
+        "the operation the filter removes is not emitted: {code}",
+    );
+}
+
 /// A parameter declared `in: path` with no matching `{placeholder}` in the path
 /// template must fail generation with a guided error rather than silently drop
 /// the parameter from the generated signature.

crates/oapi-codegen/tests/fixtures/operation_name_collision_error.yaml

@@ -0,0 +1,34 @@
+openapi: 3.0.3
+info:
+  title: Operation name collision without a remedy
+  version: 1.0.0
+paths:
+  # Two `operationId`s that differ only in punctuation and case collapse onto one
+  # Rust method name (`list_widgets`). Every artifact of an operation derives from
+  # that name, so generation must stop.
+  /widgets:
+    get:
+      operationId: list-widgets
+      responses:
+        "200":
+          description: ok
+  /gadgets:
+    get:
+      operationId: listWidgets
+      responses:
+        "200":
+          description: ok
+  # A second, independent collision on `get_thing`. One run must report this
+  # collision and the one above together, and not stop at the first one.
+  /things:
+    get:
+      operationId: get-thing
+      responses:
+        "200":
+          description: ok
+  /stuff:
+    get:
+      operationId: getThing
+      responses:
+        "200":
+          description: ok

crates/oapi-codegen/tests/fixtures/operation_name_collision_filtered.yaml

@@ -0,0 +1,22 @@
+openapi: 3.0.3
+info:
+  title: Operation name collision that a filter removes
+  version: 1.0.0
+paths:
+  # `list-widgets` and `listWidgets` collide, but `exclude-operation-ids` removes
+  # the second one before lowering. Only one operation then claims the name, so
+  # generation must succeed. A check that ran over the unfiltered document would
+  # report a collision between an operation that the file holds and one that it
+  # does not.
+  /widgets:
+    get:
+      operationId: list-widgets
+      responses:
+        "200":
+          description: ok
+  /gadgets:
+    get:
+      operationId: listWidgets
+      responses:
+        "200":
+          description: ok