feat!: implemented preprocess and removed preTransforms#145
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis update removes all pre-transform functionality from the schema processing and validation system. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant PreprocessSchema
participant PreprocessFn
participant UnderlyingSchema
User->>PreprocessSchema: Parse(input, &output)
PreprocessSchema->>PreprocessFn: preprocess(input, ctx)
PreprocessFn-->>PreprocessSchema: transformed, error?
alt error
PreprocessSchema-->>User: Return issue
else success
PreprocessSchema->>UnderlyingSchema: process(transformed, &output)
UnderlyingSchema-->>PreprocessSchema: issues
PreprocessSchema-->>User: Return issues
end
Possibly related PRs
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Oudwins
changed the title
Deploying zog with
|
| Latest commit: |
0cf2e1e
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://12597a2a.zog-3a0.pages.dev |
| Branch Preview URL: | https://feat-preprocess.zog-3a0.pages.dev |
Oudwins
force-pushed
the
feat/preprocess
branch
from
b1939b4 to
8c06210
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 12
🔭 Outside diff range comments (5)
custom.go (1)
3-9: 🛠️ Refactor suggestion
reflectimport is now required
process(see below) is switching to reflection; however the import block currently brings in onlyfmt.
Addreflectto avoid a compilation error after the fix proposed next.import ( "fmt" + "reflect" "github.com/Oudwins/zog/conf"pointers.go (1)
82-93: 🛠️ Refactor suggestionNil‑allocation branch may panic on non‑pointer
ValPtr
reflect.ValueOf(ctx.ValPtr).Elem()assumesctx.ValPtritself is a pointer.
If a caller accidentally passes a non‑pointer (runtime error paths, legacy code), this will panic.Safeguard with an early check:
rv := reflect.ValueOf(ctx.ValPtr) if rv.Kind() != reflect.Ptr { ctx.AddIssue(ctx.IssueFromUnknownError(fmt.Errorf("expected pointer, got %s", rv.Kind()))) return } destPtr := rv.Elem()Failing fast avoids crashing the validation pipeline.
internals/contexts.go (1)
132-141:⚠️ Potential issuePool object retains user data – clear
Data/ValPtrbefore re‑use
SchemaCtx.Free()(line 207) puts the context back into async.Pool, but the newly‑addedDataandValPtrfields are never reset.
Large objects referenced here will therefore be held alive indefinitely, causing surprising memory retention.func (c *SchemaCtx) Free() { + // Avoid leaking references across pooled instances + c.Data = nil + c.ValPtr = nil + c.Path = nil + c.Test = nil SchemaCtxPool.Put(c) }slices.go (1)
84-90:⚠️ Potential issuePossible nil‑pointer panic when validating
reflect.ValueOf(ctx.ValPtr).Elem()will panic if the caller passednilfor the slice pointer (perfectly legal for an optional field).
Guard against this before dereferencing:-refVal := reflect.ValueOf(ctx.ValPtr).Elem() +if ctx.ValPtr == nil { + // treat nil the same as zero‑length slice + if v.required != nil { + ctx.AddIssue(ctx.IssueFromTest(v.required, nil)) + } + return +} +refVal := reflect.ValueOf(ctx.ValPtr).Elem()struct.go (1)
105-110: 🛠️ Refactor suggestionUnsafe upper‑casing logic and fixed‑size buffer
- The ASCII‑only subtraction (
b[0] -= 32) silently mis‑handles non‑ASCII field names (e.g. “mañana”).- Copying into a
[32]bytearray truncates keys longer than 32 bytes, causing undefined behaviour when the struct contains longer field names.Prefer a safer approach:
- var b [32]byte - copy(b[:], key) - b[0] -= 32 - key = string(b[:len(key)]) + r := []rune(key) + r[0] = unicode.ToUpper(r[0]) + key = string(r) + // import "unicode" at the topThis eliminates the length cap and handles Unicode correctly.
♻️ Duplicate comments (1)
struct.go (1)
161-170: See earlier note: mismatch in post‑transform receiver
validate()still passesctx.Datahere. Apply the same change suggested above to keep behaviour consistent.
🧹 Nitpick comments (11)
docs/docs/NOTES_COMMON_MISTAKES.md (1)
1-29: Useful documentation on common schema usage mistakes.This documentation is valuable for guiding users on proper schema usage, highlighting important points about immutability and reuse. The examples effectively illustrate the difference between optional and defined schemas.
There are a few minor issues to address:
- Line 13 is missing a period at the end of the bullet point.
- Line 28-29 appears to be an incomplete thought that ends abruptly. Consider removing it or completing the explanation about how "zog" behaves differently.
-Using the data after parsing even though there were errors +Using the data after parsing even though there were errors. -that is another footgun. That this isn't how it works in zog +that is another footgun. This behavior differs from zog which [complete explanation here].🧰 Tools
🪛 LanguageTool
[uncategorized] ~13-~13: A period might be missing here.
Context: ...ta after parsing even though there were errors --- Composition and Reuse Schema are ...(AI_EN_LECTOR_MISSING_PUNCTUATION_PERIOD)
string_test.go (1)
45-56: Commented out test for removed functionality.The
TestStringTrimtest has been commented out rather than deleted, which is consistent with the removal of pre-transform functionality across the codebase. Since theTrimmethod has been removed or commented out in the main code, this test is no longer applicable.Consider completely removing the commented test code rather than leaving it commented out. Commented code can lead to confusion and maintenance issues in the future. If you need to reference this implementation later, the git history will preserve it.
-// func TestStringTrim(t *testing.T) { -// field := String().Required().Trim() -// var dest string -// -// errs := field.Parse(" foo ", &dest) -// assert.Empty(t, errs) -// assert.Equal(t, "foo", dest) -// -// errs = field.Parse(123, &dest) -// assert.Empty(t, errs) -// assert.Equal(t, "123", dest) -// }time.go (1)
57-57: Commented out code should be addressedThe call to
setCoercerhas been commented out rather than properly removed. Consider either removing this commented line completely or adding a proper comment explaining why it's being retained for future reference.- // s.setCoercer(conf.TimeCoercerFactory(format)) + // Function no longer sets coercer - removed as part of pre-transform eliminationstring_validate_test.go (1)
46-58: Dead‑code: remove or migrate the commented‑outTrimtestThe
TestValidateStringTrimblock has been fully commented out.
Leaving large chunks of commented code in the test suite quickly becomes stale and makes it hard to see real coverage gaps.Actionable options
1. Delete the block completely (preferred – Git history keeps it).
2. If the intent is to replaceTrimwith the newPreprocessSchemaequivalent, add a new test that exercises that path instead of commenting this one out.Either way, keeping the file free of commented‑out tests improves maintainability and prevents confusion about expected behaviour.
pointers.go (2)
62-75: Side‑effect onctx.Datarisks losing original input
ctx.Data = valwrites the dereferenced factory output back into the parent context.
If downstream logic relies on the original data value for error reporting, this mutation may hide useful information (e.g. inIssueFromUnknownErroror later coercion checks).Consider:
- ctx.Data = val + // Keep original for debugging; store resolved value only in sub‑context. + subCtxData := valand pass
subCtxDatato the sub‑context instead of mutating the parent.
That preserves the source value while still feeding the resolved one to the schema.
112-118: Error type reported with transformed context dataSimilar to the earlier note,
IssueFromTest(v.required, ctx.Data)now usesctx.Data, which for validate paths is the pointer value (**T).
Consider passing the dereferenced form so error messages are meaningful to the user.- ctx.AddIssue(ctx.IssueFromTest(v.required, ctx.Data).SetDType(v.schema.getType())) + ctx.AddIssue(ctx.IssueFromTest(v.required, destPtr.Interface()).SetDType(v.schema.getType()))string.go (1)
102-115: Remove stale TODO / commented‑out pre‑transform blockThe whole pre‑transform feature is gone, but the commented code and TODO remain, which may confuse future readers.
-// TODO -// PreTransform: trims the input data of whitespace if it is a string -// func (v *StringSchema[T]) Trim() *StringSchema[T] { -// ... -// }Consider deleting this block entirely or replacing it with guidance on preferred post‑transform usage.
preprocess.go (1)
41-48: Pointer‑handling switch is incomplete & can silently corrupt dataThe current switch only caters for
*Tand**T.
If the schema appears nested (e.g.,***Tafter several preprocessing layers) the default branch panics, breaking validation flows that previously worked.Consider generalising:
for rv := reflect.ValueOf(ctx.ValPtr); rv.Kind() == reflect.Ptr; rv = rv.Elem() { if rv.IsNil() { rv.Set(reflect.New(rv.Type().Elem())) } if rv.Elem().CanSet() { rv.Elem().Set(reflect.ValueOf(out)) break } }or document clearly that only single‑/double‑pointer forms are supported.
preprocess_test.go (1)
73-76: Inconsistent error assertions risk false positivesHere you use
assert.Nil(t, errs)while earlier tests checklen(errs) == 0. IfParsereturns a non‑nil, zero‑length slice, this assertion will fail even though no issues exist.For consistency and robustness use:
assert.Zero(t, len(errs))or add a helper
assertNoIssues(t, errs)for all tests.preprocess_validate_test.go (2)
265-272: Returning a pointer to a local slice header risks accidental bugsYou create a copy (
s := **data) and return&s. Although the slice header escapes to the heap, you lose the link with the original variable and add an extra level of indirection. You can mutate in‑place and return the original pointer:- s := **data - for i, str := range s { - s[i] = str + "!" - } - return &s, nil + for i := range **data { + (**data)[i] += "!" + } + return *data, nilThis avoids the heap escape and keeps the pointer structure unchanged.
11-42: Inconsistent nil‑vs‑len(…)==0 assertionsSome tests use
assert.Equal(t, 0, len(errs)), othersassert.Nil(t, errs). Both are valid but mixing styles reduces readability. Consider standardising (preferassert.Nilif library guarantees a nil slice on no errors).Also applies to: 55-63, 66-75, 97-123, 125-154, 156-172, 197-206, 208-220, 236-248, 250-263, 281-306
🛑 Comments failed to post (12)
slices_validate_test.go (1)
53-73: 🛠️ Refactor suggestion
Commented test should be removed or replaced
Instead of commenting out the test for SlicePreTransform, it would be better to either remove it completely or replace it with an equivalent test using the new approach.
-// func TestValidateSlicePreTransform(t *testing.T) { -// preTransform := func(val any, ctx Ctx) (any, error) { -// if v, ok := val.([]string); ok { -// out := make([]string, len(v)) -// copy(out, v) -// for i := range out { -// out[i] = strings.ToUpper(out[i]) -// } -// return out, nil -// } -// return val, nil -// } - -// validator := Slice(String()).PreTransform(preTransform) -// dest := []string{"test", "example"} -// errs := validator.Validate(&dest) -// if len(errs) > 0 { -// t.Errorf("Expected no errors, got %v", errs) -// } -// assert.Equal(t, []string{"TEST", "EXAMPLE"}, dest) -// }If you want to keep testing this functionality, consider adding a replacement using the new approach (e.g., using PreprocessSchema, if appropriate).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.custom.go (1)
46-52:
⚠️ Potential issueCompile‑time error: cannot use type assertion to a type parameter
d, ok := ctx.Data.(T)is illegal – the Go spec forbids asserting to a type parameter.
The file will not compile.A reflection‑based coercion avoids this limitation and gives a clearer error when the incoming value is of the wrong type.
- d, ok := ctx.Data.(T) - if !ok { - ctx.AddIssue(ctx.IssueFromCoerce(fmt.Errorf("expected %T, got %T", new(T), ctx.Data))) - return - } - ptr := ctx.ValPtr.(*T) - *ptr = d + ptr, ok := ctx.ValPtr.(*T) + if !ok { + ctx.AddIssue(ctx.IssueFromCoerce(fmt.Errorf("internal error: expected *T destination, got %T", ctx.ValPtr))) + return + } + + // Verify assignability of ctx.Data → *ptr using reflection. + var zero T + want := reflect.TypeOf(zero) + gotVal := reflect.ValueOf(ctx.Data) + if !gotVal.IsValid() || !gotVal.Type().AssignableTo(want) { + ctx.AddIssue(ctx.IssueFromCoerce(fmt.Errorf("expected value of type %v, got %T", want, ctx.Data))) + return + } + ptr.Elem().Set(gotVal.Convert(want))This resolves the compilation error and produces a precise coercion issue when types mismatch.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.ptr, ok := ctx.ValPtr.(*T) if !ok { ctx.AddIssue(ctx.IssueFromCoerce( fmt.Errorf("internal error: expected *T destination, got %T", ctx.ValPtr), )) return } // Verify assignability of ctx.Data → *ptr using reflection. var zero T want := reflect.TypeOf(zero) gotVal := reflect.ValueOf(ctx.Data) if !gotVal.IsValid() || !gotVal.Type().AssignableTo(want) { ctx.AddIssue(ctx.IssueFromCoerce( fmt.Errorf("expected value of type %v, got %T", want, ctx.Data), )) return } ptrVal := reflect.ValueOf(ptr).Elem() ptrVal.Set(gotVal.Convert(want))slices.go (1)
189-191:
⚠️ Potential issueSame nil‑pointer risk during parsing
destVal := reflect.ValueOf(ctx.ValPtr).Elem()will also panic ifdestis a nil pointer supplied by the caller. Apply a similar guard here.-if ctx.ValPtr == nil { - ctx.AddIssue(ctx.Issue().SetMessage("destination slice pointer is nil")) - return -} -destVal := reflect.ValueOf(ctx.ValPtr).Elem() +if ctx.ValPtr == nil { + ctx.AddIssue(ctx.Issue().SetMessage("destination slice pointer is nil")) + return +} +destVal := reflect.ValueOf(ctx.ValPtr).Elem()Committable suggestion skipped: line range outside the PR's diff.
struct.go (1)
70-77: 🛠️ Refactor suggestion
Inconsistent receiver passed to post‑transformers between Parse and Validate
process()correctly invokes post‑transformers withctx.ValPtr, whereasvalidate()(line 166) passesctx.Data.
Although both currently reference the same pointer, this coupling is accidental and will break as soon asValidate()is called with a non‑identicaldata/destPtrpair (or ifStructSchema.Validateis ever changed to wrap input providers the wayprocessdoes).Recommend unifying the two call‑sites:
- err := fn(ctx.Data, ctx) + err := fn(ctx.ValPtr, ctx)This keeps the contract of
PostTransformstable (always operates on the destination value) and prevents subtle future regressions.Committable suggestion skipped: line range outside the PR's diff.
preprocess.go (2)
35-48:
⚠️ Potential issue
Validatepath uses the wrong value for preprocessing
ctx.ValPtris a pointer toT, yet the preprocessing function expects anF(often the non‑pointer form).
The current type assertion fails wheneverFis not a pointer, making.Validateunusable for most practical cases.- out, err := s.fn(ctx.ValPtr.(F), ctx) + vAny := p.UnwrapPtr(ctx.ValPtr) + v, ok := vAny.(F) + if !ok { + ctx.AddIssue(ctx.IssueFromCoerce( + fmt.Errorf("preprocess expected %T but got %T", *new(F), vAny))) + return + } + out, err := s.fn(v, ctx)Using
internals.UnwrapPtrmirrors theprocesspath and guarantees the function receives the intended value.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func (s *PreprocessSchema[F, T]) validate(ctx *p.SchemaCtx) { // unwrap any pointer layers so we can assert to F vAny := p.UnwrapPtr(ctx.ValPtr) v, ok := vAny.(F) if !ok { ctx.AddIssue(ctx.IssueFromCoerce( fmt.Errorf("preprocess expected %T but got %T", *new(F), vAny))) return } out, err := s.fn(v, ctx) if err != nil { ctx.AddIssue(ctx.Issue().SetMessage(err.Error())) return } switch dest := ctx.ValPtr.(type) { case *T: *dest = out case **T: *dest = &out default: panic(fmt.Sprintf( "Preprocessed should be passed in schema.Validate() a value pointer "+ "that is compatible with its returned type T. Either *T or **T. Got %T", dest)) } }
22-26:
⚠️ Potential issueMissing early‑exit after type‑assertion failure leads to panics
If
ctx.Datacannot be asserted to typeF, the code records an issue but still proceeds to call the preprocessing function with the zero‑value ofF, which is almost certainly incorrect and may panic.if !ok { ctx.AddIssue(ctx.IssueFromCoerce( fmt.Errorf("preprocess expected %T but got %T", *new(F), ctx.Data))) -} + return // stop processing on type mismatch +}Fail‑fast prevents unexpected behaviour further down the pipeline.
Committable suggestion skipped: line range outside the PR's diff.
preprocess_validate_test.go (3)
188-194:
⚠️ Potential issueTest disabled by commented assertions – either re‑enable or mark as skipped
The test currently provides no safety net:
_ = errs // assert.Nil(t, errs) // assert.Equal(t, User{...}, user)If the test is still flaky, use
t.Skip("explain reason"); otherwise restore the assertions so CI actually validates behaviour.
45-53: 🛠️ Refactor suggestion
Use tolerant float comparison to avoid brittle tests
assert.Equalperforms a strict bit‑for‑bit comparison; small IEEE‑754 rounding errors will make this test flaky on some architectures/Go versions. Preferassert.InDelta(orassert.InEpsilon) with a tiny epsilon.-assert.Equal(t, 124.45, f) +assert.InDelta(t, 124.45, f, 1e-9)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.s := Preprocess(func(data *float64, ctx Ctx) (out float64, err error) { return *data + 1.0, nil }, Float64().GT(0)) f := 123.45 errs := s.Validate(&f) assert.Equal(t, 0, len(errs)) assert.InDelta(t, 124.45, f, 1e-9) }
223-234: 🛠️ Refactor suggestion
Same float‑comparison issue here
The pointer‑based variant has the identical risk. Replace the two
assert.Equalcalls withassert.InDelta.-assert.Equal(t, 3.24, *pf) -assert.Equal(t, 3.24, f) +assert.InDelta(t, 3.24, *pf, 1e-9) +assert.InDelta(t, 3.24, f, 1e-9)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.s := Preprocess(func(data **float64, ctx Ctx) (out *float64, err error) { **data += 0.1 return *data, nil }, Ptr(Float64().GT(0))) f := 3.14 pf := &f errs := s.Validate(&pf) assert.Nil(t, errs) - assert.Equal(t, 3.24, *pf) - assert.Equal(t, 3.24, f) + assert.InDelta(t, 3.24, *pf, 1e-9) + assert.InDelta(t, 3.24, f, 1e-9) }zogSchema.go (3)
124-127:
⚠️ Potential issueSame nil / wrong‑type risk for
valPtrin validatorMirror the defensive cast suggested for
primitiveProcessorto avoid runtime panics here as well.
56-60:
⚠️ Potential issuePotential panic when
ctx.ValPtris nil or of wrong type
destPtr := ctx.ValPtr.(*T)assumes the caller always initialisesValPtrwith*T. IfValPtrisnilor of another type the code panics, crashing the whole validation run. A defensive check prevents that:- destPtr := ctx.ValPtr.(*T) + destPtr, ok := ctx.ValPtr.(*T) + if !ok || destPtr == nil { + ctx.AddIssue(ctx.IssueFromUnknownError(fmt.Errorf("internal error: expected *%T in ctx.ValPtr, got %T", *new(T), ctx.ValPtr))) + return + }
94-104: 🛠️ Refactor suggestion
Unchecked type assertion from coercer can crash
coercerreturnsany; the.(T)assertion will panic if the coercer accidentally returns the wrong dynamic type.- *destPtr = v.(T) + val, ok := v.(T) + if !ok { + ctx.AddIssue(ctx.IssueFromCoerce(fmt.Errorf("coercer returned %T, expected %T", v, *new(T)))) + return + } + *destPtr = valAlternatively change
CoercerFunc’s signature to returnTdirectly to make the compiler enforce this constraint.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
preprocess.go (1)
16-19: Consider documenting pointer type constraints.The comment "out should never be a pointer type" needs clarification. This constraint isn't enforced in the code, and the comment could be more explicit about why pointer types should be avoided as outputs.
-// out should never be a pointer type +// Preprocess creates a schema that preprocesses input data before passing it to the underlying schema. +// The preprocessing function should return a non-pointer type T to avoid confusion with pointer handling in schema validation. +// If you need to return a pointer type, consider unwrapping it before returning.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (33)
boolean.go(2 hunks)boolean_test.go(1 hunks)boolean_validate_test.go(1 hunks)custom.go(4 hunks)custom_test.go(2 hunks)internals/DataProviders.go(2 hunks)internals/contexts.go(6 hunks)internals/utils.go(2 hunks)numbers.go(2 hunks)numbers_custom_test.go(2 hunks)numbers_int64_test.go(1 hunks)numbers_test.go(1 hunks)numbers_validate_test.go(1 hunks)pointers.go(3 hunks)preprocess.go(1 hunks)preprocess_test.go(1 hunks)preprocess_validate_test.go(1 hunks)slices.go(8 hunks)slices_test.go(0 hunks)slices_validate_test.go(1 hunks)string.go(4 hunks)string_custom_test.go(1 hunks)string_test.go(1 hunks)string_validate_test.go(1 hunks)struct.go(8 hunks)struct_helper_test.go(0 hunks)struct_helpers.go(0 hunks)struct_test.go(0 hunks)struct_validate_test.go(0 hunks)time.go(3 hunks)time_test.go(0 hunks)time_validate_test.go(0 hunks)zogSchema.go(5 hunks)
💤 Files with no reviewable changes (7)
- slices_test.go
- struct_helpers.go
- struct_test.go
- time_validate_test.go
- struct_validate_test.go
- struct_helper_test.go
- time_test.go
✅ Files skipped from review due to trivial changes (2)
- slices_validate_test.go
- boolean_test.go
🚧 Files skipped from review as they are similar to previous changes (22)
- boolean.go
- time.go
- string_test.go
- string_validate_test.go
- internals/contexts.go
- internals/utils.go
- internals/DataProviders.go
- numbers_test.go
- numbers.go
- string_custom_test.go
- numbers_int64_test.go
- custom_test.go
- custom.go
- numbers_custom_test.go
- string.go
- pointers.go
- numbers_validate_test.go
- struct.go
- slices.go
- preprocess_test.go
- preprocess_validate_test.go
- zogSchema.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
preprocess.go (8)
zogSchema.go (1)
ZogSchema(10-15)internals/contexts.go (3)
Ctx(8-42)SchemaCtx(132-142)NewExecCtx(44-49)internals/utils.go (1)
UnwrapPtr(24-33)zconst/consts.go (1)
ZogType(38-38)utilsOptions.go (1)
ExecOption(73-73)internals/Issues.go (1)
NewErrsList(128-132)conf/issueFormatConf.go (1)
IssueFormatter(63-63)internals/PathBuilder.go (1)
NewPathBuilder(19-23)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Cloudflare Pages
🔇 Additional comments (7)
boolean_validate_test.go (1)
203-205: Successfully replaced PreTransform with TestFuncThis change correctly implements the PR's goal of removing pre-transform functionality. The TestFunc now explicitly evaluates if the value is true, which maintains the same validation logic that was previously handled by a PreTransform function that likely errored on false values. The validation chain now applies TestFunc, then Catch, then Required, which preserves the test's intention.
preprocess.go (6)
1-9: Well-organized imports and package structure.The code structure and imports are clean and organized, with clear dependency relationships. Good use of imports aliasing to distinguish packages like
p "github.com/Oudwins/zog/internals".
11-14: Clean generic struct definition with clear purpose.The
PreprocessSchemastruct is well-designed with generics to handle conversion between input typeFand output typeT. The embedded schema allows for composable validation logic.
41-50: Good error handling for pointer compatibility.The switch statement correctly handles
*Tand**Tcases and panics for unsupported cases, which is appropriate given this is a developer-facing API where passing the wrong type is a programming error.
52-58: Effective delegation to underlying schema.The
getTypeandsetCoercermethods correctly delegate to the underlying schema, maintaining the composition pattern.
60-74: Well-implemented Parse method with proper resource management.The
Parsemethod correctly initializes necessary contexts and resources, properly handles cleanup withdefer, and applies user options effectively.
76-90: Validate method follows same pattern as Parse method.The
Validatemethod is consistent with theParsemethod in terms of resource management and execution flow, which is good for maintainability.
| func (s *PreprocessSchema[F, T]) validate(ctx *p.SchemaCtx) { | ||
| out, err := s.fn(ctx.ValPtr.(F), ctx) | ||
| if err != nil { | ||
| ctx.AddIssue(ctx.Issue().SetMessage(err.Error())) | ||
| return | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add type assertion check in validate method.
The unchecked type assertion at line 36 (ctx.ValPtr.(F)) could panic if the assertion fails. Add an explicit check to handle this case gracefully.
func (s *PreprocessSchema[F, T]) validate(ctx *p.SchemaCtx) {
- out, err := s.fn(ctx.ValPtr.(F), ctx)
+ v, ok := ctx.ValPtr.(F)
+ if !ok {
+ ctx.AddIssue(ctx.Issue().SetMessage(fmt.Sprintf("preprocess expected %T but got %T", *new(F), ctx.ValPtr)))
+ return
+ }
+ out, err := s.fn(v, ctx)
if err != nil {
ctx.AddIssue(ctx.Issue().SetMessage(err.Error()))
return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func (s *PreprocessSchema[F, T]) validate(ctx *p.SchemaCtx) { | |
| out, err := s.fn(ctx.ValPtr.(F), ctx) | |
| if err != nil { | |
| ctx.AddIssue(ctx.Issue().SetMessage(err.Error())) | |
| return | |
| } | |
| func (s *PreprocessSchema[F, T]) validate(ctx *p.SchemaCtx) { | |
| v, ok := ctx.ValPtr.(F) | |
| if !ok { | |
| ctx.AddIssue(ctx.Issue().SetMessage( | |
| fmt.Sprintf("preprocess expected %T but got %T", *new(F), ctx.ValPtr), | |
| )) | |
| return | |
| } | |
| out, err := s.fn(v, ctx) | |
| if err != nil { | |
| ctx.AddIssue(ctx.Issue().SetMessage(err.Error())) | |
| return | |
| } | |
| // …rest of method… | |
| } |
feat!: implemented preprocess and removed preTransforms
1 participant
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Chores