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

chore: update shared schema spec for consistency #204

Merged
merged 1 commit into from
Oct 21, 2022

Conversation

mkurapov
Copy link
Contributor

@mkurapov mkurapov commented Oct 21, 2022

There is an open Rafiki PR that generates Typescript types from the Open API spec using the openapi-typescript package.
Whenever type generation script runs, it generates a file with types as such:

export interface paths {
   ...
}

export interface components {
   schemas: {
      ...
   }
   responses: {
      ...
   }
   parameters: {
      ...
   }
}

export interface operations {
   ...
}

export interface external {
   ...
}

The external interface is the one that holds references to the types for when an external $ref is used in a spec (e.g. $ref: ../shared/schemas.yaml#/amount.

However, after the spec split, the type generation for the resource server failed to populate this external interface, since shared/schemas.yaml was declaring components directly at the root like:

assetCode:
  title: Asset code
  type: string
  description: The assetCode is a code that indicates the underlying asset. This SHOULD be an ISO4217 currency code.

The generator can only populate the external/shared schema interface if using the components -> schemas declaration, meaning the shared schema items must be declared like:

components:
  schema:
    ...
    assetCode:
      title: Asset code
      type: string
      description: The assetCode is a code that indicates the underlying asset. This SHOULD be an ISO4217 currency code.

This PR nests the fields in openapi/shared/schemas.yaml to be under components -> schemas in order to allow type generation to work. If this is too much of a change to support this tool, I might be able to figure out a different (possibly convoluted) solution, but I thought this standardization wouldn't cause issues nor be too much of a stretch.

Comment on lines +2 to +11
info:
title: Open Payments - Shared schemas
version: '1.0'
license:
name: Apache 2.0
identifier: Apache-2.0
summary: Open Payments - Shared schemas
description: 'Shared schemas used across Open Payments APIs'
contact:
email: tech@interledger.org
Copy link
Contributor Author

Choose a reason for hiding this comment

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

not necessary perse, but just additional information

Comment on lines +12 to +13
components:
schemas:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no changes other than this nesting

@mkurapov mkurapov marked this pull request as ready for review October 21, 2022 14:40
Copy link
Contributor

@wilsonianb wilsonianb left a comment

Choose a reason for hiding this comment

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

related issue:

We can consider reverting this if/when that's resolved.

@mkurapov
Copy link
Contributor Author

@wilsonianb yes, that's the one I was looking at, the final response in that issue was a bit different than the earlier one in that thread that was relevant to this change. I hope if there is a change to the library both of those fixes will be addressed.

@mkurapov mkurapov merged commit 7bb2e6a into main Oct 21, 2022
@mkurapov mkurapov deleted the 669/mk/update-shared-schema branch October 21, 2022 18:02
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

Successfully merging this pull request may close these issues.

2 participants