feat: adjust reflection#62
Merged
Merged
Conversation
…h reflection edge cases - Add InterceptPropFunc and InterceptPropParams to ReflectorConfig; hook called pre/post per struct field in StructSchema (return ErrSkipProperty to skip) - Add InterceptSchemaFunc and InterceptSchemaParams to ReflectorConfig; hook called pre/post per type in SchemaForType (inline/primitive) and RefSchema (components); pre-call stop=true overrides default schema generation - Add RequiredPropByValidateTag option: marks fields required when validate tag contains "required"; configurable tag name and separator - Fix sanitizeDefName: skip package prefix for types from "main" package - Fix uint32 and uint format from int32 to int64 (uint32 max 4.3B exceeds int32 max 2.1B) - Fix RefSchema pre-hook: assign StructSchema fields onto existing pointer so pre-hook customizations (e.g. Extensions, Description) survive to post-hook - Fix ApplyNullable (3.1+): merge "null" into existing []string type instead of silently skipping when schema.Type is already a multi-type slice - Add tests for all new hooks, RequiredPropByValidateTag, and patched edge cases
- Fix Reflected Go Types table: uint8/uint16 use int32, uint/uint32/uint64/uintptr use int64 - Add InterceptSchema, InterceptProp, RequiredPropByValidateTag to Reflector Configuration section with usage examples and option table - Add new hooks to Features list
![llamapreview[bot]](https://avatars.githubusercontent.com/in/1023541?s=60&v=4){kind=small}
There was a problem hiding this comment.
AI Code Review by LlamaPReview
🎯 TL;DR & Recommendation
Recommendation: Request Changes
This PR adds powerful schema/property interception hooks and a RequiredPropByValidateTag option, but introduces a critical breaking change in the format of uint/uint32 types and a duplicate required field bug that can produce invalid OpenAPI specs.
📄 Documentation Diagram
This diagram documents the new schema and property interception hooks flow.
sequenceDiagram
participant Caller as Spec Router
participant Reflector as Reflector
participant SchemaHook as InterceptSchema
participant PropHook as InterceptProp
Caller->>Reflector: SchemaForType(t, mode, field)
Reflector->>SchemaHook: Pre-call (Processed=false)
alt stop==true
SchemaHook-->>Reflector: Use provided schema<br/>Skip default processing
else stop==false
Reflector->>Reflector: Generate schema (inline/component)
Reflector->>SchemaHook: Post-call (Processed=true)<br/>Modify final schema
end
Reflector->>PropHook: For each field: Pre-call (Processed=false)<br/>Return ErrSkipProperty to skip
alt skipped
PropHook-->>Reflector: Skip field
else not skipped
Reflector->>Reflector: Generate property schema
Reflector->>PropHook: Post-call (Processed=true)<br/>Modify or skip property<br/>Note: RequiredPropByValidateTag uses post-hook
end
Reflector->>Caller: Return final schema
🌟 Strengths
- Solid test coverage for new interception hooks, including pre/post call order and component scenarios.
⚡ Key Risks & Improvements (P1)
- option/reflector.go:
RequiredPropByValidateTagcan add duplicate entries to therequiredarray when combined with existingrequiredstruct tags, producing semantically invalid specs that may fail consumer validation. - internal/reflect/converter.go: Changing
uint/uint32format fromint32toint64is a breaking change for downstream consumers; this should be documented as a major version change and a migration path considered.
📈 Risk Diagram
This diagram illustrates the risk of breaking existing consumers due to the uint/uint32 format change.
sequenceDiagram
participant Upstream as Library vPrevious
participant Consumer as Existing Clients
participant Lib as Library vNew (PR)
Consumer->>Upstream: Generate spec with uint32<br/>Expected format: int32
Upstream-->>Consumer: uint32 format: int32
note over Consumer,Upstream: R1(P1): uint/uint32 format changed<br/>from int32 to int64<br/>Schemas now emit format: int64
Consumer->>Lib: Regenerate spec after upgrade
Lib-->>Consumer: uint32 format: int64
note over Consumer: Client stubs/code gen/validators<br/>may fail due to format mismatch
💡 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! |
…opagation - Chain InterceptSchema and InterceptProp hooks instead of overwriting previous ones - Propagate errors from all hook calls through SchemaForType, StructSchema, RefSchema, SchemaForValue, RequestParts, ParameterSchema, AddRequest, and AddResponse - Add pre-hook coverage for SchemaExposer types in both SchemaForType and SchemaForValue - Clean up r.Components and r.Generating on pre-hook error in RefSchema to prevent stale empty components blocking retry - Add uniqueStrings deduplication on Required to prevent duplicates when both required struct tag and RequiredPropByValidateTag hook are used - Add parentSnapshot restore on ErrSkipProperty in post-hooks to roll back parent schema mutations (AllOf, AnyOf, OneOf, Extensions, Extra) - Pass ParentType to InterceptPropParams for richer hook context
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Type of Change
Checklist
make checkpasses locally (sync + tidy + lint + test)make test-update)