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

Add create_result and update_order #11

Merged
merged 1 commit into from
May 13, 2022
Merged

Conversation

diazruy
Copy link
Contributor

@diazruy diazruy commented May 6, 2022

  • Now using output directly as generated from OpenAPI generator, with no modifications. This nukes previous manual edits performed to various parts which were dangerous and easy to overlook (most of which were technically unnecessary and have been dealt with in different manners (for example, see this). No further manual modifications should be being made to the generated files to prevent this.
  • scripts/generate_clients.sh in connect has been replaced by individual rake tasks per language and the Ruby one has been made to consume a version number (e.g. rake connect_client:ruby[1.0.0]) that gets piped into the generated client, as well as copying the updated files to the sibling folder containing the gem, ready for publishing.
  • Now includes the models generated off of the Swagger schemas, given that the responses to client calls (which now have explicit schemas for better documentation) would otherwise be wrapped in InlineResponse200X classes which makes for a poor dev UX when using the client. Now the responses are more clearly PrimaryConnectClient::Orders, PrimaryConnectClient::OrderIds, etc.
  • Adds the new update_order and the previously undocumented create_result API endpoints

Copy link
Contributor

@DamonGroove DamonGroove left a comment

Choose a reason for hiding this comment

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

lgtm

@diazruy
Copy link
Contributor Author

diazruy commented May 6, 2022

@shawnd

Check if manual edits here need to be carried over: #11 (files)

So I followed the rabbit hole, and turns out that OpenAPI doesn't have a "good" way to handle redirect responses. The only suggestion I found was this one, which suggests indicating that the response has a Location header, which I did by adding

      response(302, 'found') do
        header 'Location', schema:  { type: :string }, description: 'The signed URL to download the report'

which correctly adds the documentation like this

image

IMPORTANT: rswag docs indicate that the header should be specified like

        header 'Location', type: :string, description: 'The signed URL to download the report'

(without the schema:), but this actually causes OpenAPI generator to choke on it given that the generated YML ends up like

      responses:
        '302':
          description: found
          headers:
            location:
              type: string
              description: The signed URL to download the report

instead of

      responses:
        '302':
          description: found
          headers:
            location:
-              schema:
-                type: string
+              type: string
              description: The signed URL to download the report

which is actually what the Swagger docs indicate it should be.

That all said, even with these changes, the generated get_lab_report method on the client still returns nil, which is what I think @samsohn28 was trying to get around. I think what needs to happen is that when we want access to the response headers, we need to call get_lab_report_with_http_info instead of get_lab_report (which uses get_lab_report_with_http_info internally and returns the data exclusively), which returns not just the data (which is useless in our case) but also the status_code, and (more importantly) the headers, which can then be used to follow the redirect.

Interestingly enough, looks like health is not even actually yet using the get_lab_report endpoint anyway.

Copy link
Collaborator

@samsohn28 samsohn28 left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for handling the manual edits I made!

@diazruy diazruy force-pushed the ruy__add_order_update branch 3 times, most recently from f889aae to db0c5de Compare May 12, 2022 00:03
- Added `create_result`
- Added `update_order`
- Included auto-generated models
@diazruy diazruy merged commit 63f2bad into master May 13, 2022
@diazruy diazruy deleted the ruy__add_order_update branch May 13, 2022 22: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.

4 participants