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

Patch OpenAPI spec instead of writing custom code #17

Closed
wants to merge 10 commits into from

Conversation

0xkubectl
Copy link
Contributor

@0xkubectl 0xkubectl commented Jul 23, 2024

In #12 custom code was introduced to fix vrchatapi/specification#241.
I have opened vrchatapi/specification#352, but we noticed that some of the generators do not support the oneOf. Its reasonable to assume that this is on hold until upstream generators of all supported languages can deal with oneOf.

This change generates code that is similar to what was added manually but does this via patching the spec instead. IMHO this a more maintainable way of achieving the same goal. Please give feedback.

EDIT: Based on these DC Messages: One, Two
I opted to use JSONPatch instead of a regular patch as it was mentioned they are quite hard to maintain.

@Rexios80
Copy link

The dart package used to do this, so I see no issue

@ariesclark
Copy link
Member

We could add this to the specification repository and, during bundling, create two specs. Generators that support oneOf can then use the feature-enabled spec.

@ariesclark
Copy link
Member

Something like:

/auth/user:
    get:
      summary: Login and/or Get Current User Info
      tags:
        - authentication
      x-codeSamples:
        $ref: "../codeSamples/authentication.yaml#/~1auth~1user/get"
      responses:
        '200':
          x-feature:
            key: oneOf
            schema: 
              oneOf:
                - $ref: ../responses/authentication/CurrentUserLoginResponse.yaml
                - $ref: '#/components/schemas/TwoFactorRequired'
            fallback:
              $ref: ../responses/authentication/CurrentUserLoginResponse.yaml
        '401':
          $ref: ../responses/MissingCredentialsError.yaml

then during bundling/building, we're turn this into two specification files.

Copy link
Collaborator

@C0D3-M4513R C0D3-M4513R left a comment

Choose a reason for hiding this comment

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

You include way to much changes here.
I split off the avoidBoxedModals and formatting into seperate pr's.

Making the .unwrap() into a .expect() and the example change together could be a seperate change.

examples/online.rs Outdated Show resolved Hide resolved
examples/online.rs Outdated Show resolved Hide resolved
@0xkubectl 0xkubectl force-pushed the kubectl/use-openapi-patch branch from 4ed0136 to ddc9fd2 Compare July 26, 2024 23:53
@0xkubectl
Copy link
Contributor Author

I think fixing this here is not a good solution at this point in time and is not implemented in a clean way. Closing for now.

@0xkubectl 0xkubectl closed this Sep 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Missing "requiresTwoFactorAuth" object
4 participants