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

[Regression] Type collisions for types referenced from OData actions #1198

Closed
andrueastman opened this issue Feb 14, 2022 · 5 comments · Fixed by #1215
Closed

[Regression] Type collisions for types referenced from OData actions #1198

andrueastman opened this issue Feb 14, 2022 · 5 comments · Fixed by #1215
Assignees
Labels
generator Issues or improvements relater to generation capabilities. type:bug A broken experience

Comments

@andrueastman
Copy link
Member

#1100 involved the refactoring of this function

This seems to have the side effect that the class name is no longer inferred from the URL but is now derived from the schema. This causes some type collisions (e.g in delta functions) as the type name now changes from Delta to the actual type name (e.g Messages,MailFolder) to cause ambiguity in request builders to cause compilation failures.

This is replicated when using the full open Api description.

Original
image

After
image

We can enforce lookup for types in all namespaces before calling this function. We might need to move some files to the default models folder.

@baywet baywet added type:bug A broken experience generator Issues or improvements relater to generation capabilities. labels Feb 14, 2022
@baywet baywet added this to the TypeWriter Replacement milestone Feb 14, 2022
@baywet
Copy link
Member

baywet commented Feb 14, 2022

Thanks for reporting this @andrueastman !
I noticed that while working on #1174 but I wasn't sure whether the behaviour was caused by previous changes or current ones.
The reason why I added the schema in the evaluation was to get the proper name for the error classes that was sometimes missing.
If you don't mind having a look at it, it'd be greatly appreciated!

@baywet
Copy link
Member

baywet commented Feb 14, 2022

Note: while working on that, you should also test with Mail.yml from this PR to make sure the error classes still get the right name.

@andrueastman
Copy link
Member Author

No worries @baywet.

At the moment I can work around this by passing checkInAllNamespaces: true to the function here but that has the side effect of have crossing references across other namespaces.

@baywet
Copy link
Member

baywet commented Feb 14, 2022

yep, this should stay as it is, otherwise we'll start having race conditions that'll reference the wrong types in different namespace depending on the order of execution.
I think the right way to address this issue would be to pass the schema ONLY when we need it to get a more precise class name, or something along those lines.

@andrueastman
Copy link
Member Author

I think the right way to address this issue would be to pass the schema ONLY when we need it to get a more precise class name, or something along those lines.

I agree. Will action and resolve in a PR soon.

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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants