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

tsp-openapi3 - prevent collisions when same name is used in Open API 3 components categories. #4151

Closed
4 tasks done
chrisradek opened this issue Aug 12, 2024 · 1 comment · Fixed by #4216
Closed
4 tasks done
Assignees
Labels
bug Something isn't working openapi3:converter Issues for @typespec/openapi3 openapi to typespec converter triaged:core

Comments

@chrisradek
Copy link
Member

Describe the bug

Open API 3 allows defining schemas, parameters, and other shared entities in #/components. It's valid to have a schema named Foo, and then a parameter also named Foo that references the schema.

When such collisions occur, tsp-openapi3 behaves unexpectedly by trying to merge the parameter-associated TypeSpec model with whatever the schema-associated TypeSpec type is. This can cause very confusing output when the referenced parameter schema is not an object.

Reproduction

openapi: 3.0.0
info:
  title: Widget Service
  version: 0.0.0
tags: []
paths:
  /{id}:
    get:
      operationId: Widgets_read
      parameters:
        - $ref: "#/components/parameters/Foo"
      responses:
        "200":
          description: The request has succeeded.
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/Foo"
components:
  schemas:
    Foo:
      type: string
  parameters:
    Foo:
      name: foo
      in: query
      required: true
      schema:
        type: arrray
        item:
          - $ref: "#/components/schemas/Foo"

This generates the following Foo:

scalar Foo extends string;
@query foo: unknown;

Checklist

@chrisradek chrisradek added bug Something isn't working openapi3:converter Issues for @typespec/openapi3 openapi to typespec converter labels Aug 12, 2024
github-merge-queue bot pushed a commit that referenced this issue Aug 12, 2024
…peSpec data types (#4149)

Fixes #4080  and #4088

This also fixes a few bugs/rough edges around generating TypeSpec types
from OpenAPI3 component schemas.

Still 2 things to follow up with that I'll create issues for:
1. Support `allOf` with more than 1 member. Likely this will result in
spreading all the members, except extending 1 if there is 1 that has a
discriminator. (#4152)
2. Place all component schemas under a `Schemas` namespace (and do
something similar for `Parameters`) to prevent name collisions.
(#4151)

---------

Co-authored-by: Christopher Radek <Christopher.Radek@microsoft.com>
@timotheeguerin
Copy link
Member

timotheeguerin commented Aug 13, 2024

my vote here would be to get the Parameter suffix/namespace for parmaeters but keep schemas as they are

@markcowl markcowl added this to the [2024] September milestone Aug 19, 2024
@chrisradek chrisradek self-assigned this Aug 20, 2024
github-merge-queue bot pushed a commit that referenced this issue Aug 29, 2024
)

Fixes #4151 

This PR updates tsp-openapi3's model generation so that all top-level
parameters (`#/components/parameters`) are nested in a `Parameters`
block namespace.

Prior to this change, if top-level parameter had the same name as a
top-level schema, we would attempt to merge the two. This worked OK if
the schema was an object type, but led to broken results if the schema
was anything else.

Note:
In the linked issue, it was suggested that top-level schemas not be
scoped to their own namespace, so if a schema is referenced by a
parameter, it will now qualify it with the file-level namespace. This PR
introduces a `context` object that contains some state that can be
passed around. This is useful for keeping track of the file-level
namespace and using it when necessary, but the context will also be
useful in cases where we need to look at the definition of a referenced
schema from another schema.

---------

Co-authored-by: Christopher Radek <Christopher.Radek@microsoft.com>
weidongxu-microsoft pushed a commit to weidongxu-microsoft/typespec that referenced this issue Sep 3, 2024
…crosoft#4216)

Fixes microsoft#4151 

This PR updates tsp-openapi3's model generation so that all top-level
parameters (`#/components/parameters`) are nested in a `Parameters`
block namespace.

Prior to this change, if top-level parameter had the same name as a
top-level schema, we would attempt to merge the two. This worked OK if
the schema was an object type, but led to broken results if the schema
was anything else.

Note:
In the linked issue, it was suggested that top-level schemas not be
scoped to their own namespace, so if a schema is referenced by a
parameter, it will now qualify it with the file-level namespace. This PR
introduces a `context` object that contains some state that can be
passed around. This is useful for keeping track of the file-level
namespace and using it when necessary, but the context will also be
useful in cases where we need to look at the definition of a referenced
schema from another schema.

---------

Co-authored-by: Christopher Radek <Christopher.Radek@microsoft.com>
sarangan12 pushed a commit to sarangan12/typespec that referenced this issue Sep 16, 2024
…peSpec data types (microsoft#4149)

Fixes microsoft#4080  and microsoft#4088

This also fixes a few bugs/rough edges around generating TypeSpec types
from OpenAPI3 component schemas.

Still 2 things to follow up with that I'll create issues for:
1. Support `allOf` with more than 1 member. Likely this will result in
spreading all the members, except extending 1 if there is 1 that has a
discriminator. (microsoft#4152)
2. Place all component schemas under a `Schemas` namespace (and do
something similar for `Parameters`) to prevent name collisions.
(microsoft#4151)

---------

Co-authored-by: Christopher Radek <Christopher.Radek@microsoft.com>
sarangan12 pushed a commit to sarangan12/typespec that referenced this issue Sep 16, 2024
…crosoft#4216)

Fixes microsoft#4151 

This PR updates tsp-openapi3's model generation so that all top-level
parameters (`#/components/parameters`) are nested in a `Parameters`
block namespace.

Prior to this change, if top-level parameter had the same name as a
top-level schema, we would attempt to merge the two. This worked OK if
the schema was an object type, but led to broken results if the schema
was anything else.

Note:
In the linked issue, it was suggested that top-level schemas not be
scoped to their own namespace, so if a schema is referenced by a
parameter, it will now qualify it with the file-level namespace. This PR
introduces a `context` object that contains some state that can be
passed around. This is useful for keeping track of the file-level
namespace and using it when necessary, but the context will also be
useful in cases where we need to look at the definition of a referenced
schema from another schema.

---------

Co-authored-by: Christopher Radek <Christopher.Radek@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working openapi3:converter Issues for @typespec/openapi3 openapi to typespec converter triaged:core
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants