fix: support $ref from endpoint response to components/responses BNCH-109662 #213

eli-bl · 2024-10-24T22:53:58Z

I've also opened a PR from the same branch in the upstream repo (here), but here I'm targeting it to our fork repo in against a new 2.x branch (which I created by pulling the latest release code on main from the upstream repo). The main branch in our fork repo is still the same as it was, that is, a heavily modified fork of a very old version of the upstream repo.

See also: https://benchling.atlassian.net/wiki/spaces/AE/pages/730268159/Fixes+necessary+to+use+openapi-python-client+for+v3+API

Why we need this fix: The V3 API spec makes heavy use of reusable response definitions like this, so that we don't have to spell out the same standard error responses for every endpoint.

This makes it so the following—

components:
  responses:
    GoodResponse:
      description: OK
      content:
        "application/json":
          schema:
            $ref: "#/components/schemas/MyObject"

paths:
  "/endpoint":
    get:
      responses:
        "200":
          $ref: "#/components/responses/GoodResponse"

—is exactly equivalent to:

paths:
  "/endpoint":
    get:
      responses:
        "200":
          description: OK
          content:
            "application/json":
              schema:
                $ref: "#/components/schemas/MyObject"

Previously, it would have failed to follow the response reference and behaved as if the response had no content.

This still requires the response to be fully defined inline in components/responses; it can't itself consist of just a $ref.

eli-bl · 2024-10-25T00:28:56Z

end_to_end_tests/baseline_openapi_3.0.json

@@ -991,6 +991,20 @@
        }
      }
    },


All of the files under end_to_end_tests can be ignored - they're just example data that's used by a test that runs the generator against a big spec file and checks for the expected output files.

damola-benchling · 2024-10-28T16:19:56Z

openapi_python_client/parser/responses.py

+        if not ref_path.startswith("/components/responses/"):
+            return ParseError(data=data, detail=f"$ref to {data.ref} not allowed in responses"), schemas
+        resp_data = responses.get(ref_path.split("/")[-1], None)
+        if not resp_data:
+            return ParseError(data=data, detail=f"Could not find reference: {data.ref}"), schemas
+        if not isinstance(resp_data, oai.Response):
+            return ParseError(data=data, detail="Top-level $ref inside components/responses is not supported"), schemas


instead of individual return statements like these,... what if we used a error_detail: str variable and just have one return statement at the end of this function that has one return statement for ParseError -- this may or may not look very pretty but at least it would avoid disabling PLR0911 rule

I honestly find it easier to follow the logic when there's explicit short-circuiting like this. Maybe it's because I did a lot of Go and that's the preferred pattern in Go. But if I see a bunch of this kind of thing—

if bad_condition_1: error_detail = message1 elif bad_condition_2: error_detail = message2 else: error_detail = None if error_detail: return ParseError(data=data, detail=error_detail)

—it bothers me a little because now the code paths for the error conditions are not really independent of each other; if for instance I messed up and put "if" instead of "elif" for the second one, now the bad_condition_1 test would be effectively a no-op. It also means that I have to be constructing the same kind of error, ParseError, each time (and the same way each time: data=data), which happens to be true in this case but isn't logically inevitable; if I had to make a change where now one of those cases raises a different error, I'd have to refactor the whole thing.

PLR0911 is only a helpful rule when it's helpful, and this codebase disables it in quite a few parsing/validation functions like this.

PLR0911 is only a helpful rule when it's helpful, and
agree

this codebase disables it in quite a few parsing/validation functions like this

okay this sounds good to me

damola-benchling · 2024-10-28T16:24:01Z

openapi_python_client/parser/responses.py

+            return ref_path, schemas
+        if not ref_path.startswith("/components/responses/"):
+            return ParseError(data=data, detail=f"$ref to {data.ref} not allowed in responses"), schemas
+        resp_data = responses.get(ref_path.split("/")[-1], None)


I'm a little surprised that getting reference data is not a little more standardized or at least with a helper.... is doing a split and -1 the general pattern in the rest of the repo?

