Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: implement interface for alternative JSONSchema() function #130

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

olistrik
Copy link

Hi there,

I have a particular use case of jsonschema where we have the following situation:

type Foo int // implements Stringer, JSONMashaller, TextMarshaller, etc

const (
  A Foo = iota
  B
  C
  // etc
)

type FooMap[T any] map[Foo]T

We have a Foo "enum" that has a limited set of known values, and implements various marshallers so that a string representation of the "enum" is used for all inbound and outbound serialization.

Unfortunately we've noticed that when serializing a map with a key of Foo jsonschema only considers the underlying integer value when reflecting the schema and produces the following schema:

{
  "type": "object",
  "patternProperties" : {
    "^[0-9]+$": ...
  }
}

Given that the JSON marshaller will (un)marshal map keys using the TextMarshaller interface, this is an issue for us. Perhaps their is also an argument that jsonschema should do the same, but my first approach to this was to try and provide an alternative schema using JSONSchema and JSONSchemaExtend. However, I have noticed that JSONSchema in particular is only usable for leaf nodes, you cannot return the result of a new jsonschema.Reflect as this may not have the same configuration as the parent and it will nest a new schema in the parent.

JSONSchemaExtend is possible to use for this, however it requires significant modification to the resulting schema in a way that I'm not personally comfortable with.

With this merge request I've added an alternative definition of the JSONSchema interface that provides a reflect callback which can be used to continue reflection into children.

The implementation of this for the foo map would be as follows:

func (af FooMap[T]) JSONSchema(reflect func(any) *jsonschema.Schema) *jsonschema.Schema {
	properties := jsonschema.NewProperties()

	var t T
	tSchema := reflect(&t)

	allFields := AllFields()
	for i := range allFields {
		properties.Set(allFields[i].String(), tSchema)
	}

	return &jsonschema.Schema{
		Type:       "object",
		Properties: properties,
	}
}

My reasoning about using the same function name JSONSchema over JSONSchemaIntercept or something similar, is because it keeps backwards compatibility while preventing implementing both interfaces on a single type, which would be a strange thing to do imo.

I haven't written tests for this yet, I wanted to hear your opinions on it first.

@samlown samlown added the needs tests Not enough tests for this to be accepted label Feb 19, 2024
@olistrik
Copy link
Author

@samlown I've added tests for this one too!

@8tomat8
Copy link

8tomat8 commented Apr 12, 2024

@olistrik love this change, switching to your fork for now :D

In my case it was super beneficial to have this method, to avoid cumbersome tags for the enum values. Now, I can, relatively easily, manage enum values in code, and those are reflected in the schema.

type ActionType string

const (
	ActionTypeA string = "A"
	ActionTypeB string = "B"
	ActionTypeC string = "C"
)

func (t ActionType) JSONSchema(reflect func(any) *jsonschema.Schema) *jsonschema.Schema {
	return &jsonschema.Schema{
		Type: "string",
		Enum: []any{
			ActionTypeA,
			ActionTypeB,
			ActionTypeC,
		},
	}
}

@olistrik
Copy link
Author

@8tomat8 glad you like it! Though unless you're using the reflect func(any) *jsonschema.Schema callback you probably don't need my fork. The standard JSONSchema() *jsonschema.Schema implementation should work fine for leaf nodes.

@olistrik
Copy link
Author

@8tomat8 oh also, if you do need the reflect callback, I'd recommend using the klippa-app fork rather than my own. I don't plan on maintaining my fork, but klippa will continue to pull upstream changes and maintain their fork if these changes don't get merged here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Not enough tests for this to be accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants