feat: Refactor codegen to make it more flexible #15

This PR refactors the OpenAPI client code generation from a single forgejo-client crate into a generalized api-clients crate backed by a reusable openapi-gen-build build-time library. The architecture is cleaner, the multi-contract discovery is well thought out, and the post-processing for namespace rewriting is a nice touch.

One real bug needs fixing before merge: spec names containing characters invalid for Rust identifiers (e.g. hyphens in my-api.yaml) will produce unparseable pub mod my-api; declarations in the generated module tree. The file_stem should be sanitized (hyphens → underscores) in discover_specs. The rest are minor style / robustness concerns.

Severity	Count
🐛 Bug	1
⚠️ Robustness	1
💅 Style	1

*🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: changes

# Review Summary This PR refactors the OpenAPI client code generation from a single `forgejo-client` crate into a generalized `api-clients` crate backed by a reusable `openapi-gen-build` build-time library. The architecture is cleaner, the multi-contract discovery is well thought out, and the post-processing for namespace rewriting is a nice touch. **One real bug** needs fixing before merge: spec names containing characters invalid for Rust identifiers (e.g. hyphens in `my-api.yaml`) will produce unparseable `pub mod my-api;` declarations in the generated module tree. The `file_stem` should be sanitized (hyphens → underscores) in `discover_specs`. The rest are minor style / robustness concerns. | Severity | Count | |----------|-------| | 🐛 Bug | 1 | | ⚠️ Robustness | 1 | | 💅 Style | 1 | --- *🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: **changes**

clank-bot reviewed

2026-07-21 19:06:47 +00:00

clank-bot left a comment

Code review findings from opencode.

crates/openapi-gen-build/src/lib.rs

[bug] Spec names derived from file stems may contain characters invalid for Rust identifiers (e.g. my-api.yaml → pub mod my-api;). discover_specs should sanitize the name — replace hyphens and other non-identifier chars with underscores — before storing it in Spec.name.

[bug] Spec names derived from file stems may contain characters invalid for Rust identifiers (e.g. `my-api.yaml` → `pub mod my-api;`). `discover_specs` should sanitize the name — replace hyphens and other non-identifier chars with underscores — before storing it in `Spec.name`.

crates/openapi-gen-build/src/lib.rs

[style] let _ = fs::remove_dir_all(out) silently discards the error. If the directory doesn't exist or can't be removed, subsequent generation may misbehave. Use .ok() or log the error instead of a bare discard.

[style] `let _ = fs::remove_dir_all(out)` silently discards the error. If the directory doesn't exist or can't be removed, subsequent generation may misbehave. Use `.ok()` or log the error instead of a bare discard.

crates/openapi-gen-build/src/lib.rs

[style] unwrap() calls within walk_dir and recursive_copy will panic on I/O errors. While acceptable in build scripts (where panics fail the build), these should ideally propagate as Error::Io variants for consistency with the rest of the library.

[style] `unwrap()` calls within `walk_dir` and `recursive_copy` will panic on I/O errors. While acceptable in build scripts (where panics fail the build), these should ideally propagate as `Error::Io` variants for consistency with the rest of the library.

martin added 1 commit

2026-07-21 19:19:31 +00:00

sanitize crate names

ci/woodpecker/pr/pr-review Pipeline was successful

Details

ci/woodpecker/push/verify Pipeline was successful

Details

da04911fe7

clank-bot commented

2026-07-21 19:20:42 +00:00

Review Summary

This PR generalizes the OpenAPI codegen architecture: forgejo-client → api-clients (multi-contract support) and extracts the generator logic into a reusable openapi-gen-build build-dependency crate.

Overall direction is sound — the refactor reduces duplication and makes adding new API contracts trivial. However, there are correctness bugs and fragility issues.

Major Issues

1. Spec names with leading digits produce invalid Rust module names

The discover_specs sanitization (lib.rs:356) does not prepend an underscore for names starting with a digit. A spec file like 123api.yaml produces module name 123api, which generates pub mod 123api; — invalid Rust syntax. The test at lib.rs:357-362 confirms this bug (it asserts the invalid output).

Fix: prepend _ when the sanitized name starts with a digit.

2. Rust reserved keywords as spec names are not handled

No keyword filtering exists. A spec named for.json, match.yaml, or type.json would produce pub mod for; etc., which fails to compile.

Fix: reject or escape Rust reserved keywords (or add a _ suffix).

Minor Issues

3. `unwrap()` in production paths (`post_process_namespace`, `walk_dir`, `recursive_copy`)

Lines 499, 505, 531-541 use unwrap() on I/O operations. While build-time panics are less severe, propagating errors with ? would make the library more robust.

4. Fragile string replacement in `post_process_namespace`

The exact string use crate::{apis::ResponseContent, models}; (line 488) is hardcoded. If openapi-generator changes its output format (additional imports, different ordering), the rewriting silently fails, producing confusing compilation errors.

Fix: use a regex or AST-based approach, or at minimum match against a more flexible pattern.

5. Empty `mod.rs` when specs list is empty

If generate() is called with an empty specs list (e.g., if discover_specs returns error but manual spec() is never called), write_top_mod writes an empty mod.rs. While guarded by discover_specs returning an error on empty, the public API allows constructing an empty Config and calling generate().

Fix: return Err if specs are empty in generate().

Positive Aspects

Clean builder pattern with method chaining.
discover_specs auto-discovers all specs in a directory — very extensible.
Post-processing correctly rewrites crate:: → namespaced paths.
Proper emit_rerun_if_changed() support.
Error enum is well-designed with Display + Error impl.
Tests cover sanitization comprehensively (except the digit/keyword edge cases).

*🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: changes

