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

Support hierarchy subpath #2080

Merged
merged 44 commits into from
Nov 4, 2023
Merged

Conversation

qiaozha
Copy link
Member

@qiaozha qiaozha commented Oct 24, 2023

fixes #2035 #2066

This PR is for hierarchy client support, document some agreements we have:

  1. hierarchyClient will be enabled by default, unless it's specified as false.
  2. If hierarchyClient is true, we will generate hierarchy client in the classic layer. every namespace and interface will be considered as a layer.
  3. if hierarchyClient is true, generate hierarchy client subpath in the api layer. customer will no longer be able to access the deep layered apis from the top ./api subpath export.
  4. we will generate models with full namespace as prefix if we detect there's model name conflict or enableModelNamespace is true.
  5. we will generate hierarchy folder structure in the api layer, and generate classic folder to wrapper the classical client calls to the api layer. which has the same hierarchy folder structure as the api layer. which is also true if hierarchyClient is false and enableOperationGroup is true, we will just not export the api subpath in that case.
  6. we will generate all the operation options in the models/options.ts and if there're any name conflicts, we will add full namespace as prefix to avoid them. Though I tried to put them in the api hierarchy subpath to avoid resolve the conflicts with namespace prefix, but the api extractor doesn't look happy and we have to export all the operation options to the top level index.ts.
  7. If hierarchyClient is false, and enableOperationGroup is false, enableModelNamespace is also false, there should be no code change, which can be proved in those hierarchyClient false cases.

About testing,
I have enabled hierarchyClient true for three smoke test, hierarchy_generic, openai_generic and widget_dpg,
And disabled hierarchyClient for all the rest smoke tests and modular integration tests. The change for modularIntegration tests are code reorganize as point out in the above point 4. Their API doesn't change which can be proved from no integration tests change.

Copy link
Member Author

@qiaozha qiaozha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initial review for some implementation details to be confirmed.

Copy link
Member Author

@qiaozha qiaozha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Still missing the subpath export info in the package.json

* loadtesting-with-hierarchy-false

* do not return namespace hierarchies

* use interface name when enable operation group in rlc

* fix package.json
* loadtesting-with-hierarchy-false

* do not return namespace hierarchies

* use interface name when enable operation group in rlc

* fix package.json

* update generic generated files just import order chaneg
* disable-hierarchy-client-and-not-introduce-extra-changes

* fix model import issues

* reserve work

* reserve work

* fix hierarchy layer overload

* fix generation failure

* fall back to enableOperationGroup logic if hierarchyClient is false

* fix package.json and api layer export

* fix smoke-test
Copy link
Member Author

@qiaozha qiaozha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

left some reviews after disable hierarchy client in most of the other test cases.

Comment on lines +35 to +39
this.query = getQueryOperations(this._client);
this.property = getPropertyOperations(this._client);
this.header = getHeaderOperations(this._client);
this.requestBody = getRequestBodyOperations(this._client);
this.responseBody = getResponseBodyOperations(this._client);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of hierarchyClient is false, and operationGroup is true, we will just organize the code in the hierarchy way for better documentation purpose, and we will not export the api layer as subpath. @joheredi how do you think ? it should have no api interface change in both classic layer and api layer expect for the change of exporting the operation group operations interface which we have discussed earlier that have to be exported as of the api extractor and the change of operation operations change as I memtioned earlier.

@qiaozha qiaozha marked this pull request as ready for review November 2, 2023 10:27
qiaozha and others added 2 commits November 2, 2023 20:07
* hierarchy-namespace-in-client-tsp

* fix client structure with operations

* update smoke-test
@qiaozha qiaozha requested a review from joheredi November 2, 2023 13:03
@qiaozha qiaozha merged commit 7b641fe into Azure:main Nov 4, 2023
28 checks passed
@qiaozha qiaozha deleted the support-hierarchy-subpath branch November 4, 2023 01:13
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

Successfully merging this pull request may close these issues.

Ensure all operations (non-overloads) have unique names
2 participants