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

'group-parameters: true' does not work #4688

Open
2 tasks done
dolauli opened this issue Dec 8, 2022 · 9 comments
Open
2 tasks done

'group-parameters: true' does not work #4688

dolauli opened this issue Dec 8, 2022 · 9 comments

Comments

@dolauli
Copy link
Contributor

dolauli commented Dec 8, 2022

Before filling a bug

  • have you checked the faq for known issues.
  • have you checked existing issues

I am using autorest/modelerfour@4.23.1, when I try to enable x-ms-parameter-grouping by setting group-parameters: true.
I will run into an exception as below (with swagger here).

C:\Users\xidi\.autorest\@autorest_modelerfour@4.23.1\node_modules\@autorest\modelerfour\dist\main.js - FAILURE  {} TypeError: this.codeModel.schemas.add is not a function
    at Grouper.processParameterGroup (C:\Users\xidi\.autorest\@autorest_modelerfour@4.23.1\node_modules\@autorest\src\grouper\grouper.ts:98:34) 
    at Grouper.process (C:\Users\xidi\.autorest\@autorest_modelerfour@4.23.1\node_modules\@autorest\src\grouper\grouper.ts:45:18)
    at processRequest (C:\Users\xidi\.autorest\@autorest_modelerfour@4.23.1\node_modules\@autorest\src\grouper\plugin-grouper.ts:22:27)
    at C:\Users\xidi\.autorest\@autorest_modelerfour@4.23.1\libs\extension-base\dist\autorest-extension.js:47:1
PLUGIN FAILURE: this.codeModel.schemas.add is not a function, TypeError: this.codeModel.schemas.add is not a function
    at Grouper.processParameterGroup (C:\Users\xidi\.autorest\@autorest_modelerfour@4.23.1\node_modules\@autorest\src\grouper\grouper.ts:98:34) 
    at Grouper.process (C:\Users\xidi\.autorest\@autorest_modelerfour@4.23.1\node_modules\@autorest\src\grouper\grouper.ts:45:18)
    at processRequest (C:\Users\xidi\.autorest\@autorest_modelerfour@4.23.1\node_modules\@autorest\src\grouper\plugin-grouper.ts:22:27)
    at C:\Users\xidi\.autorest\@autorest_modelerfour@4.23.1\libs\extension-base\dist\autorest-extension.js:47:1, {}
fatal   | TypeError: this.codeModel.schemas.add is not a function
fatal   | Process() cancelled due to exception : Plugin grouper reported failure. / Error: Plugin grouper reported failure.
    at C:\Users\xidi\.autorest\@autorestcore@3.9.3\node_modules\@autorest\core\dist\src_lib_autorest-corets.js:2817:19
    at ScheduleNode (C:\Users\xidi\.autorest\@autorestcore@3.9.3\node_modules\@autorest\core\dist\src_lib_autorest-corets.js:1351:29)
error   |   Error: Plugin grouper reported failure.
error   | Autorest completed with an error. If you think the error message is unclear, or is a bug, please declare an issues at https://github.com/Azure/autorest/issues with the error message you are seeing.
debug   | [6.82 s] Shutting Down.
debug   | [6.82 s] Exiting.
 xidi on  ~/acsharp/samples

I also have a try with a pretty simple swagger here. And it throws a different exception as below.

C:\Users\xidi\.autorest\@autorest_modelerfour@4.23.1\node_modules\@autorest\modelerfour\dist\main.js - FAILURE {} TypeError: request.updateSignatureParameters is not a function at Grouper.process (C:\Users\xidi\.autorest\@autorest_modelerfour@4.23.1\node_modules\@autorest\src\grouper\grouper.ts:46:21) at processRequest (C:\Users\xidi\.autorest\@autorest_modelerfour@4.23.1\node_modules\@autorest\src\grouper\plugin-grouper.ts:22:27) at C:\Users\xidi\.autorest\@autorest_modelerfour@4.23.1\libs\extension-base\dist\autorest-extension.js:47:1 PLUGIN FAILURE: request.updateSignatureParameters is not a function, TypeError: request.updateSignatureParameters is not a function at Grouper.process (C:\Users\xidi\.autorest\@autorest_modelerfour@4.23.1\node_modules\@autorest\src\grouper\grouper.ts:46:21) at processRequest (C:\Users\xidi\.autorest\@autorest_modelerfour@4.23.1\node_modules\@autorest\src\grouper\plugin-grouper.ts:22:27) at C:\Users\xidi\.autorest\@autorest_modelerfour@4.23.1\libs\extension-base\dist\autorest-extension.js:47:1, {} fatal | TypeError: request.updateSignatureParameters is not a function fatal | Process() cancelled due to exception : Plugin grouper reported failure. / Error: Plugin grouper reported failure. at C:\Users\xidi\.autorest\@autorestcore@3.9.3\node_modules\@autorest\core\dist\src_lib_autorest-corets.js:2817:19 at ScheduleNode (C:\Users\xidi\.autorest\@autorestcore@3.9.3\node_modules\@autorest\core\dist\src_lib_autorest-corets.js:1351:29) error | Error: Plugin grouper reported failure.

Please provide as much information as you can. This would be:
- OpenAPI files having the issues
- Autorest command used

Expected behavior
A clear and concise description of what you expected to happen.

Additional context
Add any other context about the problem here.

@timotheeguerin
Copy link
Member

timotheeguerin commented Dec 13, 2022

@dolauli I can't seem to reproduce this issue(With neither of those swagger) is there not another flag or config maybe that's also causing issue?

@dolauli
Copy link
Contributor Author

dolauli commented Dec 13, 2022

group-parameters: true

@timotheeguerin

Below is my configuration. And based on my test, it seems the issue is caused by the config emit-yaml-tags: false.

  emit-yaml-tags: false
  lenient-model-deduplication: true
  additional-checks: false
  always-create-content-type-parameter: false
  always-seal-x-ms-enums: true
  treat-type-object-as-anything: true
  group-parameters: true

@dolauli
Copy link
Contributor Author

dolauli commented Dec 15, 2022

@timotheeguerin
Could you reproduce the issue with emit-yaml-tags: false?

@timotheeguerin
Copy link
Member

timotheeguerin commented Dec 15, 2022

hhm yeah that reproduce now. Seems like the issue is that moderfour different plugins weren't designed with that in mind. The problem is without the tags the codemodel loose the type information and it can't be rebuilt as with classes which is what the plugins expect to be using.

There is 2 things we could do:

  1. make sure we don't go and serialize/deserialize in between plugins
  2. stop using those classes and work with basic js objects that are serializable

however

  1. Not sure if that's really feasible without doing a bad hack. As for each plugin the data needs to be serialized to be sent back over rpc to autorest core.
  2. is most likely not going to happen because it would be incompatible with tags serialization which other languages depend on.

Questions:

  • why do you want to have emit-yaml-tags set to false?
  • is this a new issue? was it working before?

@dolauli
Copy link
Contributor Author

dolauli commented Dec 19, 2022

Do your have a plan or ETA for fixing this issue?

Answer to your questions.

  • why do you want to have emit-yaml-tags set to false?
    Since in remodeler (v3), there are no yaml tags, when we migrate from v3 to modelerfour, we set it false.
  • is this a new issue? was it working before?
    I do not think it is a new issue. I find this issue recently when trying to support x-ms-parameter-group by setting 'group-parameters: true'.

@timotheeguerin
Copy link
Member

timotheeguerin commented Dec 19, 2022

By new issue I mean, has this ever worked? and from your answer I assumed it hasn't?

as said above this is tricky to fix. There might be a 3rd option which would be to ignore in each plugin that those are classes and treat it as object but I feel like that is a super big source of bugs where we'd be breaking languages expecting tags.

Can your yaml parser not ignore the tags?

@dolauli
Copy link
Contributor Author

dolauli commented Dec 21, 2022

By new issue I mean, has this ever worked? and from your answer I assumed it hasn't?

I just guess. Not sure whether it ever works before.

as said above this is tricky to fix. There might be a 3rd option which would be to ignore in each plugin that those are classes and treat it as object but I feel like that is a super big source of bugs where we'd be breaking languages expecting tags.

Instead of implementing emit-yaml-tags as a global setting, can you have it to be set on specific plugins(For my case, for powershell plugins, I can set it to false, for the other plugins, I can set it to true)? By doing that, It may reduce potential bugs as you mentioned about in 3rd option.

Can your yaml parser not ignore the tags?
Need further investigation, also as you mentioned for your 3rd option, it may be a source of bugs if I make this change.

@timotheeguerin
Copy link
Member

Instead of implementing emit-yaml-tags as a global setting, can you have it to be set on specific plugins(For my case, for powershell plugins, I can set it to false, for the other plugins, I can set it to true)? By doing that, It may reduce potential bugs as you mentioned about in 3rd option.

This doesn't work, the other generators can't run in parallel, the problem is if we change the m4 plugins to forget that we have those classes in case there is no tags then that means for some(including the grouper) that add new schema then the tag won't get added. So if you are actually expecting the tag to exists then it doesn't work anymore so now breaking every other generator. Can you check if your yaml parser cannot just ignore those tags. this is just extra metadata. If not the other alternative is to have a new plugin that parse it with tags and dump it without tags just before yours

@dolauli
Copy link
Contributor Author

dolauli commented Jan 3, 2023

Thanks @timotheeguerin. Now I am handling x-ms-parameter-grouping in my plugin by setting 'group-parameters: false.' It seems to work fine.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants