refactor: built in openapi generator#58
Conversation
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR modernizes the OpenAPI generator to use a built-in model and reflection, but introduces a compilation error in an internal mapper file due to a renamed type (openapi.OAuthFlowsImplicit → openapi.OAuthFlow), and leaves CONNECT method support inconsistently gated across adapters.
📄 Documentation Diagram
This diagram documents the new OpenAPI generation flow using the built-in generator with reflection.
sequenceDiagram
participant Client
participant Router as spec.Router
participant Gen as Generator
participant Reflector as Reflector
participant Doc as openapi.Document
Client->>Router: NewRouter(opts...)
Router->>Gen: create generator
Client->>Router: Post(path, opts...)
Router->>Gen: Register route
Client->>Router: Get(path, opts...)
Router->>Gen: Register route
Client->>Router: MarshalYAML()
Router->>Gen: buildOnce()
Gen->>Reflector: reflect schemas from Go types
Reflector-->>Gen: *openapi.Schema
Gen->>Doc: build OpenAPI document
note over Router,Doc: PR replaces external swaggest library<br/>with owned OpenAPI model and reflector
Router-->>Client: YAML/JSON output
🌟 Strengths
- Solid golden-file test updates ensuring deterministic output changes are reviewed.
| Priority | File | Category | Impact Summary | Anchors |
|---|---|---|---|---|
| P1 | adapter/chiopenapi/router_test.go |
Bug | Stale OAuthFlowsImplicit type blocks compilation |
path:internal/mapper/openapi3.go |
| P2 | adapter/chiopenapi/router.go |
Architecture | CONNECT gating not applied to other adapters | |
| P2 | Makefile |
Testing | Fragile coverage aggregation, less readable output | |
| P2 | adapter/chiopenapi/router_test.go |
Testing | Raw byte diff makes tests fragile to formatting |
🔍 Notable Themes
- Inconsistent CONNECT version gating: Only Chi adapter checks
openapi.Version320before skipping CONNECT; other adapters silently ignore it with OpenAPI 3.2. - Test fragility: Replacing semantic YAML comparison with byte comparison will cause false-positive failures on formatting changes.
📈 Risk Diagram
This diagram illustrates the stale mapper reference risk introduced by the type rename.
sequenceDiagram
participant Test as Test (router_test.go)
participant Mapper as internal/mapper/openapi3.go
participant OpenAPIPkg as openapi package
Test->>OpenAPIPkg: uses openapi.OAuthFlow (new type)
OpenAPIPkg-->>Test: compiles fine
Mapper->>OpenAPIPkg: references openapi.OAuthFlowsImplicit (old type)
OpenAPIPkg-->>Mapper: type not found – compile error
note over Mapper: R1(P1): Stale reference to removed type<br/>breaks build of internal/mapper
💡 Have feedback? We'd love to hear it in our GitHub Discussions.
✨ This review was generated by LlamaPReview Advanced, which is free for all open-source projects. Learn more.
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
…pesific version and style readme
Description
Type of Change
Checklist
make checkpasses locally (sync + tidy + lint + test)make test-update)Notes for Reviewers