# Review Summary This PR generalizes the OpenAPI codegen architecture: `forgejo-client` → `api-clients` (multi-contract support) and extracts the generator logic into a reusable `openapi-gen-build` build-dependency crate. **Overall direction is sound** — the refactor reduces duplication and makes adding new API contracts trivial. However, there are correctness bugs and fragility issues. ## Major Issues ### 1. Spec names with leading digits produce invalid Rust module names The `discover_specs` sanitization (`lib.rs:356`) does not prepend an underscore for names starting with a digit. A spec file like `123api.yaml` produces module name `123api`, which generates `pub mod 123api;` — invalid Rust syntax. The test at `lib.rs:357-362` confirms this bug (it asserts the invalid output). **Fix**: prepend `_` when the sanitized name starts with a digit. ### 2. Rust reserved keywords as spec names are not handled No keyword filtering exists. A spec named `for.json`, `match.yaml`, or `type.json` would produce `pub mod for;` etc., which fails to compile. **Fix**: reject or escape Rust reserved keywords (or add a `_` suffix). ## Minor Issues ### 3. `unwrap()` in production paths (`post_process_namespace`, `walk_dir`, `recursive_copy`) Lines 499, 505, 531-541 use `unwrap()` on I/O operations. While build-time panics are less severe, propagating errors with `?` would make the library more robust. ### 4. Fragile string replacement in `post_process_namespace` The exact string `use crate::{apis::ResponseContent, models};` (line 488) is hardcoded. If openapi-generator changes its output format (additional imports, different ordering), the rewriting silently fails, producing confusing compilation errors. **Fix**: use a regex or AST-based approach, or at minimum match against a more flexible pattern. ### 5. Empty `mod.rs` when specs list is empty If `generate()` is called with an empty specs list (e.g., if `discover_specs` returns error but manual `spec()` is never called), `write_top_mod` writes an empty `mod.rs`. While guarded by `discover_specs` returning an error on empty, the public API allows constructing an empty `Config` and calling `generate()`. **Fix**: return `Err` if specs are empty in `generate()`. ## Positive Aspects - Clean builder pattern with method chaining. - `discover_specs` auto-discovers all specs in a directory — very extensible. - Post-processing correctly rewrites `crate::` → namespaced paths. - Proper `emit_rerun_if_changed()` support. - `Error` enum is well-designed with `Display` + `Error` impl. - Tests cover sanitization comprehensively (except the digit/keyword edge cases). --- *🤖 Review by opencode (opencode/deepseek-v4-flash-free) — verdict: **changes**

clank-bot reviewed

2026-07-21 19:20:43 +00:00

clank-bot left a comment

Code review findings from opencode.

crates/openapi-gen-build/src/lib.rs

					
				@ -0,0 +2,4 @@

				use std::fs;

				use std::path::{Path, PathBuf};

				use std::process::Command;

[style] Production code uses unwrap() extensively in post_process_namespace (lines 499, 505), walk_dir (lines 550-557), and recursive_copy (lines 531-541). These panic on I/O errors. Propagate with ? instead.

[style] Production code uses `unwrap()` extensively in `post_process_namespace` (lines 499, 505), `walk_dir` (lines 550-557), and `recursive_copy` (lines 531-541). These panic on I/O errors. Propagate with `?` instead.

crates/openapi-gen-build/src/lib.rs

					
				@ -0,0 +353,4 @@

				        drop(dir);

				    }

				    #[test]

[bug] Spec names starting with a digit (e.g. 123api.yaml → 123api) produce invalid Rust module names. pub mod 123api; is a syntax error. Prepending _ when the first character is a digit would fix this.

[bug] Spec names starting with a digit (e.g. `123api.yaml` → `123api`) produce invalid Rust module names. `pub mod 123api;` is a syntax error. Prepending `_` when the first character is a digit would fix this.

[bug] Rust reserved keywords (e.g. for, match, type, ref, self, impl) are not filtered. A spec named for.json would produce pub mod for; which fails to compile. Add keyword detection and apply an escape (e.g. _ suffix).

[bug] Rust reserved keywords (e.g. `for`, `match`, `type`, `ref`, `self`, `impl`) are not filtered. A spec named `for.json` would produce `pub mod for;` which fails to compile. Add keyword detection and apply an escape (e.g. `_` suffix).

crates/openapi-gen-build/src/lib.rs

					
				@ -0,0 +354,4 @@

				    }

				    #[test]

				    fn test_sanitize_starts_with_digit() {

[bug] The test test_sanitize_starts_with_digit asserts that 123api is the expected module name, encoding the bug. This test should instead assert the output is _123api (or similar) after the fix.

[bug] The test `test_sanitize_starts_with_digit` asserts that `123api` is the expected module name, encoding the bug. This test should instead assert the output is `_123api` (or similar) after the fix.

crates/openapi-gen-build/src/lib.rs

					
				@ -0,0 +384,4 @@

				    }

				    struct TempDir(std::path::PathBuf);

[style] generate() does not guard against an empty specs list. If the builder is misconfigured, write_top_mod writes an empty mod.rs. Add a check returning Err when specs is empty.

[style] `generate()` does not guard against an empty `specs` list. If the builder is misconfigured, `write_top_mod` writes an empty `mod.rs`. Add a check returning `Err` when specs is empty.

crates/openapi-gen-build/src/lib.rs

[style] Exact string use crate::{apis::ResponseContent, models}; is hardcoded. If openapi-generator changes import formatting (different items, ordering, or multi-line), the replacement silently fails. Consider a more flexible pattern or AST-based rewrite.

[style] Exact string `use crate::{apis::ResponseContent, models};` is hardcoded. If openapi-generator changes import formatting (different items, ordering, or multi-line), the replacement silently fails. Consider a more flexible pattern or AST-based rewrite.