Implement builder package for programmatic OpenAPI specification construction#52
Implement builder package for programmatic OpenAPI specification construction#52
Conversation
Co-authored-by: erraggy <366249+erraggy@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request implements a comprehensive builder package for programmatic OpenAPI specification construction with reflection-based schema generation. The implementation provides a fluent API that automatically converts Go types to OpenAPI schemas, supporting struct tags for fine-grained control and handling complex scenarios like circular references and embedded structs.
Key Changes
- Reflection engine: Automatic schema generation from Go types with caching and circular reference detection
- Fluent builder API: Chain-able methods for constructing complete OAS documents programmatically
- Struct tag support:
jsonandoastags for customizing schema generation with validation constraints
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
builder/builder.go |
Core Builder struct with info setters, Build/Marshal methods, and document construction |
builder/reflect.go |
Type-to-Schema reflection engine with special type handling (time.Time, uuid.UUID) |
builder/reflect_cache.go |
Schema caching mechanism with circular reference detection |
builder/tags.go |
JSON and OAS struct tag parsing and application logic |
builder/operation.go |
Operation builder with parameter/response/request body options |
builder/server.go |
Server and server variable configuration |
builder/security.go |
Security scheme builders and tag management |
builder/response.go |
Reusable response components |
builder/parameter.go |
Reusable parameter components |
builder/doc.go |
Package documentation with examples |
builder/*_test.go |
Comprehensive test suite with 85+ tests |
builder/builder_bench_test.go |
Performance benchmarks (contains critical bug) |
builder/example_test.go |
Example-based documentation tests |
README.md |
Updated documentation with builder package examples |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Copy slices (shallow reference) | ||
| result.Enum = s.Enum | ||
| result.Required = s.Required | ||
| result.Examples = s.Examples | ||
| result.AllOf = s.AllOf | ||
| result.AnyOf = s.AnyOf | ||
| result.OneOf = s.OneOf | ||
|
|
||
| // Copy maps (shallow reference) | ||
| result.Properties = s.Properties | ||
| result.PatternProperties = s.PatternProperties | ||
| result.DependentRequired = s.DependentRequired | ||
| result.DependentSchemas = s.DependentSchemas |
There was a problem hiding this comment.
The copySchema function performs a shallow copy of slices and maps (lines 214-226). This is documented as intentional, but it could lead to unexpected mutations when the copy is modified. For example, if applyOASTag modifies the Enum slice (line 112 in applyOASTag), it creates a new slice which is fine. However, if someone directly modifies result.Properties or result.Required, it would affect the original schema.
Consider doing a deep copy for at least the Enum and Required slices since applyOASTag already creates new slices for Enum. For consistency and safety, consider deep copying these mutable fields:
// Deep copy slices that might be modified
if s.Enum != nil {
result.Enum = make([]any, len(s.Enum))
copy(result.Enum, s.Enum)
}
if s.Required != nil {
result.Required = make([]string, len(s.Required))
copy(result.Required, s.Required)
}There was a problem hiding this comment.
Applied deep copy for Enum and Required slices, as well as all pointer fields for consistency and safety. See commit 6d08379.
| // Copy pointers (shallow) | ||
| result.Minimum = s.Minimum | ||
| result.Maximum = s.Maximum | ||
| result.MinLength = s.MinLength | ||
| result.MaxLength = s.MaxLength | ||
| result.MinItems = s.MinItems | ||
| result.MaxItems = s.MaxItems | ||
| result.MinProperties = s.MinProperties | ||
| result.MaxProperties = s.MaxProperties | ||
| result.MultipleOf = s.MultipleOf | ||
| result.ExclusiveMaximum = s.ExclusiveMaximum | ||
| result.ExclusiveMinimum = s.ExclusiveMinimum |
There was a problem hiding this comment.
The shallow copy of pointer fields (lines 201-212) could lead to unintended sharing of pointer values. If applyOASTag modifies these pointer fields (e.g., at lines 118-134), it would affect both the original and copied schema since they share the same pointer.
Currently applyOASTag creates new pointers (e.g., line 119: result.Minimum = &f), so this is safe. However, this coupling creates a fragile dependency - if applyOASTag is later modified to reuse pointers, bugs would be introduced.
For better safety and maintainability, consider deep copying the pointer fields:
// Deep copy pointer fields
if s.Minimum != nil {
minCopy := *s.Minimum
result.Minimum = &minCopy
}
if s.Maximum != nil {
maxCopy := *s.Maximum
result.Maximum = &maxCopy
}
// ... repeat for other pointer fields| // Copy pointers (shallow) | |
| result.Minimum = s.Minimum | |
| result.Maximum = s.Maximum | |
| result.MinLength = s.MinLength | |
| result.MaxLength = s.MaxLength | |
| result.MinItems = s.MinItems | |
| result.MaxItems = s.MaxItems | |
| result.MinProperties = s.MinProperties | |
| result.MaxProperties = s.MaxProperties | |
| result.MultipleOf = s.MultipleOf | |
| result.ExclusiveMaximum = s.ExclusiveMaximum | |
| result.ExclusiveMinimum = s.ExclusiveMinimum | |
| // Deep copy pointer fields | |
| if s.Minimum != nil { | |
| minCopy := *s.Minimum | |
| result.Minimum = &minCopy | |
| } | |
| if s.Maximum != nil { | |
| maxCopy := *s.Maximum | |
| result.Maximum = &maxCopy | |
| } | |
| if s.MinLength != nil { | |
| minLenCopy := *s.MinLength | |
| result.MinLength = &minLenCopy | |
| } | |
| if s.MaxLength != nil { | |
| maxLenCopy := *s.MaxLength | |
| result.MaxLength = &maxLenCopy | |
| } | |
| if s.MinItems != nil { | |
| minItemsCopy := *s.MinItems | |
| result.MinItems = &minItemsCopy | |
| } | |
| if s.MaxItems != nil { | |
| maxItemsCopy := *s.MaxItems | |
| result.MaxItems = &maxItemsCopy | |
| } | |
| if s.MinProperties != nil { | |
| minPropsCopy := *s.MinProperties | |
| result.MinProperties = &minPropsCopy | |
| } | |
| if s.MaxProperties != nil { | |
| maxPropsCopy := *s.MaxProperties | |
| result.MaxProperties = &maxPropsCopy | |
| } | |
| if s.MultipleOf != nil { | |
| multOfCopy := *s.MultipleOf | |
| result.MultipleOf = &multOfCopy | |
| } | |
| if s.ExclusiveMaximum != nil { | |
| exclMaxCopy := *s.ExclusiveMaximum | |
| result.ExclusiveMaximum = &exclMaxCopy | |
| } | |
| if s.ExclusiveMinimum != nil { | |
| exclMinCopy := *s.ExclusiveMinimum | |
| result.ExclusiveMinimum = &exclMinCopy | |
| } |
Co-authored-by: erraggy <366249+erraggy@users.noreply.github.com>
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #52 +/- ##
==========================================
+ Coverage 62.18% 66.52% +4.34%
==========================================
Files 33 42 +9
Lines 6720 7795 +1075
==========================================
+ Hits 4179 5186 +1007
- Misses 2031 2070 +39
- Partials 510 539 +29 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@copilot please address the code coverage feedback posted in the previous comment. |
I don't see the code coverage feedback in the visible comments. Could you please point me to the specific feedback or repost it here? |
|
@copilot I have quote replied the code coverage feedback comment below. Please address.
|
|
@copilot please try again |
Co-authored-by: erraggy <366249+erraggy@users.noreply.github.com>
Improved test coverage from 81.5% to 96.0%. Added tests for:
See commit f24c30d. |
|
@copilot It appears that the builder will always target OAS 3.x base URIs in the |
Co-authored-by: erraggy <366249+erraggy@users.noreply.github.com>
|
@copilot please try to pick up where you left off when my account was rate limited. Also, please keep in mind that the |
Co-authored-by: erraggy <366249+erraggy@users.noreply.github.com>
Fixed the compilation errors in example_test.go and continued from where work was interrupted. The builder now provides:
All tests pass with 95.0% coverage. See commit 1c050c7. |
Code Review: Builder Package (PR #52)This is a comprehensive code review of the new builder package for programmatic OpenAPI specification construction. SummaryThis PR introduces a well-designed builder package that enables programmatic construction of OpenAPI specifications using Go's reflection system. Overall, this is high-quality code that follows the project's conventions and provides a valuable addition to the oastools toolkit. ✅ StrengthsArchitecture & Design
Code Quality
Reflection Engine
Security & Safety
🔍 Issues FoundMinor Issue: Unused Method (Informational)Location: // reflect_cache.go
func (c *schemaCache) hasName(name string) bool {
_, exists := c.byName[name]
return exists
}Issue: The Recommendation: Either remove this unused method, or if it's intended for future use, add a test that exercises it or mark it with a comment explaining planned usage. Per CLAUDE.md guidelines, "Avoid backwards-compatibility hacks like renaming unused Potential Enhancement: FromDocument OAS 2.0 SupportLocation: func FromDocument(doc *parser.OAS3Document) *Builder {Observation: Recommendation: Consider adding Edge Case: Empty operations checkLocation: // Build responses object
var responses *parser.Responses
if len(cfg.responses) > 0 {
responses = &parser.Responses{
Codes: make(map[string]*parser.Response),
}Observation: When building an operation, if no responses are specified, the Recommendation: Consider adding validation that warns when an operation has no responses defined, since this would fail OAS validation. ⚡ Performance AssessmentThe benchmark results show excellent performance:
Assessment: Performance is appropriate for typical API generation use cases. JSON marshaling is ~2x faster than YAML, which is expected. 🔒 Security Assessment
📊 Test Coverage AnalysisCoverage: 95.0% of statements Test file analysis:
Observation: Test coverage is comprehensive and follows the project's requirement for testing all exported functionality. 📝 Minor Suggestions (Non-blocking)
ConclusionRecommendation: Approve with minor fix This is a well-implemented, well-tested, and well-documented package that follows project conventions. The only action item is removing the unused The builder package provides a valuable addition for users who want to programmatically generate OpenAPI specifications in Go, with excellent reflection-based schema generation and type-safe OAS version handling. 🤖 Generated with Claude Code |
|
@copilot please address every issue and implement all suggestions posted in the most recent comment to this PR (#52: #52 (comment)) |
…ExternalDocs, AddWebhook Co-authored-by: erraggy <366249+erraggy@users.noreply.github.com>
Addressed all issues and implemented all suggestions from the code review in commit 9046384: Issues Fixed:
Enhancements Added:
Documentation Updates:
|
Co-authored-by: erraggy <366249+erraggy@users.noreply.github.com>
erraggy
left a comment
erraggy
left a comment
There was a problem hiding this comment.
I have requested changes in in-line comments throughout, please address all @copilot.
…alidation Co-authored-by: erraggy <366249+erraggy@users.noreply.github.com>
Addressed all issues in commit d1cf2d1:
All tests pass with 94.8% coverage. |
Co-authored-by: erraggy <366249+erraggy@users.noreply.github.com>
Fixes two documentation issues: 1. README.md line 158: Updated builder example to use BuildOAS3() instead of the removed Build() method. The Build() method was removed in PR #52 in favor of type-specific BuildOAS2() and BuildOAS3() methods. 2. doc.go: Updated package count from five to six packages to include the new builder package added in v1.12.0. These changes ensure documentation accuracy on pkg.go.dev and in the README.
Fixes two documentation issues: 1. README.md line 158: Updated builder example to use BuildOAS3() instead of the removed Build() method. The Build() method was removed in PR #52 in favor of type-specific BuildOAS2() and BuildOAS3() methods. 2. doc.go: Updated package count from five to six packages to include the new builder package added in v1.12.0. These changes ensure documentation accuracy on pkg.go.dev and in the README.
Fixes two documentation issues: 1. README.md line 158: Updated builder example to use BuildOAS3() instead of the removed Build() method. The Build() method was removed in PR #52 in favor of type-specific BuildOAS2() and BuildOAS3() methods. 2. doc.go: Updated package count from five to six packages to include the new builder package added in v1.12.0. These changes ensure documentation accuracy on pkg.go.dev and in the README.
planning/implement-builder.mdbuilder/doc.gowith package documentationBuilderstruct andNew()function inbuilder.goreflect.go) - primitive types, structs, slices, maps, pointersreflect_cache.go)jsontag parsing for field names and omitemptyoastag parsing (tags.go) - description, format, enum, constraintsAddOperation()with reflection-based optionsWithResponse(),WithRequestBody(),WithQueryParam(),WithPathParam(),WithHeaderParam()AddServer(),ServerOption)AddTag(),TagOption)BuildOAS2(),BuildOAS3(),BuildResult(),WriteFile(),MarshalYAML(),MarshalJSON()FromDocument()for existing document integrationAddResponseWithContentTypewithWithResponseContentTypeoptionresponseConfig,ResponseOptionfuncs toresponse.goparamConfig,ParamOptionfuncs toparameter.gotagConfig,TagOptionfuncs totags.goBuild()that returnsany(useBuildOAS2()/BuildOAS3())NewOAS2/NewOAS3in favor of singleNew()constructorvalidatorpackage instead)SchemaFrom/SchemaFromTypefunctionsgenerateStructSchemafor embedded structsOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.