[fix] [doc] fix multiple apis in the automatically generated documentation use the same anchor point #19193

poorbarcode · 2023-01-11T17:21:06Z

Motivation

Now that site admin-rest-api is automatically generated through swagger, each API in the site has its own link that we can walk through, the generated rule of API-link is https://pulsar.apache.org/admin-rest-api/#operation/{operationId}, we call the suffix #operation/{operationId} of link anchor.

Since the {operationId}'s default value is the method name of the endpoint class, and there has the same method name in the multi endpoint-classes, such as Namespaces.setDispatchRate and PersistentTopics.setDispatchRate, so these both APIs will generate the same link: https://pulsar.apache.org/admin-rest-api/#operation/setDispatchRate.

Modifications

In the update to version 3.1.8 of swagger-maven-plugin, there are another ways to set operationId: The Operation Id Format. see https://github.com/kongchen/swagger-maven-plugin/blob/e4cf3af6ed4a4987b2269a05b7037549bc4a5511/src/main/java/com/github/kongchen/swagger/docgen/reader/AbstractReader.java#L535-L540. So we can generate operationId using the specified rule {className}_{methodName}.

Does this pull request potentially affect one of the following parts:

If the box was checked, please highlight the changes

Documentation

doc
doc-required
doc-not-needed
doc-complete

Matching PR in forked repository

PR in forked repository:

[ci] [fix] [doc] fix multiple apis in the automatically generated document poorbarcode/pulsar#51

dave2wave

These look like actual API changes. Please explain.

poorbarcode · 2023-01-11T18:03:57Z

Hi @dave2wave

These look like actual API changes. Please explain.

Already changed the plain to only modify Swagger annotations. please take a look again

mattisonchao · 2023-01-12T07:02:44Z

@tisonkun What do you think about this PR? I am unfamiliar with swagger, but I know you have recently worked on this part. please help to review it, thanks!

tisonkun · 2023-01-12T07:13:07Z

I agree that it's not a breaking change for end-users but only affects the doc experience.

Some related works previous:

And I agree that a nickname alias can be a better solution. I can approve this move but I appreciate seeing the discussion on the mailing list continue for a while (days or after the lunar new year holiday).

cc @tuteng @shoothzj

shoothzj · 2023-01-12T10:01:28Z

Now we aggregate rest api into a single file. I think it might be better to split them. This is also good for us to generate other language admin clients. I am trying to work on it.

github-actions · 2023-02-12T02:02:43Z

The pr had no activity for 30 days, mark with Stale label.

poorbarcode · 2023-03-08T08:02:06Z

@tisonkun @tuteng @heesung-sn

Please take a look, thanks

poorbarcode · 2023-03-08T08:33:04Z

/pulsarbot rerun-failure-checks

poorbarcode · 2023-03-08T10:34:30Z

/pulsarbot rerun-failure-checks

poorbarcode · 2023-03-08T13:20:51Z

/pulsarbot rerun-failure-checks

codecov-commenter · 2023-03-08T13:59:22Z

Codecov Report

Merging #19193 (e414067) into master (af1360f) will increase coverage by 35.91%.
The diff coverage is n/a.

@@              Coverage Diff              @@
##             master   #19193       +/-   ##
=============================================
+ Coverage     26.40%   62.31%   +35.91%     
+ Complexity     6427     3503     -2924     
=============================================
  Files          1597     1848      +251     
  Lines        123682   135866    +12184     
  Branches      13511    14952     +1441     
=============================================
+ Hits          32658    84665    +52007     
+ Misses        86336    43466    -42870     
- Partials       4688     7735     +3047

Flag	Coverage Δ
inttests	`24.53% <ø> (-0.05%)`	⬇️
systests	`25.28% <ø> (?)`
unittests	`59.54% <ø> (+42.14%)`	⬆️

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

