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.

Sign up for GitHub

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

Jump to bottom

openapi3: allow Extensions next to $ref in SchemaRef #901

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

openapi3: allow Extensions next to $ref in SchemaRef #901

wants to merge 2 commits into from
+1,258 −363

Conversation

paulmach
Copy link

@paulmach paulmach commented Jan 30, 2024 •
edited

See #900 for full details and use-case. If this approach is agreed upon, I can extend to all *Refs (e.g. CallbackRef, ExampleRef, ResponseRef, etc.)

Captures x-order into SchemaRef.Extensions

type: object
properties:
 id:
   $ref: "#/components/schemas/id"
   x-order: 1
 name:
   type: string

Only extensions (those starting with 'x-') are captured. This is because extra attributes are technically not allowed by the spec, but also to maintain the way JSONLookup works. For example:

schemas:
  id:
    type: integer
    x-order: 2
  thing:
    type: object
    properties:
     thing_id:
       $ref: "#/components/schemas/id"
       type: string
       x-order: 1
     name:
       type: string

Doesn't change: thingIDRef.JSONLookup("type") will continue to return "integer" as overriding the type next to the ref is not allowed and bad practice, in my opinion. There could be a use-case for stuff like nullable, but for now I would rather not change the behavior.

Does change: thingIDRef.JSONLookup("x-order"). Today, 2, as it goes back to the $ref definition. After this change it'll be 1 (value next to the $ref).

Does change: Validate() behavior. It's now okay, by default, to have extensions next to the $ref. To change this behavior, use the ProhibitExtensionsWithRef option.

@paulmach paulmach mentioned this pull request Jan 30, 2024
Open
@paulmach
Copy link
Author

Oh snap, looks like refs.go is auto generated. Please see if you like this change, then I make the change to refs.sh.

fenollp
fenollp reviewed Jan 31, 2024
View reviewed changes
openapi3/refs.go
@@ -559,6 +560,10 @@ func (x *ResponseRef) JSONLookup(token string) (interface{}, error) {
// SchemaRef represents either a Schema or a $ref to a Schema.
// When serializing and both fields are set, Ref is preferred over Value.
type SchemaRef struct {
// Extensions only captures fields starting with 'x-' as no other fields
// are allowed by the openapi spec.
Extensions map[string]interface{}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this should live here: this field already exists in the .Value. Or maybe it'd make more sense to move .Value.Extensions here, alongside .Ref .Value and .extra. This makes sense to me. You?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(this change alone would be its own PR)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did a quick test and it looks like ref.Value objects are reused for all the places they are referenced. Thus updating them will change them everywhere.

I personally think we should keep it as proposed here. This allows users to know where the extension came from. They can use schemaRef.JSONLookup("x-....") to do the most common thing, check ref, then fallback to Value.

fenollp
fenollp reviewed Jan 31, 2024
View reviewed changes
openapi3/refs_test.go Outdated Show resolved Hide resolved
fenollp
fenollp requested changes Jan 31, 2024
View reviewed changes
openapi3/refs_test.go Outdated Show resolved Hide resolved
@paulmach
Copy link
Author

paulmach commented Feb 1, 2024

I made the following changes:

  • moved tests in ref_test.go into refs_issue222_test.go and refs_issue247_test.go
  • added validation options AllowExtensionsWithRef (default) and ProhibitExtensionsWithRef
  • updated refs.sh to generate the code
  • add refs_test.sh to generate tests
  • reran docs.sh and added refs_test.sh as part of the github workflow.

I think the only "question" is if ref.Extensions should capture ^x- fields or all extra fields. As mentioned above, there could be some impact for "weird" schemas if we capture all fields. I propose to only capture ^x- fields for now as more backwards compatible. We can always change it based on feedback.

@fenollp
Copy link
Collaborator

fenollp commented Feb 1, 2024

This is great work thank you!

WRT non x-.. in Extensions you're right: let's just keep the x-.. keys in here for now anyway

@fenollp fenollp linked an issue Feb 2, 2024 that may be closed by this pull request
v3: allow extensions next to $ref in SchemaRef #900
Open
@paulmach paulmach requested a review from fenollp February 7, 2024 01:03
@paulmach
Copy link
Author

I rebased on the latest master. Hoping to get this into the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fenollp fenollp Awaiting requested review from fenollp

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

v3: allow extensions next to $ref in SchemaRef
2 participants
@paulmach @fenollp