refactor: change openapi library#2
Conversation
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR attempts to migrate from swaggest/openapi-go to oaswrap/spec but the core SpecCollector struct hasn't been updated, causing a compilation error in the test file. Additionally, leftover imports remain in several files.
🚨 Critical Issues (P0)
- builder_test.go: The test references
sc.doc.PathsbutSpecCollectorstill uses the oldreflectorfield — this will cause a compilation failure and block CI.
📄 Documentation Diagram
This diagram documents the refactored spec collection process using oaswrap/spec.
sequenceDiagram
participant DSL as DSL
participant SC as SpecCollector
participant Router as spec.Router
participant Doc as openapi.Document
participant WS as WriteSpec
DSL->>SC: RegisterDSLOperation
SC->>Router: reflect route
Router-->>Doc: merge generated operation
note over SC: PR #35;2 replaced swaggest/openapi-go<br/>with oaswrap/spec
WS->>Doc: serialize to YAML/JSON
💡 Suggestions (P2)
- go.mod: The PR removes the swaggest/openapi-go dependency but leaves its imports intact in 7 other files — this will prevent the module from building and must be cleaned up as part of the migration.
📈 Risk Diagram
This diagram illustrates the risk of incomplete library migration leading to compilation failure.
sequenceDiagram
participant Test as builder_test
participant SC as SpecCollector
Test->>SC: check sc.doc.Paths["/test"]
note over Test: R1(P0): sc.doc is undefined
SC--xTest: compilation error
⚠️ **Unanchored Suggestions (Manual Review Recommended)**
The following suggestions could not be precisely anchored to a specific line in the diff. This can happen if the code is outside the changed lines, has been significantly refactored, or if the suggestion is a general observation. Please review them carefully in the context of the full file.
📁 File: go.mod
Speculative: The PR removes the swaggest/openapi-go dependency from go.mod but leaves its imports and usages intact in numerous core files (spec.go, spec_dsl.go, internal/output/output_sanitize.go, spec_security.go, spec_params.go, spec_schemas.go, validate_extra_test.go). This inconsistent state will prevent the entire module from building. The root cause is the same incomplete migration noted in the P0 finding, but here the evidence is only visible in unchanged files, so the priority is downgraded per the gate rules. The overall migration cannot succeed until all import paths and type references are updated to use oaswrap/spec instead of swaggest/openapi-go.
Suggestion:
Replace all remaining imports of `github.com/swaggest/openapi-go/openapi3` with the equivalent types from `github.com/oaswrap/spec` and update every usage of `sc.reflector` to use the new `doc` document model.
Related Code:
- github.com/swaggest/jsonschema-go v0.3.79
- github.com/swaggest/openapi-go v0.2.60
- github.com/swaggest/refl v1.4.0
💡 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.
|
|
||
| // spec should have registered path | ||
| if _, ok := sc.reflector.Spec.Paths.MapOfPathItemValues["/test"]; !ok { | ||
| if _, ok := sc.doc.Paths["/test"]; !ok { |
There was a problem hiding this comment.
P0 | Confidence: High
The PR changes the unit test to reference sc.doc.Paths["/test"], but the SpecCollector struct in spec.go still defines the old reflector field and does not introduce a doc field. The related context (spec.go) confirms no doc field exists. This direct change in a test file will cause a compilation error because sc.doc is undefined. The migration to the new library is incomplete; the core data structure and logic have not been updated to match the test expectations.
Code Suggestion:
Revise `spec.go` to replace `reflector` with a `doc` field (e.g., `doc *openapi.Document`) and refactor all internal methods accordingly. For the test, the check should align with the actual internal structure.Evidence: path:spec.go
No description provided.