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

Redundant externalResource and endpoint #930

Closed
JBBianchi opened this issue Jul 19, 2024 · 10 comments
Closed

Redundant externalResource and endpoint #930

JBBianchi opened this issue Jul 19, 2024 · 10 comments
Assignees
Labels
area: spec Changes in the Specification change: breaking A breaking change that will impact in a major version change.
Milestone

Comments

@JBBianchi
Copy link
Member

JBBianchi commented Jul 19, 2024

What would you like to be added:
In the JSON Schema, externalResource and endpoint are very similar:

# from #/$defs
  externalResource:
    oneOf:
      - type: string
        format: uri
      - type: object
        properties:
          uri:
            type: string
            format: uri
            description: The endpoint's URI.
          authentication:
            description: The authentication policy to use.
            oneOf:
              - $ref: '#/$defs/authenticationPolicy'
              - type: string
          name:
            type: string
            description: The external resource's name, if any.
        required: [ uri ]

  endpoint:
    type: object
    properties:
      uri:
        type: string
        format: uri-template
        description: The endpoint's URI.
      authentication:
        description: The authentication policy to use.
        oneOf:
          - $ref: '#/$defs/authenticationPolicy'
          - type: string
    required: [ uri ]

And endpoint is only used once in CallHTTP as:

# from #/$defs/callTask/oneOf/2 (CallHTTP)/properties/with/properties
  endpoint:
    description: The HTTP endpoint to send the request to.
    oneOf:
      - $ref: '#/$defs/endpoint'
      - type: string
        format: uri-template

There is a clear overlap. The only difference is the name property in externalResource.

I think we could either keep only externalResource or refactor it to use endpoint:

  externalResource:
    oneOf:
      - type: string
        format: uri
      - type: object
        $ref: #/$defs/endpoint
        unevaluatedProperties: false
        properties:
          name:
            type: string
            description: The external resource's name, if any.

Why is this needed:
It keeps the JSON Schema better alligned with the DRY principle.

@cdavernas
Copy link
Member

cdavernas commented Jul 19, 2024

@JBBianchi I disagree here. Even though the difference is slight, it exists imho: while there is no reason to name an endpoint, there often is the need to name an external resource/file, which is the reason why I segregated both concepts.

I'm however obviously biased, so I'll let you guys decide what to do 😉

@JBBianchi
Copy link
Member Author

@JBBianchi I disagree here. Even though the difference is slight, it exists imho: while there is no reason to name an endpoint, there often is the need to name an external resource/file, which is the reason why I segregated both concepts.

I'm however obviously biased, so I'll let you guys decide what to do 😉

The strange thing is that the external resources is only named when it's an object. If it's defined as a string, it's unnamed. And even with the object, the name is still not required.

Anyhow, Isn't it still valid to reference the endpoint and extend its definition with the name property for an external resource, as the last sample in the issue suggests?

@cdavernas
Copy link
Member

The strange thing is that the external resources is only named when it's an object. If it's defined as a string, it's unnamed. And even with the object, the name is still not required.

Indeed, but I don't see why that's strange: you can name it, but only if you want/need to.

Anyhow, Isn't it still valid to reference the endpoint and extend its definition with the name property for an external resource, as the last sample in the issue suggests?

You are perfectly right: the external resource should inherit the endpoint's schema! Can you take care of the resulting PR?

@JBBianchi
Copy link
Member Author

Indeed, but I don't see why that's strange: you can name it, but only if you want/need to.

What I meant is if you want to name your resource, then it must be an endpoint and not a string, but I'm just splitting hairs here.

You are perfectly right: the external resource should inherit the endpoint's schema! Can you take care of the resulting PR?

Sure can do.

@cdavernas
Copy link
Member

What I meant is if you want to name your resource, then it must be an endpoint and not a string, but I'm just splitting hairs here.

Yes, of course! How else would you do it using a string?

@JBBianchi
Copy link
Member Author

Let me illustrate what I have in mind (endpoint unification & factoring out name).

The idea would be to refactor endpoint and externalResource such as:

unevaluatedProperties: false
$defs:
  authenticationPolicy:
    type: object
  endpoint:
    unevaluatedProperties: false
    oneOf:
      - type: string
        format: uri-template
        description: The endpoint's URI.
      - type: object
        properties:
          uri:
            type: string
            format: uri-template
            description: The endpoint's URI.
          authentication:
            description: The authentication policy to use.
            oneOf:
              - $ref: '#/$defs/authenticationPolicy'
              - type: string
        required:
          - uri
  externalResource:
    type: object
    unevaluatedProperties: false
    properties:
      name:
        type: string
      endpoint:
        $ref: '#/$defs/endpoint'
    required:
      - endpoint
type: object
properties:
  document:
    meta: See CallAsyncAPI with document
    $ref: '#/$defs/externalResource'
    description: The document that defines the AsyncAPI operation to call.
  endpoint:
    meta: See CallHTTP with endpoint
    $ref: '#/$defs/endpoint'
    description: The HTTP endpoint to send the request to.

Which validates:

document:
  name: my-resource
  endpoint: https//petstore
endpoint: https//petstore

or

document:
  endpoint:
    uri: https//petstore
    authentication: foo
endpoint:
  uri: https//petstore
  authentication:
    foo: bar

At the price of adding an extra level to externalResource

@cdavernas
Copy link
Member

Well, why not! Looks good to me!

@matthias-pichler-warrify @fjtirado @ricardozanini Any opinion of this?

@fjtirado
Copy link
Collaborator

@JBBianchi proposal looks good to me, good catch!

@matthias-pichler
Copy link
Collaborator

Looks good 👍

@ricardozanini ricardozanini added area: spec Changes in the Specification change: breaking A breaking change that will impact in a major version change. labels Aug 2, 2024
@cdavernas cdavernas added this to the v1.0.0-alpha3 milestone Aug 7, 2024
@cdavernas
Copy link
Member

Closed by #975

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: spec Changes in the Specification change: breaking A breaking change that will impact in a major version change.
Projects
Status: Done
Development

No branches or pull requests

5 participants