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

Kiota should resolve polymorphic references (oneOf, allOf, anyOf) for request bodies when generating sliced OpenAPI for plugin #5164

Closed
Tracked by #5162
maisarissi opened this issue Aug 14, 2024 · 8 comments · Fixed by #5308
Assignees
Labels
Milestone

Comments

@maisarissi
Copy link
Contributor

maisarissi commented Aug 14, 2024

Today, Copilot plugins don't support polymorphic references (oneOf, allOf, anyOf) and circular references in OpenAPI descriptions.

For circular references, we shouldn't do anything and just throw an error as we can't know for sure how many degrees of separation should be considered, or how the user would like to resolve it, and giving control to the users to influence the behavior would be even more complex.
Note: Circular references moved to a separate issue, #5381

For polymorphic references in request bodies, rather than failing on plugin generation, Kiota should:

  • inline allOfs
  • pick the first entry for anyOfs and oneOfs

For allOfs, anyOfs and oneOfs in responses, we shouldn't do anything and just keep as it is. We shouldn't throw an errors as well.

@maisarissi maisarissi added the status:waiting-for-triage An issue that is yet to be reviewed or assigned label Aug 14, 2024
@github-project-automation github-project-automation bot moved this to Needs Triage 🔍 in Kiota Aug 14, 2024
@baywet
Copy link
Member

baywet commented Aug 14, 2024

This is really ambiguous. What do you expect to happen when a circular pattern is detected? drop the property causing it?
What about multiple degrees of separation? (group, members, user, memberof) how many degrees of separation should be considered here?
Should we give any control to the user to influence the behaviour here? (e.g. what if I really care about group members, but not user member of, but for some reason kiota is trimming properties the other way around from what I want?)

@baywet baywet added status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close and removed status:waiting-for-triage An issue that is yet to be reviewed or assigned labels Aug 14, 2024
@baywet baywet moved this from Needs Triage 🔍 to Waits for author 🔁 in Kiota Aug 14, 2024
@petrhollayms
Copy link
Contributor

@darrelmiller How do you see it for GA?

@petrhollayms petrhollayms added blocked This work can't be done until an external dependent work is done. and removed status:waiting-for-author-feedback Issue that we've responded but needs author feedback to close labels Aug 21, 2024
@petrhollayms petrhollayms moved this from Waits for author 🔁 to Proposed 💡 in Kiota Aug 21, 2024
@maisarissi maisarissi changed the title Use flattened schema for polymorphic references (oneOf, allOf, anyOf) and circular references when generating sliced OpenAPI for plugin Kiota should resolve polymorphic references (oneOf, allOf, anyOf) when generating sliced OpenAPI for plugin Aug 23, 2024
@maisarissi maisarissi changed the title Kiota should resolve polymorphic references (oneOf, allOf, anyOf) when generating sliced OpenAPI for plugin Kiota should resolve polymorphic references (oneOf, allOf, anyOf) for request bodies when generating sliced OpenAPI for plugin Aug 23, 2024
@maisarissi
Copy link
Contributor Author

I've updated the description based on internal discussion with @baywet and @darrelmiller
@petrhollayms we should be good to go.

@petrhollayms petrhollayms removed the blocked This work can't be done until an external dependent work is done. label Aug 27, 2024
@petrhollayms petrhollayms moved this from Proposed 💡 to Todo 📃 in Kiota Aug 27, 2024
@calebkiage
Copy link
Contributor

Question, shouldn't we fail for anyOf and oneOf? Otherwise, we'll get a bunch of bug reports because picking the first entry will change the meaning of the schema.

@petrhollayms
Copy link
Contributor

@maisarissi I am also asking myself this question on your initial description:

For allOfs, anyOfs and oneOfs in responses, we shouldn't do anything and just keep as it is. We shouldn't throw an errors as well.

Are we assuming there won't be multiple entries or why would the first item work otherwise?
@darrelmiller Any thoughts here?

@maisarissi
Copy link
Contributor Author

After some discussion with @baywet and @darrelmiller, we realized that yes, picking the first one is not the best solution, however, seems to be the least worst option.
Otherwise, the other option would validate whether the OpenAPI has oneOf/anyOf for the selected paths, and throw an error. This would eliminate several (if not almost all) uses cases for the users.

@calebkiage
Copy link
Contributor

I have another question about the expectation for allOf. In case I have a schema with allOf defined like:

schema:
  allOf:
  - type: string
  - type: number

should the slicing fail?

@baywet
Copy link
Member

baywet commented Sep 3, 2024

@calebkiage this specific example is impossible to resolve at runtime RFC
So yes, I believe that constraints which are impossible to resolve should result in a failure. Alternative being to select one of the entries, which could have undesired side effects.

@calebkiage calebkiage self-assigned this Sep 3, 2024
@calebkiage calebkiage moved this from Todo 📃 to In Progress 🚧 in Kiota Sep 3, 2024
@petrhollayms petrhollayms added this to the Kiota v1.18 milestone Sep 5, 2024
@baywet baywet removed this from the Kiota v1.18 milestone Sep 5, 2024
@baywet baywet added this to the Kiota v1.19 milestone Sep 5, 2024
@github-project-automation github-project-automation bot moved this from In Progress 🚧 to Done ✔️ in Kiota Sep 9, 2024
@baywet baywet modified the milestones: Kiota v1.18.1, Kiota v1.19 Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

4 participants