There's no use case in the rest of the repo for getting just the type name from a reference.

Note that this isn't really "getting reference data". The helper is just converting the string "#/components/schemas/MyTypeName" to the string "MyTypeName".

Whoops, I was wrong, there are a couple other usages like this. So I've added a helper.

damola-benchling · 2024-10-28T16:26:43Z

openapi_python_client/parser/responses.py

    *,
    status_code: HTTPStatus,
    data: Union[oai.Response, oai.Reference],
    schemas: Schemas,
+    responses: Dict[str, Union[oai.Response, oai.Reference]],


this is a breaking change to this function's signature....What's the general methodology to validate that this is safe (possibly other places that might call this function?)
I think we should have good confidence from passing tests but just calling it out in case there's anything additional we need to consider

It's validated by the existing unit tests for the repo, which include 1. a bunch of tests of building an Endpoint (which is literally the only code path response_from_data is called from), and 2. a full end-to-end test of running the generator on an entire API spec that includes endpoints with responses.

building an Endpoint (which is literally the only code path response_from_data is called from

oh okayy cool... this is what I wanted to confirm!

damola-benchling · 2024-10-28T19:49:51Z

openapi_python_client/parser/properties/schemas.py

+def get_reference_simple_name(ref_path: str) -> str:
+    """
+    Takes a path like `/components/schemas/NameOfThing` and returns a string like `NameOfThing`.
+    """
+    return ref_path.split("/", 3)[-1]
+
+


fix: support $ref from endpoint response to components/responses

41e954d

eli-bl commented Oct 25, 2024

View reviewed changes

eli-bl requested a review from damola-benchling October 25, 2024 00:31

eli-bl added 2 commits October 24, 2024 17:36

lint

fb0182e

lint

fe6b7dd

eli-bl changed the title ~~fix: support $ref from endpoint response to components/responses~~ fix: support $ref from endpoint response to components/responses BNCH-109662 Oct 25, 2024

eli-bl added 3 commits October 25, 2024 10:09

test coverage

8b0aca1

test coverage

2edc6fe

fix test to test the right thing

9df3bb4

damola-benchling reviewed Oct 28, 2024

View reviewed changes

add a helper

78bd860

eli-bl requested a review from damola-benchling October 28, 2024 19:25

damola-benchling reviewed Oct 28, 2024

View reviewed changes

damola-benchling approved these changes Oct 28, 2024

View reviewed changes

eli-bl merged commit 23b6caf into 2.x Oct 28, 2024

eli-bl deleted the responses-by-ref branch October 29, 2024 22:21

eli-bl restored the responses-by-ref branch October 29, 2024 22:21

eli-bl deleted the responses-by-ref branch November 20, 2024 23:29

eli-bl restored the responses-by-ref branch November 20, 2024 23:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: support $ref from endpoint response to components/responses BNCH-109662 #213

fix: support $ref from endpoint response to components/responses BNCH-109662 #213

eli-bl commented Oct 24, 2024 •

edited

Loading

eli-bl Oct 25, 2024

damola-benchling Oct 28, 2024

eli-bl Oct 28, 2024

damola-benchling Oct 28, 2024

damola-benchling Oct 28, 2024

eli-bl Oct 28, 2024

eli-bl Oct 28, 2024

eli-bl Oct 28, 2024

damola-benchling Oct 28, 2024

eli-bl Oct 28, 2024 •

edited

Loading

damola-benchling Oct 28, 2024

damola-benchling Oct 28, 2024

@@ @@ -991,6 +991,20 @@ @@
                       }
                     }
                   },

fix: support $ref from endpoint response to components/responses BNCH-109662 #213

fix: support $ref from endpoint response to components/responses BNCH-109662 #213

Conversation

eli-bl commented Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eli-bl Oct 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eli-bl commented Oct 24, 2024 •

edited

Loading

eli-bl Oct 28, 2024 •

edited

Loading