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

fix(codegen) OpenAPI now works with cargo-progenitor #146

Merged
merged 3 commits into from
Oct 22, 2024

Conversation

mkatychev
Copy link
Contributor

@mkatychev mkatychev commented Oct 21, 2024

Pull Request

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Description

This PR addresses issues in cargo-progenitor code generation
by adding missing types to certain OpenAPI objects during code generation with the codegen below:

RUST_LOG="debug" cargo run -- -i ./openapi.yaml -o bnaclient -n bnaclient -v 1.0.0 --interface builder

Checklist

  • [] I have updated the documentation accordingly
  • [] I have updated the Changelog (if applicable)

Fixes: : oxidecomputer/progenitor#877 (comment)

@pull-request-size pull-request-size bot added the size/S Small (10-29 lines of changes) label Oct 21, 2024
@mkatychev
Copy link
Contributor Author

I've run into a separate issue from oxidecomputer/progenitor#877
with type.len assertions ehre:
https://github.com/oxidecomputer/progenitor/blob/1a61bf1e663af551f3cfe89116f08bbd88d6b57b/progenitor-impl/src/method.rs#L1264

This is run into with things like getAnalysis when there is more than one good and bad request:

bna-api/openapi.yaml

Lines 86 to 101 in 438bca0

operationId: getAnalysis
summary: Get the summary of a specific analysis
description: Get the summary of a specific analysis .
tags:
- rating
parameters:
- $ref: "#/components/parameters/analysis_id"
responses:
200:
$ref: "#/components/responses/analysis"
400:
$ref: "#/components/responses/bad_request"
403:
$ref: "#/components/responses/forbidden"
404:
$ref: "#/components/responses/not_found"

@mkatychev mkatychev marked this pull request as ready for review October 21, 2024 23:09
@mkatychev mkatychev changed the title Open API generation fix fix(codegen) OpenAPI now works with cargo-progenitor Oct 21, 2024
@rgreinho
Copy link
Member

We are almost there!

With these changes, the openapi.yaml is not valid anymore though:

image

We're very close, I'm sure we can make it both valid and work with progenitor 🤞

@rgreinho
Copy link
Member

And yes! Just fixing these 3 violations makes the AOS file valid AND it still works with progenitor!

You can apply this patch to your branch and then we can merge 👍

From 57349001d51730e8a457054ccb12cf29915fd411 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?R=C3=A9my=20Greinhofer?= <[email protected]>
Date: Mon, 21 Oct 2024 20:53:01 -0500
Subject: [PATCH] Fix OAS violations

---
 openapi.yaml | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/openapi.yaml b/openapi.yaml
index 4b0db10..3c30406 100644
--- a/openapi.yaml
+++ b/openapi.yaml
@@ -1043,9 +1043,6 @@ components:
       description: "A collection of errors"
       items:
         $ref: "#/components/schemas/error"
-      responses:
-        400:
-          $ref: "#/components/responses/bad_request"
     features:
       type: object
       properties:
@@ -1457,7 +1454,6 @@ components:
       description: "An analysis"
       content:
         application/json:
-          type: array
           schema:
             $ref: "#/components/schemas/analysis"
     bad_request:
@@ -1546,9 +1542,6 @@ components:
         application/json:
           schema:
             $ref: "#/components/schemas/enqueue"
-      responses:
-        403:
-          $ref: "#/components/responses/forbidden"
     forbidden:
       description: >
         You weren't authorized to make your request; most likely this indicates an issue
-- 
2.47.0

Signed-off-by: Mikhail Katychev <[email protected]>
@mkatychev
Copy link
Contributor Author

Thanks for the patch!

@rgreinho
Copy link
Member

Thank you so much for fixing the generation! ❤️

I'll add the client as soon as we're done with the work on the API refactoring 💪

Copy link
Member

@rgreinho rgreinho left a comment

Choose a reason for hiding this comment

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

👍

@kodiakhq kodiakhq bot merged commit b3cde70 into PeopleForBikes:main Oct 22, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S Small (10-29 lines of changes)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants