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

Escaped models are randomly failing at generation #2676

Closed
sebastienlevert opened this issue May 17, 2023 · 5 comments · Fixed by #2857
Closed

Escaped models are randomly failing at generation #2676

sebastienlevert opened this issue May 17, 2023 · 5 comments · Fixed by #2857
Assignees
Labels
type:bug A broken experience TypeScript Pull requests that update Javascript code WIP
Milestone

Comments

@sebastienlevert
Copy link
Contributor

No description provided.

@sebastienlevert sebastienlevert converted this from a draft issue May 17, 2023
@sebastienlevert sebastienlevert added this to the Kiota v1.3 milestone May 17, 2023
@sebastienlevert sebastienlevert added javascript Pull requests that update Javascript code and removed Needs: Triage 🔍 labels May 17, 2023
@sebastienlevert sebastienlevert added type:bug A broken experience TypeScript Pull requests that update Javascript code and removed type:bug A broken experience javascript Pull requests that update Javascript code labels May 17, 2023
@baywet
Copy link
Member

baywet commented Jun 7, 2023

@sebastienlevert we're going to start enforcing fines for issues like these.
What language? What description? Can you consistently reproduce it?...

@baywet baywet added type:bug A broken experience Needs: Author Feedback labels Jun 7, 2023
@baywet baywet modified the milestones: Kiota v1.3, Kiota v1.4 Jun 7, 2023
@sebastienlevert
Copy link
Contributor Author

We can reproduce in Typescript, pretty much on every generation with Graph.

@koros please share some background.

@baywet
Copy link
Member

baywet commented Jun 7, 2023

Is that issue for the smoke test in kiota typescript that has been falling for a while now?

@sebastienlevert
Copy link
Contributor Author

I don't know for the smoke test, but we can reproduce on every run of v1 for graph. Seems to be a race condition when going from classes to interfaces, where we generate some interfaces and some classes.

@baywet
Copy link
Member

baywet commented Jul 5, 2023

After a long investigation on the subject we have a race condition happening which either results:

  • model is an interface, with static factory, serialization and deserialization functions (expected)
  • model is a class, with static factory, serialization and deserialization functions (unexpected, it's kind of halfway through the transformation to an interface)

In both cases the code interface declaration writer is being called, in the former after, in the later before the code class declaration writer. We effectively have BOTH the class and the interface in the DOM as the TypeScript transformation from class to interface is not cleanly pulling the classes. Why is that more present on classes that have been escaped, I'm not sure yet.

This is due to multiple factors:

  • We build the Code Dom in parallels so the order is never guaranteed.
  • Multiple places in the code rename the interface by simply affecting the code interface name, but not always removing the code class.
  • When the rename is done, the element entry name in the DOM hasn't been updated.

This issue manifests itself much more often on larger descriptions.

Here is what I have started:

  • All rename calls to go through the dedicated code class method (so if we have an issue, it's at one place)
  • Call AddRange systematically during permutations (so conflicts are handled instead of failing silently)
  • Throwing when a remove is called but nothing was found (same reason)

Although the problem isn't solved yet, we're getting closer.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:bug A broken experience TypeScript Pull requests that update Javascript code WIP
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

3 participants