[AMORO-3256] Amoro Open Api with Swagger in Ams by mansonliwh · Pull Request #3296 · apache/amoro

mansonliwh · 2024-10-23T14:51:49Z

Why are the changes needed?

Brief change log

This PR introduces Swagger and OpenAPI Generator. To avoid intrusive development, this design does not use a large number of annotations on the controller. Instead, it predefines the openapi.yaml. The interface specifications defined in openapi.yaml are used to generate the Swagger page, API docs, and an SDK using the Apache HttpClient framework.

In the Maven build, you can use the generate-sdk profile to specify whether to generate the SDK.

How was this patch tested?

[ ☑️ ] Add screenshots for manual tests if appropriate

Swagger Page:

Auth Setting:

Generated API docs & SDK demo:

[ ☑️ ] Run test locally before making a pull request

czy006 · 2024-10-24T08:56:30Z

amoro-ams/pom.xml

            <scope>test</scope>
        </dependency>

+        <dependency>


we can use ${swagger.version} replace 2.2.10

Yes, we may define the versions in properties.

Thank you for your suggestion. I have removed that part of the dependency.

czy006 · 2024-10-24T08:57:29Z

amoro-ams/pom.xml

+
+            <plugin>
+                <groupId>org.openapitools</groupId>
+                <artifactId>openapi-generator-maven-plugin</artifactId>


the latest version is 7.9.0 why we don't use it?

the latest version is 7.9.0 why we don't use it?

Thank you for your suggestion. The latest version has dependency conflicts, and I believe the current version should be stable enough to meet the requirements.

codecov-commenter · 2024-11-04T02:31:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 21.60%. Comparing base (f13f3f3) to head (0c7516a).
Report is 16 commits behind head on master.

❗ There is a different number of reports uploaded between BASE (f13f3f3) and HEAD (0c7516a). Click for more details.

HEAD has 1 upload less than BASE

Flag BASE (f13f3f3) HEAD (0c7516a)

core 1 0

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #3296      +/-   ##
============================================
- Coverage     30.17%   21.60%   -8.57%     
+ Complexity     3840     2311    -1529     
============================================
  Files           580      426     -154     
  Lines         48020    39719    -8301     
  Branches       6207     5623     -584     
============================================
- Hits          14488     8582    -5906     
+ Misses        32542    30409    -2133     
+ Partials        990      728     -262

Flag	Coverage Δ
core	`?`
trino	`21.60% <ø> (?)`

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

zhoujinsong

@mansonliwh Thanks for the contribution! I left some comments.

zhoujinsong · 2024-11-04T03:18:32Z

amoro-ams/pom.xml

    <url>https://amoro.apache.org</url>

    <properties>
+        <skip-generate-sdk>false</skip-generate-sdk>


IMO, we should not generate the SDK by default as it is not required for everyone.
But the doc is necessary for the dashboard.

So we may need to separate the properties for SDK and DOC.

zhoujinsong · 2024-11-04T03:19:04Z

amoro-ams/pom.xml

            <scope>test</scope>
        </dependency>

+        <dependency>


Yes, we may define the versions in properties.

zhoujinsong · 2024-11-04T03:19:47Z

amoro-ams/pom.xml

+            <version>2.2.10</version>
+        </dependency>
+
+        <dependency>


Do we need the dependency by default?
Or should we separate them for the doc and the SDK?

Do we need the dependency by default? Or should we separate them for the doc and the SDK?

Thank you for your suggestion. I have already enabled the default 'skip' option while separated the 'doc' and the 'sdk'.

zhoujinsong · 2024-11-04T03:22:16Z

amoro-ams/pom.xml

+            <version>4.1.1</version>
+        </dependency>
+
+        <dependency>


We already had httpclient:5.2 and httpcore:5.2 dependencies in runtime scope, do we still need this?

huyuanfeng2018 · 2024-11-08T03:39:10Z

amoro-web/src/components/SwaggerUI.vue

+  mounted() {
+    SwaggerUI({
+      dom_id: '#swagger-ui',
+      url: '/swagger-docs', // 这里是你的 OpenAPI 规范文件的路径


Comments should be English.

amoro-web/src/router/index.ts

czy006

At present, we have no relevant descriptive documents for this section. Maybe we need to supplement them.

czy006

LGTM

zhoujinsong

@mansonliwh I left some comments, PTAL.

amoro-ams/pom.xml

zhoujinsong

LGTM.

Thanks for the contribution!

github-actions bot added module:ams-server Ams server module type:build module:ams-dashboard Ams dashboard module labels Oct 23, 2024

czy006 reviewed Oct 24, 2024

View reviewed changes

czy006 requested review from majin1102 and zhoujinsong October 28, 2024 07:02

zhoujinsong reviewed Nov 4, 2024

View reviewed changes

mansonliwh force-pushed the feature-swagger branch from 2e4fa1d to ae5b371 Compare November 8, 2024 02:07

github-actions bot added type:docs Improvements or additions to documentation module:common labels Nov 8, 2024

mansonliwh force-pushed the feature-swagger branch from ae5b371 to b169a1f Compare November 8, 2024 02:11

github-actions bot removed type:docs Improvements or additions to documentation module:common labels Nov 8, 2024

mansonliwh force-pushed the feature-swagger branch 2 times, most recently from ae5b21e to b136b28 Compare November 8, 2024 02:34

huyuanfeng2018 reviewed Nov 8, 2024

View reviewed changes

mansonliwh force-pushed the feature-swagger branch from b136b28 to 97f4ee5 Compare November 11, 2024 07:06

czy006 reviewed Nov 18, 2024

View reviewed changes

mansonliwh force-pushed the feature-swagger branch 2 times, most recently from cbd34de to 49a35cf Compare November 22, 2024 07:46

czy006 approved these changes Nov 24, 2024

View reviewed changes

zhoujinsong reviewed Nov 25, 2024

View reviewed changes

amoro-ams/pom.xml Outdated Show resolved Hide resolved

amoro-ams/pom.xml Outdated Show resolved Hide resolved

amoro-ams/pom.xml Outdated Show resolved Hide resolved

mansonliwh force-pushed the feature-swagger branch 2 times, most recently from fd62196 to 0c7516a Compare November 26, 2024 02:24

zhoujinsong reviewed Nov 26, 2024

View reviewed changes

amoro-ams/pom.xml Outdated Show resolved Hide resolved

amoro-ams/pom.xml Outdated Show resolved Hide resolved

[AMORO-3256] Amoro Open Api with Swagger in Ams

e972fc1

mansonliwh force-pushed the feature-swagger branch from f740008 to e972fc1 Compare November 26, 2024 06:08

zhoujinsong approved these changes Nov 26, 2024

View reviewed changes

czy006 merged commit 9e0f057 into apache:master Nov 26, 2024

klion26 mentioned this pull request Dec 16, 2024

Does Amoro provide Java API to external users #3357

Open

2 tasks

Conversation

mansonliwh commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are the changes needed?

Brief change log

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-commenter commented Nov 4, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

zhoujinsong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mansonliwh Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

czy006 left a comment

Choose a reason for hiding this comment

Uh oh!

czy006 left a comment

Choose a reason for hiding this comment

Uh oh!

zhoujinsong left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhoujinsong left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

mansonliwh commented Oct 23, 2024 •

edited

Loading

codecov-commenter commented Nov 4, 2024 •

edited

Loading

mansonliwh Nov 8, 2024 •

edited

Loading