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

Items in Schema should be a single SchemaProxy instead of an array #28

Closed
TristanSpeakEasy opened this issue Dec 1, 2022 · 8 comments
Closed
Assignees

Comments

@TristanSpeakEasy
Copy link
Contributor

TristanSpeakEasy commented Dec 1, 2022

https://json-schema.org/understanding-json-schema/reference/array.html

Items can either be a single schema (for v2, v3.0 and v3.1) or a boolean (v3.1).

And along with PrefixItems there should be a Contains field as well which is a single schema along with minContains / maxContains fields.

@daveshanley
Copy link
Member

Yes, this is a mistake on my part. It was during some copy pasta work. Good call on the missing JSON Schema properties ( I expect there are a few others missing also).

I'll look into this as soon as possible.

@daveshanley daveshanley self-assigned this Dec 1, 2022
daveshanley added a commit that referenced this issue Dec 3, 2022
Added support for missing 3.1 schema properties, however it does not cover the `boolean` case
@daveshanley
Copy link
Member

daveshanley commented Dec 3, 2022

I've changed this in the high level model so Items is a single SchemaProxy and added contains and the min/max properties also. However when I was thinking about the or a boolean part, I hit a crossroad and i'd like some of your input.

In order to handle these dynamic values, in the low level model I used a SchemaDynamicValue that can hold two types (A or B)
https://github.com/pb33f/libopenapi/blob/main/datamodel/low/base/schema.go#L15

This API is awkward, and there is a reason I didn't bring it up high, but it allows multiple types to sit in a single value across versions. The question I have is:

Would this API be appropriate up high? because to use a single property, the model would need to use this dynamic type approach or use interfaces.

Another question: What kind of signature/interface/design would you like to see?

I have the rest of the code ready in a branch:
https://github.com/pb33f/libopenapi/blob/issue28-contains/datamodel/high/base/schema.go#L58

Let me know what you think.

@TristanSpeakEasy
Copy link
Contributor Author

So kin-openapi as an example adds an additional field to the structure when a field can be represented by multiple types, here is an example for AdditionalProperties https://github.com/getkin/kin-openapi/blob/master/openapi3/schema.go#L165 which could be one option but does mean the model doesn't match the spec exactly.

Or a container type that has a .Type() method which then allows you to case the .Value (any type) to the correct type to access the value?

@daveshanley
Copy link
Member

I'm open to suggestions on this one. Personally, it makes more sense to me to create a MultiType container (which is what the low-level does), but it might be too ugly for a high-level API.

Which of the two designs do you prefer? Another property or a multi-type container?

@danielgtaylor, what do you think?

@TristanSpeakEasy
Copy link
Contributor Author

I don't mind working with a container if it helps reduce some of the boiler plate around casting to types, maybe taking advantage of generics to return the correct type from a get method or similar

@danielgtaylor
Copy link
Contributor

There's already some high-level precedent for using separate fields, e.g. see the exclusive min/max:

	// In versions 2 and 3.0, this ExclusiveMaximum can only be a boolean.
	ExclusiveMaximumBool *bool

	// In version 3.1, ExclusiveMaximum is an integer.
	ExclusiveMaximum *int64

	// In versions 2 and 3.0, this ExclusiveMinimum can only be a boolean.
	ExclusiveMinimum *int64

	// In version 3.1, ExclusiveMinimum is an integer.
	ExclusiveMinimumBool *bool

The downside is it's harder to tell which one was set vs. a multi-type container like the low-level model provides where you can query for whether a or b is set. I don't have a strong preference on this one but we may want to think about consistency of the model, i.e. if Items gets a multi-type container then maybe the exclusive values should as well.

Another option is to have less of a one-to-one mapping in the high-level model via abstraction. items: false is semantically saying that no additional items are allowed when used with prefixItems so you could add a high-level additionalItems bool. Same with the exclusive min/max - rather than me managing all that mess you could just provide a convenience IsMinimumExclusive() bool or something along those lines. I guess it's a question of how much abstraction we can stomach knowing things may change again with Open API 3.2.

I know I'm an early adopter (you can see libopenapi in action right now in Restish 0.15.0) so don't mind if we decide it's best to break and I'll do some updating for the next release 👍 My test coverage of the Open API code is pretty good at this point.

daveshanley added a commit that referenced this issue Dec 8, 2022
Added support for missing 3.1 schema properties, however it does not cover the `boolean` case
daveshanley added a commit that referenced this issue Dec 8, 2022
Also adding in other properties to schema that are missing. Test coverage still needs improving and this is a breaking change to low and high models.
@daveshanley
Copy link
Member

Please check PR #40

daveshanley added a commit that referenced this issue Dec 9, 2022
Added support for missing 3.1 schema properties, however it does not cover the `boolean` case
daveshanley added a commit that referenced this issue Dec 9, 2022
Also adding in other properties to schema that are missing. Test coverage still needs improving and this is a breaking change to low and high models.
@daveshanley
Copy link
Member

This is resolved in v0.4.0 And the use of DynamicValue will allow high-level models to support multi-types moving foward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants