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

Missing names sanitization cause conflicts #3400

Closed
andreaTP opened this issue Oct 2, 2023 · 5 comments · Fixed by #3479
Closed

Missing names sanitization cause conflicts #3400

andreaTP opened this issue Oct 2, 2023 · 5 comments · Fixed by #3479
Assignees
Labels
generator Issues or improvements relater to generation capabilities. type:bug A broken experience WIP
Milestone

Comments

@andreaTP
Copy link
Contributor

andreaTP commented Oct 2, 2023

Description

If the specification contains an endpoint called models ( most notably OpenAI ).

Apparently, there are issues with things getting not generated or overwritten ...

Repro

A minimal repro looks like:

openapi: 3.0.0
info:
  title: Test
  version: 1.0.0
  description: something
paths:
  /api/something/v1:
    post:
      requestBody:
        required: true
        content:
          application/json:
            schema:
              $ref: "#/components/schemas/Test1"
      responses:
        "200":
          description: "something"
          content:
            application/json: {}
  /models:
    post:
      requestBody:
        required: true
        content:
          application/json:
            schema:
              $ref: "#/components/schemas/Test2"
      responses:
        "200":
          description: "something"
          content:
            application/json: {}
components:
  schemas:
    Test1:
      description: this is a first test
      properties:
        test:
          description:
          type: string
      type: object
    Test2:
      description: this is a second test
      properties:
        test:
          description:
          type: integer
      type: object

Resolution

I'm not sure what would need to be escaped properly. Let me know if you need a further narrowing down of the issue.

@github-project-automation github-project-automation bot moved this to Todo in Kiota Oct 2, 2023
@baywet baywet added type:bug A broken experience generator Issues or improvements relater to generation capabilities. labels Oct 2, 2023
@baywet baywet added this to the Kiota v1.8 milestone Oct 2, 2023
@baywet
Copy link
Member

baywet commented Oct 2, 2023

🤦‍♂️ we hardcoded that namespace name and didn't make it reserved...
I'm guessing the best approach would probably be to project a "models" root path segment to "modelsRequests" or something like that in the builder (before any refiner). Thoughts?
Is this something you want to take on?

@andreaTP
Copy link
Contributor Author

andreaTP commented Oct 2, 2023

I'm guessing the best approach would probably be to project a "models" root path segment to "modelsRequests" or something like that in the builder (before any refiner). Thoughts?

I agree that this should be done before the refiner phase.
Not sure how this works now, but we can probably add models to the reserved words for all of the languages.
Additionally we can make the models namespace customizable by the user, wdyt?

Is this something you want to take on?

My backlog in this project is growing a little too fast compared to my availability those days 😅
I'll feel more comfortable if we keep it unassigned and I can pick it up, eventually, if no one else resolves it before me.

@baywet
Copy link
Member

baywet commented Oct 2, 2023

Not sure how this works now, but we can probably add models to the reserved words for all of the languages.

This would lead to the "hardcoded" models namespace to be considered conflicting and get renamed in the current design. This is why I was suggesting to do that earlier.

Additionally we can make the models namespace customizable by the user, wdyt?

I'm torn on this one. On one hand we want to keep as few parameters as possible (simplicity), on the other we allow configuration of the root namespace name and the client class. I think that even if we allowed configuration, we'd still need to handle potential collisions. So I'd consider configurability as an orthogonal aspect to this one.

My backlog in this project is growing a little too fast compared to my availability those days

I feel you... I'll try to have a look at it on the next "sprint", let's consider it assigned to me for now and ping me on this issue if you want to take it over before I get to it.

@andreaTP
Copy link
Contributor Author

andreaTP commented Oct 3, 2023

I think that even if we allowed configuration, we'd still need to handle potential collisions. So I'd consider configurability as an orthogonal aspect to this one.

Agreed.

I'll try to have a look at it on the next "sprint", let's consider it assigned to me for now and ping me on this issue if you want to take it over before I get to it.

Deal 🙂

@baywet
Copy link
Member

baywet commented Oct 12, 2023

Authored #3479 to address the problem

@github-project-automation github-project-automation bot moved this from In Progress to Done in Kiota Oct 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
generator Issues or improvements relater to generation capabilities. type:bug A broken experience WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants