'group-parameters: true' does not work

dolauli commented 1 year ago

Before filling a bug

[x] have you checked the faq for known issues.
[x] 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 commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

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

timotheeguerin commented 1 year ago

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:

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

however

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.
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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

Azure / autorest

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