Fix double field bug

[jpalawaga]

This is a reformulation of #113. I've added additional code to support the suggestion from the other PR. I wanted to change the branch off of master in my own fork.

Test changes:

• I've changed the test TestReflector_Reflect_ptrDefault, which currently breaks on master when the second field is included (you do not get json schema, because specifying the default twice on a named type breaks the package).
• I've updated other tests that broke when removing siblings from $ref. Hopefully this is desirable!

jsonschema-go has a subtle bug: when resolving complex types, second or more redeclarations of the same type would cause some parsing to fall back to json parsing.

The first commit fixes an issue where the reflect did not have enough information to parse tags like default or example on second or more definitions. On later occurrences, it reused an existing $ref and lost that local type metadata, so parsing fell back to generic JSON decoding.

For string-like values, that caused errors such as all being rejected because valid JSON would need "all" (not all).

The second commit fixes the allOf issue that the PR review comment alludes to: instead of placing siblings to ref, we optionally wrap the reference in an allOf which we can then safely specify other properties for.

[llamapreview]

AI Code Review by LlamaPReview

🎯 TL;DR & Recommendation

Recommendation: Request Changes

This PR fixes a critical bug in JSON Schema generation but introduces breaking API changes and schema structure modifications that could impact downstream consumers.

🌟 Strengths

• Fixes a subtle bug where repeated type declarations caused JSON parsing errors.
• Improves compliance with JSON Schema draft-07 by correctly handling $ref siblings.

Priority	File	Category	Impact Summary (≤12 words)	Anchors
P1	reflect.go	Bug	API signature change could break external calls or cause missed updates.	method:checkInlineValue, path:context.go
P1	reflect.go	Architecture	allOf wrapper changes schema structure, affecting downstream consumers.	method:walkProperties, method:hasRefSiblings
P2	reflect.go	Maintainability	hasRefSiblings uses indirect JSON marshaling, which is fragile and inefficient.
P2	reflect.go	Architecture	checkInlineValue fetches definitions, creating hidden dependencies and complexity.	method:checkInlineValue, method:walkProperties
P2	reflect_test.go	Testing	Test updated to validate fix for double field bug, increasing confidence.

🔍 Notable Themes

• The PR emphasizes JSON Schema draft-07 compliance, but adds complexity in schema generation logic.
• Changes introduce coupling between reflection context and value checking functions.

📈 Risk Diagram

This diagram illustrates the risks associated with API changes and schema structure modifications in the JSON schema generation process.

sequenceDiagram
    participant WP as walkProperties
    participant CI as checkInlineValue
    participant HS as hasRefSiblings
    participant S as Schema

    WP->>CI: call checkInlineValue with rc
    note over CI: R1(P1): API signature change could break external calls
    WP->>HS: call hasRefSiblings
    HS-->>WP: return bool
    alt has siblings
        WP->>S: wrap ref in allOf
        note over S: R2(P1): Schema structure change affects downstream consumers
    end

Loading

---

💡 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.

[llamapreview]

P1 | Confidence: High

The signature of checkInlineValue changed to accept a ReflectContext parameter, requiring callers to pass it. This affects three call sites in walkProperties (for default and const) and reflectExample. The related_context shows the ReflectContext type definition in context.go and confirms it's used in walkProperties. This is a direct API change that could break any external code calling checkInlineValue directly (though it's likely internal). However, the primary risk is within the codebase: if any call site was missed, it would cause a compilation error. The PR shows the calls in walkProperties and reflectExample were updated, so the immediate build is preserved.

[llamapreview]

P1 | Confidence: High

This change introduces an allOf wrapper for schemas with a $ref and additional properties to comply with JSON Schema draft-07's $ref sibling semantics. It's a public API/schema structure change. The related_context shows the walkProperties method where this logic is applied. The impact is that any downstream consumers that parse the generated JSON Schema and assume $ref properties have direct siblings (like default or example at the same level) will see a different structure. The PR updates multiple tests to reflect this new output (e.g., "namedMapOmitempty" now uses "allOf":[{"$ref":"..."}]). This is a breaking change for schema consumers, but it's a correction toward spec compliance as intended.

[llamapreview]

P2 | Confidence: High

The test TestReflector_Reflect_ptrDefault was updated to include a second field (DiscoverMode2). This is a positive change that validates the fix for the "double field bug" described in the PR. The test now ensures that multiple fields of the same referenced type can each have their own default value correctly applied (via the new allOf wrapping). This increases confidence in the fix. However, the test's golden JSON output now shows both fields using allOf wrappers, which is the new expected behavior. The update is consistent with the architectural change.

[llamapreview]

P2 | Confidence: Medium

Speculative: The hasRefSiblings function uses JSON marshaling to determine if a schema has properties beyond Ref, Type, ReflectType, and Parent. This approach is indirect and could be fragile. For example, it depends on the Schema struct's JSON serialization behavior and might be affected by zero-value vs. omitted fields. It also incurs a performance overhead (serialization) for each referenced schema property. A more direct and efficient approach would be to check the relevant fields explicitly.

Code Suggestion:

func hasRefSiblings(schema *Schema) bool {
    if schema == nil || schema.Ref == nil {
        return false
    }
    // Check for the presence of any significant field other than the excluded ones.
    // This is a non-exhaustive example; the actual list should be derived from the Schema struct.
    return schema.Title != nil ||
        schema.Description != nil ||
        schema.Default != nil ||
        schema.Examples != nil ||
        schema.Const != nil ||
        schema.Enum != nil ||
        schema.Minimum != nil ||
        schema.Maximum != nil ||
        // ... check other fields that should not be siblings to $ref
        len(schema.AllOf) > 0 ||
        len(schema.AnyOf) > 0 ||
        len(schema.OneOf) > 0 ||
        schema.Not != nil ||
        schema.Items != nil ||
        schema.Properties != nil ||
        schema.AdditionalProperties != nil ||
        schema.PatternProperties != nil ||
        // ... etc.
}

[llamapreview]

P2 | Confidence: Medium

Speculative: The checkInlineValue function now fetches the referenced schema definition from the ReflectContext to read the correct type for parsing tag values. This creates a hidden dependency: the rc.getDefinition method must be available and must return a valid schema for the given reference path. If rc is nil or the definition isn't found, this could lead to a panic or incorrect behavior (falling back to the old bug). While the PR's context suggests this is set up correctly, this coupling increases the function's complexity and assumes the ReflectContext is fully populated at this point in the reflection process.