Impacted Files	Coverage Δ
...in/java/org/apache/pulsar/common/api/AuthData.java	`71.42% <0.00%> (ø)`
.../apache/pulsar/broker/namespace/LookupOptions.java	`87.50% <0.00%> (ø)`
.../apache/pulsar/common/naming/SystemTopicNames.java	`55.55% <0.00%> (ø)`
...apache/pulsar/common/util/SafeCollectionUtils.java	`0.00% <0.00%> (ø)`
...pache/pulsar/common/configuration/BindAddress.java	`22.22% <0.00%> (ø)`
...r/client/admin/internal/data/AuthPoliciesImpl.java	`65.21% <0.00%> (ø)`
...sar/common/policies/data/impl/BundlesDataImpl.java	`92.30% <0.00%> (ø)`
...ar/common/policies/data/InactiveTopicPolicies.java	`83.33% <0.00%> (ø)`
...r/authentication/AuthenticationProviderAthenz.java	`69.64% <0.00%> (ø)`
...r/io/hdfs3/sink/text/HdfsAbstractTextFileSink.java	`80.00% <0.00%> (ø)`
... and 1354 more

heesung-sn · 2023-03-08T22:39:43Z

pulsar-broker/pom.xml

            <configuration>
              <apiSources>
                <apiSource>
                  <springmvc>false</springmvc>
+                  <operationIdFormat>{{className}}_{{methodName}}</operationIdFormat>
+                  <outputFormats>json</outputFormats>


nit: Is this json outputFormats necessary?

nit: Is this json outputFormats necessary?

It should be optional, but maven or swagger had issues, and I had null Pointers when testing locally. I tried troubleshooting the issue yesterday but couldn't get the root cause quickly, so I thought adding this configuration would avoid unnecessary trouble.

heesung-sn · 2023-03-08T22:42:06Z

pulsar-broker/pom.xml

            <configuration>
              <apiSources>
                <apiSource>
                  <springmvc>false</springmvc>
+                  <operationIdFormat>{{className}}_{{methodName}}</operationIdFormat>


nit: I think {className}_{methodName} is reasonable as the unique operationIdFormat.

LGTM if we don't have duplicated apiSources from the same class and methodName.

After keeping only the latest version of admin API(such as v2, v3), {className}_{methodName} will not conflict. If we open multiple versions of API in the future, we need to change the rule to {package_name}_{className}_{methodName}

tisonkun

Please take a look at the OWASP failure. Although it seems unrelated to this patch, we should fix it later.

poorbarcode · 2023-03-09T15:06:43Z

Hi @tisonkun

Please take a look at the OWASP failure. Although it seems unrelated to this patch, we should fix it later.

I have pushed a #19764 to fix it, so I am merging the current PR now.

github-actions bot added the doc Your PR contains doc changes, no matter whether the changes are in markdown or code files. label Jan 11, 2023

dave2wave reviewed Jan 11, 2023

View reviewed changes

poorbarcode changed the title ~~[fix] [doc] [fix] [doc] fix multiple apis in the automatically generated documentation use the same anchor point~~ [fix] [doc] fix multiple apis in the automatically generated documentation use the same anchor point Jan 11, 2023

poorbarcode force-pushed the doc/fix_admin_api_for_swagger branch from 1a991e1 to baf7e79 Compare January 11, 2023 18:02

poorbarcode requested a review from dave2wave January 12, 2023 02:00

momo-jun mentioned this pull request Feb 8, 2023

[feat][doc] Add docs for message dispatch throttling apache/pulsar-site#386

Merged

4 tasks

github-actions bot added the Stale label Feb 12, 2023

[fix] [doc] fix conflict link of admin api in pulsar-site

e414067

poorbarcode force-pushed the doc/fix_admin_api_for_swagger branch from 413fb63 to e414067 Compare March 8, 2023 07:56

poorbarcode added type/bug The PR fixed a bug or issue reported a bug ready-to-test and removed Stale labels Mar 8, 2023

poorbarcode self-assigned this Mar 8, 2023

poorbarcode added this to the 3.0.0 milestone Mar 8, 2023

tisonkun self-requested a review March 8, 2023 11:04

heesung-sn reviewed Mar 8, 2023

View reviewed changes

heesung-sn approved these changes Mar 8, 2023

View reviewed changes

tisonkun added type/enhancement The enhancements for the existing features or docs. e.g. reduce memory usage of the delayed messages and removed type/bug The PR fixed a bug or issue reported a bug labels Mar 9, 2023

tisonkun approved these changes Mar 9, 2023

View reviewed changes

tisonkun requested review from shoothzj and tuteng March 9, 2023 02:46

poorbarcode merged commit bae28f9 into apache:master Mar 9, 2023

poorbarcode deleted the doc/fix_admin_api_for_swagger branch March 9, 2023 15:24

tisonkun mentioned this pull request Nov 15, 2023

[Doc] all the links to the admin REST API reference doc are broken #21573

Closed

2 tasks

visortelle mentioned this pull request Mar 20, 2024

Use correct REST API URLs in docs apache/pulsar-site#866

Closed

4 tasks

lhotari mentioned this pull request Oct 28, 2024

Fix REST API urls in documentation apache/pulsar-site#970

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[fix] [doc] fix multiple apis in the automatically generated documentation use the same anchor point #19193

[fix] [doc] fix multiple apis in the automatically generated documentation use the same anchor point #19193

poorbarcode commented Jan 11, 2023 •

edited

Loading

dave2wave left a comment

poorbarcode commented Jan 11, 2023

mattisonchao commented Jan 12, 2023 •

edited

Loading

tisonkun commented Jan 12, 2023 •

edited

Loading

shoothzj commented Jan 12, 2023

github-actions bot commented Feb 12, 2023

poorbarcode commented Mar 8, 2023

poorbarcode commented Mar 8, 2023

poorbarcode commented Mar 8, 2023

poorbarcode commented Mar 8, 2023

codecov-commenter commented Mar 8, 2023

heesung-sn Mar 8, 2023 •

edited

Loading

poorbarcode Mar 9, 2023

heesung-sn Mar 8, 2023 •

edited

Loading

poorbarcode Mar 9, 2023

tisonkun left a comment

poorbarcode commented Mar 9, 2023

[fix] [doc] fix multiple apis in the automatically generated documentation use the same anchor point #19193

[fix] [doc] fix multiple apis in the automatically generated documentation use the same anchor point #19193

Conversation

poorbarcode commented Jan 11, 2023 • edited Loading

Motivation

Modifications

Does this pull request potentially affect one of the following parts:

Documentation

Matching PR in forked repository

dave2wave left a comment

Choose a reason for hiding this comment

poorbarcode commented Jan 11, 2023

mattisonchao commented Jan 12, 2023 • edited Loading

tisonkun commented Jan 12, 2023 • edited Loading

shoothzj commented Jan 12, 2023

github-actions bot commented Feb 12, 2023

poorbarcode commented Mar 8, 2023

poorbarcode commented Mar 8, 2023

poorbarcode commented Mar 8, 2023

poorbarcode commented Mar 8, 2023

codecov-commenter commented Mar 8, 2023

Codecov Report

heesung-sn Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

poorbarcode Mar 9, 2023

Choose a reason for hiding this comment

heesung-sn Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

poorbarcode Mar 9, 2023

Choose a reason for hiding this comment

tisonkun left a comment

Choose a reason for hiding this comment

poorbarcode commented Mar 9, 2023

poorbarcode commented Jan 11, 2023 •

edited

Loading

mattisonchao commented Jan 12, 2023 •

edited

Loading

tisonkun commented Jan 12, 2023 •

edited

Loading

heesung-sn Mar 8, 2023 •

edited

Loading

heesung-sn Mar 8, 2023 •

edited

Loading