feat(type): marshals and unmarshals multivalued type fields#176
feat(type): marshals and unmarshals multivalued type fields#176smoyer64 wants to merge 1 commit intoinvopop:mainfrom
Conversation
|
Looks like a solid improvement, thanks! Could you please merge in main or grant me modify rights? |
There was a problem hiding this comment.
Pull request overview
Adds first-class support for JSON Schema type fields that may be either a single string or an array of strings, aligning the library with the JSON Schema specification while attempting to preserve existing behavior.
Changes:
- Introduces a new
Typetype with custom JSON marshal/unmarshal supporting"type": "..."and"type": ["...", "..."]. - Updates schema reflection and tests to use
Typeconstants instead of raw string literals. - Adds fixture-backed tests covering multivalued
typemarshaling/unmarshaling.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
schema.go |
Changes Schema.Type to the new Type and implements multivalued JSON encoding/decoding. |
reflect.go |
Updates schema generation/tag handling to use Type constants and conversions. |
reflect_test.go |
Updates tests to use TypeString/TypeArray/etc. |
schema_test.go |
Adds new tests validating Type marshal/unmarshal behavior with fixtures. |
fixtures/multivalued_type.json |
Adds expected JSON output for a schema with a multivalued type. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| PropertyNames *Schema `json:"propertyNames,omitempty"` // section 10.3.2.4 | ||
| // RFC draft-bhutton-json-schema-validation-00, section 6 | ||
| Type string `json:"type,omitempty"` // section 6.1.1 | ||
| Type Type `json:"type,omitempty"` // section 6.1.1 |
There was a problem hiding this comment.
Changing Schema.Type from string to the new Type defined type is a compile-time breaking API change for callers that previously assigned a string variable (not a string literal) to Schema.Type. If the intent is to remain source-compatible in v1, consider an approach that keeps the public field as string (or adds a new field) and uses custom marshal/unmarshal to support both string and []string in JSON, or clearly document this as a breaking change requiring a major version bump.
| func (t *Type) IsMultivalued() bool { | ||
| return strings.Contains(string(*t), ",") | ||
| } | ||
|
|
||
| // MarshalJSON implements json.Marshaler. | ||
| func (t *Type) MarshalJSON() ([]byte, error) { | ||
| if t.IsMultivalued() { | ||
| return json.Marshal(strings.Split(string(*t), ",")) | ||
| } | ||
|
|
||
| return json.Marshal(string(*t)) |
There was a problem hiding this comment.
The Type methods use pointer receivers, which makes the API awkward to use: constants and temporary values (e.g. TypeArray.IsMultivalued() or NewMultivaluedType(...).IsMultivalued()) are not addressable, so these calls won’t compile. Consider switching IsMultivalued and MarshalJSON to value receivers (func (t Type) ...) while keeping UnmarshalJSON as a pointer receiver.
| func (t *Type) IsMultivalued() bool { | |
| return strings.Contains(string(*t), ",") | |
| } | |
| // MarshalJSON implements json.Marshaler. | |
| func (t *Type) MarshalJSON() ([]byte, error) { | |
| if t.IsMultivalued() { | |
| return json.Marshal(strings.Split(string(*t), ",")) | |
| } | |
| return json.Marshal(string(*t)) | |
| func (t Type) IsMultivalued() bool { | |
| return strings.Contains(string(t), ",") | |
| } | |
| // MarshalJSON implements json.Marshaler. | |
| func (t Type) MarshalJSON() ([]byte, error) { | |
| if t.IsMultivalued() { | |
| return json.Marshal(strings.Split(string(t), ",")) | |
| } | |
| return json.Marshal(string(t)) |
| // RFC draft-wright-json-schema-validation-00, section 5.26 | ||
| type Definitions map[string]*Schema | ||
|
|
||
| // Type represents the property types allowed by the JSONSchema specification,s |
There was a problem hiding this comment.
Typo in comment: "specification,s" looks like it should be "specification's" (or just "specification").
| // Type represents the property types allowed by the JSONSchema specification,s | |
| // Type represents the property types allowed by the JSONSchema specification's |
Resolves #151
This PR heeds the advice that a variant type field is problematic in a strongly typed language like Go. In addition, care was taken to be completely backwards compatible with existing applications using this library. This requires a small trade-off and custom marshaling and unmarshaling for the new
Typetype.Existing tests pass, and tests were added for the new
Typetype's functionality.