Optional schema numeric bound parsing - #59
Conversation
📝 WalkthroughWalkthroughAdds a shared ChangesNumeric bounds parsing
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/tests/schema_bounds_parsing_tests.zig (1)
94-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
.number_stringbranch ofoptionalFloatis untested.
std.json.Value.number_stringis only produced for integer-formatted numeric tokens that overflowi64(not for quoted JSON strings)."not-a-number"parses as.string, so this test actually exercisesoptionalFloat'selse => nullfallback, not the.number_string => parseFloat(...) catch nullpath added by this PR. Consider adding a test with an overflowing integer literal (e.g. an unquoted number wider thani64) to actually cover that branch.🧪 Suggested additional test
test "v3.0 schema parses overflowing integer multipleOf via number_string" { var gpa = test_utils.createTestAllocator(); const allocator = gpa.allocator(); const source = "{\"multipleOf\": 99999999999999999999999999999999}"; var parsed = try parseJsonValue(allocator, source); defer parsed.deinit(); var schema = try models.v3.Schema.parseFromJson(allocator, parsed.value); defer schema.deinit(allocator); try std.testing.expect(schema.multipleOf != null); }Please confirm this understanding of
std.json.Value's number classification for the Zig version this project targets.
JSON numbers encoded as floats may lose precision when being parsed into std.json.Value.float, which supports that.number_stringis reserved for the integer-overflow case rather than arbitrary strings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/schema_bounds_parsing_tests.zig` around lines 94 - 103, The current test in schema_bounds_parsing_tests.zig is using a quoted string, so it does not exercise the optionalFloat .number_string branch in models.v3.Schema.parseFromJson. Update or add a test that feeds an unquoted overflowing integer literal through parseJsonValue and verifies Schema.multipleOf is parsed from the .number_string path, while keeping the existing null-fallback coverage separate if needed.src/models/v3.0/schema.zig (1)
6-15: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
optionalFloathelper across v3.0 and v3.2.The identical helper is introduced verbatim in
src/models/v3.2/schema.zig(Lines 6-15). Consider extracting it into a shared module (e.g. a smalljson_helpers.zig) imported by both version-specific schema files to avoid future drift between the two copies.♻️ Suggested consolidation
// src/models/json_helpers.zig const json = `@import`("std").json; pub fn optionalFloat(value: ?json.Value) ?f64 { const val = value orelse return null; return switch (val) { .integer => |i| `@as`(f64, `@floatFromInt`(i)), .float => |f| f, .number_string => |s| `@import`("std").fmt.parseFloat(f64, s) catch null, else => null, }; }Then import it in both
src/models/v3.0/schema.zigandsrc/models/v3.2/schema.zig.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/models/v3.0/schema.zig` around lines 6 - 15, The optionalFloat helper is duplicated in both versioned schema files, so consolidate it into a shared utility module and import it from each schema. Create a shared helper (for example json_helpers.zig) containing optionalFloat, then update the v3.0 and v3.2 schema implementations to reference that shared function instead of keeping separate copies. Use the existing optionalFloat symbol as the extraction target so both schema files stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/models/v3.0/schema.zig`:
- Around line 6-15: The optionalFloat helper is duplicated in both versioned
schema files, so consolidate it into a shared utility module and import it from
each schema. Create a shared helper (for example json_helpers.zig) containing
optionalFloat, then update the v3.0 and v3.2 schema implementations to reference
that shared function instead of keeping separate copies. Use the existing
optionalFloat symbol as the extraction target so both schema files stay in sync.
In `@src/tests/schema_bounds_parsing_tests.zig`:
- Around line 94-103: The current test in schema_bounds_parsing_tests.zig is
using a quoted string, so it does not exercise the optionalFloat .number_string
branch in models.v3.Schema.parseFromJson. Update or add a test that feeds an
unquoted overflowing integer literal through parseJsonValue and verifies
Schema.multipleOf is parsed from the .number_string path, while keeping the
existing null-fallback coverage separate if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5c6300a-4a5d-4264-84df-195eb0050567
📒 Files selected for processing (4)
src/models/v3.0/schema.zigsrc/models/v3.2/schema.zigsrc/tests.zigsrc/tests/schema_bounds_parsing_tests.zig
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/tests/schema_bounds_parsing_tests.zig (2)
9-130: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffHeavy duplication between v3.0 and v3.2 test blocks.
The v3.0 (lines 9-130) and v3.2 (lines 132-253) test blocks are near-identical copies differing only by
models.v3.Schemavsmodels.v32.Schema, and the negative-bounds test is duplicated again at lines 255-266 and 334-345. A comptime-parametrized helper (looping over schema types) would cut this duplication significantly and reduce future maintenance drift between versions.Also applies to: 132-266, 334-345
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/schema_bounds_parsing_tests.zig` around lines 9 - 130, The schema bounds tests are duplicated across v3.0 and v3.2, with the same cases repeated for only the schema type changing. Refactor the repeated blocks in schema_bounds_parsing_tests.zig into a comptime-driven helper or shared test runner that accepts the schema type (for example, models.v3.Schema and models.v32.Schema) and reuses the same assertions. Also fold the duplicated negative-bounds cases into the shared helper so the v3 and v32 coverage stays aligned without maintaining parallel test copies.
268-333: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueTighten the
number_stringcases. These assertions only prove the bounds fields deserialize to a numeric value accepted byoptionalFloat; they don’t distinguish the overflow-specific path implied by the test names. Assert the parsed variant, or rename the tests if the intent is just to cover oversized numeric input.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/tests/schema_bounds_parsing_tests.zig` around lines 268 - 333, The schema bounds tests in the `schema_bounds_parsing_tests.zig` file are too weak because they only check that `multipleOf`, `maximum`, and `minimum` are non-null, which does not verify the overflow-specific `number_string` parsing path. Update the tests that call `models.v3.Schema.parseFromJson` and `models.v32.Schema.parseFromJson` so they assert the parsed value came through the `number_string` variant or equivalent overflow handling, not just any numeric value. If that level of detail is not intended, rename the affected test cases to match the actual behavior being covered.src/models/v3.0/schema.zig (1)
3-3: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGood fix for the multipleOf/maximum/minimum union-access panic; same risk remains for integer bound fields.
Using
json_helpers.optionalFloatcorrectly avoids crashing on.integervs.floattag mismatches (previouslyval.floatwould panic if the JSON value was actually an integer). However,maxLength,minLength,maxItems,minItems,maxProperties, andminProperties(Lines 191-192, 194-195, 197-198) still accessval.integerdirectly, which has the identical panic risk for malformed schema documents (e.g., a float or string value in place of an integer).src/models/v3.1/schema.zigalready guards this via a localoptionalIntegerhelper — consider extracting it intojson_helpers.zig(alongsideoptionalFloat) and reusing it here and in v3.2 for consistency and defensive parsing.Also applies to: 186-189
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/models/v3.0/schema.zig` at line 3, The integer-bound schema fields in the v3.0 parser still read union members directly and can panic on malformed JSON. Add and use a shared optionalInteger helper in json_helpers.zig, similar to optionalFloat, then update the schema parsing in schema.zig (the maxLength/minLength/maxItems/minItems/maxProperties/minProperties handling in the schema loader) to call that helper instead of accessing val.integer directly; also reuse the same helper in v3.2 for consistency.src/models/json_helpers.zig (1)
4-12: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSwallowed parse errors bypass the
catch |err|guideline.
std.fmt.parseFloat(...) catch nulldiscards the underlying error entirely instead of using thecatch |err|pattern with meaningful context. This matches the intended fallback behavior (invalid strings becomenull, as covered by the schema bounds tests), but per coding guidelines the error should still be captured/logged for diagnosability.As per coding guidelines,
**/*.zig: "Use Zig error sets andcatch |err|pattern for error handling with meaningful context messages".♻️ Proposed fix
- .number_string => |s| std.fmt.parseFloat(f64, s) catch null, + .number_string => |s| std.fmt.parseFloat(f64, s) catch |err| { + std.log.debug("optionalFloat: failed to parse number_string \"{s}\": {}", .{ s, err }); + return null; + },🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/models/json_helpers.zig` around lines 4 - 12, The optionalFloat helper in json_helpers.zig swallows parseFloat errors by using a bare catch null, which bypasses the required catch |err| pattern. Update optionalFloat to handle the .number_string branch with a named caught error, preserve the null fallback for invalid numbers, and add meaningful context using the err value so parse failures are diagnosable. Keep the behavior of returning null for bad input, but route the failure through std.fmt.parseFloat in a way that follows the project’s Zig error-handling guideline.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/models/json_helpers.zig`:
- Around line 4-12: The optionalFloat helper in json_helpers.zig swallows
parseFloat errors by using a bare catch null, which bypasses the required catch
|err| pattern. Update optionalFloat to handle the .number_string branch with a
named caught error, preserve the null fallback for invalid numbers, and add
meaningful context using the err value so parse failures are diagnosable. Keep
the behavior of returning null for bad input, but route the failure through
std.fmt.parseFloat in a way that follows the project’s Zig error-handling
guideline.
In `@src/models/v3.0/schema.zig`:
- Line 3: The integer-bound schema fields in the v3.0 parser still read union
members directly and can panic on malformed JSON. Add and use a shared
optionalInteger helper in json_helpers.zig, similar to optionalFloat, then
update the schema parsing in schema.zig (the
maxLength/minLength/maxItems/minItems/maxProperties/minProperties handling in
the schema loader) to call that helper instead of accessing val.integer
directly; also reuse the same helper in v3.2 for consistency.
In `@src/tests/schema_bounds_parsing_tests.zig`:
- Around line 9-130: The schema bounds tests are duplicated across v3.0 and
v3.2, with the same cases repeated for only the schema type changing. Refactor
the repeated blocks in schema_bounds_parsing_tests.zig into a comptime-driven
helper or shared test runner that accepts the schema type (for example,
models.v3.Schema and models.v32.Schema) and reuses the same assertions. Also
fold the duplicated negative-bounds cases into the shared helper so the v3 and
v32 coverage stays aligned without maintaining parallel test copies.
- Around line 268-333: The schema bounds tests in the
`schema_bounds_parsing_tests.zig` file are too weak because they only check that
`multipleOf`, `maximum`, and `minimum` are non-null, which does not verify the
overflow-specific `number_string` parsing path. Update the tests that call
`models.v3.Schema.parseFromJson` and `models.v32.Schema.parseFromJson` so they
assert the parsed value came through the `number_string` variant or equivalent
overflow handling, not just any numeric value. If that level of detail is not
intended, rename the affected test cases to match the actual behavior being
covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 949c2b51-247b-4df5-bcce-6a0fa20bdf4f
📒 Files selected for processing (5)
src/models/json_helpers.zigsrc/models/v3.0/schema.zigsrc/models/v3.1/schema.zigsrc/models/v3.2/schema.zigsrc/tests/schema_bounds_parsing_tests.zig
1 participant
Closes #52
Summary by CodeRabbit
Bug Fixes
multipleOf,maximum,minimum) to correctly accept integers, decimals, and numeric-string inputs.exclusiveMaximum/exclusiveMinimumboolean flags continue to be preserved.Tests