[BUG] Java Doc warnings with gradle check #496

saratvemulapalli · 2023-02-28T00:28:19Z

What is the bug?

Gradle check throws a bunch of java doc warnings.

How can one reproduce the bug?

Checkout SDK repo
Run ./gradlew check

/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/ActionExtension.java:48: warning: no @return
    default List<ActionHandler<? extends ActionRequest, ? extends ActionResponse>> getActions() {
                                                                                   ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/ActionExtension.java:56: warning: no @return
    default List<ActionType<? extends ActionResponse>> getClientActions() {
                                                       ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/ActionExtension.java:63: warning: no @return
    default List<ActionFilter> getActionFilters() {
                               ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/ActionExtension.java:79: warning: no @return
    default Collection<RestHeaderDefinition> getRestHeaders() {
                                             ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/ActionExtension.java:86: warning: no @return
    default Collection<String> getTaskHeaders() {
                               ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/ActionExtension.java:109: warning: no @param for threadContext
    default UnaryOperator<ExtensionRestHandler> getRestHandlerWrapper(ThreadContext threadContext) {
                                                ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/ActionExtension.java:109: warning: no @return
    default UnaryOperator<ExtensionRestHandler> getRestHandlerWrapper(ThreadContext threadContext) {
                                                ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/ActionExtension.java:177: warning: no @return
    default Collection<RequestValidators.RequestValidator<PutMappingRequest>> mappingRequestValidators() {
                                                                              ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/ActionExtension.java:185: warning: no @return
    default Collection<RequestValidators.RequestValidator<IndicesAliasesRequest>> indicesAliasesRequestValidators() {
                                                                                  ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/BaseExtensionRestHandler.java:103: warning: no @param for e
    protected ExtensionRestResponse exceptionalRequest(ExtensionRestRequest request, Exception e) {
                                    ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/ActionExtension.java:125: warning: no @param for action
        public ActionHandler(
               ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/ActionExtension.java:125: warning: no @param for transportAction
        public ActionHandler(
               ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/ActionExtension.java:125: warning: no @param for supportTransportActions
        public ActionHandler(
               ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/EngineExtension.java:36: warning: no @param for indexSettings
    default Optional<EngineFactory> getEngineFactory(IndexSettings indexSettings) {
                                    ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/EngineExtension.java:47: warning: no @param for indexSettings
    default Optional<CodecServiceFactory> getCustomCodecServiceFactory(IndexSettings indexSettings) {
                                          ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/EngineExtension.java:47: warning: no @return
    default Optional<CodecServiceFactory> getCustomCodecServiceFactory(IndexSettings indexSettings) {
                                          ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SDKClient.java:288: warning: no @return
        public SDKRestClient admin() {
                             ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SDKClient.java:295: warning: no @return
        public SDKClusterAdminClient cluster() {
                                     ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SDKClient.java:319: warning: no @return
        public SDKIndicesClient indices() {
                                ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:74: warning: no @return
    default List<ScoreFunctionSpec<?>> getScoreFunctions() {
                                       ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:82: warning: no @return
    default List<SignificanceHeuristicSpec<?>> getSignificanceHeuristics() {
                                               ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:90: warning: no @return
    default List<SearchExtensionSpec<MovAvgModel, MovAvgModel.AbstractModelParser>> getMovingAverageModels() {
                                                                                    ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:97: warning: no @param for context
    default List<FetchSubPhase> getFetchSubPhases(FetchPhaseConstructionContext context) {
                                ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:97: warning: no @return
    default List<FetchSubPhase> getFetchSubPhases(FetchPhaseConstructionContext context) {
                                ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:104: warning: no @return
    default List<SearchExtSpec<?>> getSearchExts() {
                                   ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:111: warning: no @return
    default Map<String, Highlighter> getHighlighters() {
                                     ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:118: warning: no @return
    default List<SuggesterSpec<?>> getSuggesters() {
                                   ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:125: warning: no @return
    default List<QuerySpec<?>> getQueries() {
                               ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:132: warning: no @return
    default List<SortSpec<?>> getSorts() {
                              ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:139: warning: no @return
    default List<AggregationSpec> getAggregations() {
                                  ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:164: warning: no @return
    default List<PipelineAggregationSpec> getPipelineAggregations() {
                                          ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:171: warning: no @return
    default List<RescorerSpec<?>> getRescorers() {
                                  ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:180: warning: no @return
    default Optional<QueryPhaseSearcher> getQueryPhaseSearcher() {
                                         ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:189: warning: no @return
    default Optional<ExecutorServiceProvider> getIndexSearcherExecutorProvider() {
                                              ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:821: warning: no @return
        public ParseField getName() {
                          ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:828: warning: no @return
        public Writeable.Reader<? extends W> getReader() {
                                             ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:835: warning: no @return
        public P getParser() {
                 ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:425: warning: no @param for <T>
        public <T extends AggregationBuilder> AggregationSpec(
                                              ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:443: warning: no @param for <T>
        public <T extends AggregationBuilder> AggregationSpec(String name, Writeable.Reader<T> reader, ContextParser<String, T> parser) {
                                              ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:484: warning: no @param for resultReader
        public AggregationSpec addResultReader(Writeable.Reader<? extends InternalAggregation> resultReader) {
                               ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:484: warning: no @return
        public AggregationSpec addResultReader(Writeable.Reader<? extends InternalAggregation> resultReader) {
                               ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:491: warning: no @param for writeableName
        public AggregationSpec addResultReader(String writeableName, Writeable.Reader<? extends InternalAggregation> resultReader) {
                               ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:491: warning: no @param for resultReader
        public AggregationSpec addResultReader(String writeableName, Writeable.Reader<? extends InternalAggregation> resultReader) {
                               ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:491: warning: no @return
        public AggregationSpec addResultReader(String writeableName, Writeable.Reader<? extends InternalAggregation> resultReader) {
                               ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:499: warning: no @return
        public Map<String, Writeable.Reader<? extends InternalAggregation>> getResultReaders() {
                                                                            ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:507: warning: no @return
        public Consumer<ValuesSourceRegistry.Builder> getAggregatorRegistrar() {
                                                      ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:515: warning: no @param for aggregatorRegistrar
        public AggregationSpec setAggregatorRegistrar(Consumer<ValuesSourceRegistry.Builder> aggregatorRegistrar) {
                               ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:515: warning: no @return
        public AggregationSpec setAggregatorRegistrar(Consumer<ValuesSourceRegistry.Builder> aggregatorRegistrar) {
                               ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:848: warning: no description for @param
         * @param highlighters
           ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:671: warning: no @param for parser
        public PipelineAggregationSpec(
               ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:683: warning: no @param for resultReader
        public PipelineAggregationSpec addResultReader(Writeable.Reader<? extends InternalAggregation> resultReader) {
                                       ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:683: warning: no @return
        public PipelineAggregationSpec addResultReader(Writeable.Reader<? extends InternalAggregation> resultReader) {
                                       ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:690: warning: no @param for writeableName
        public PipelineAggregationSpec addResultReader(String writeableName, Writeable.Reader<? extends InternalAggregation> resultReader) {
                                       ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:690: warning: no @param for resultReader
        public PipelineAggregationSpec addResultReader(String writeableName, Writeable.Reader<? extends InternalAggregation> resultReader) {
                                       ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:690: warning: no @return
        public PipelineAggregationSpec addResultReader(String writeableName, Writeable.Reader<? extends InternalAggregation> resultReader) {
                                       ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:698: warning: no @return
        public Map<String, Writeable.Reader<? extends InternalAggregation>> getResultReaders() {
                                                                            ^
/home/ubuntu/opensearch-sdk-java/src/main/java/org/opensearch/sdk/SearchExtension.java:334: warning: no @return
        public Writeable.Reader<? extends Suggest.Suggestion> getSuggestionReader() {
                                                              ^
57 warnings

What is the expected behavior?

No errors :)

What is your proposed fix?

There are 2 parts to this issue:

Force gradle check to fail when java docs has warnings
Fix all java docs warnings

The text was updated successfully, but these errors were encountered:

owaiskazi19 · 2023-02-28T00:32:07Z

@saratvemulapalli do you think we can add Good First Issue label to this issue?

dbwiddis · 2023-02-28T01:22:09Z

What is the expected behavior?

No errors :)

There are no errors. These are warnings.

This needs to be more of a discussion before we say it's a good first issue.

Switching to a strict mode to force all these to errors will result in a lot of painful, stupid javadocs such as:
- If a class has a no-arg constructor, even if it is the default (non-existent) one, we will have to create a no-arg constructor just to javadoc it with "creates an instance of Foo" where the Foo class already has its own Javadoc
- All getters will need javadocs: "Gets the foo. @return the foo" Or "Sets the foo. @param foo the foo to set".

We specifically put in the "RequireJavadoc" dependency to allow for exceptions in exactly these circumstances.

I opened an issue on OpenSearch outlining much of this same reasoning. See #4541.

I am removing the good first issue tag until we decide if we really really really want boilerplate getter and setter comments and want to force everyone to create no-arg constructors just to javadoc them with "creates an instance of the class that is documented with the class javadoc up there".

dbwiddis · 2023-02-28T01:26:37Z

In case you don't follow the link in the comment listed above, this is what you want if you want to make these errors

dbwiddis · 2023-02-28T16:02:05Z

Also relevant, the discussion on #27 where I thought I had consensus on the "don't do trivial javadocs" front, otherwise I would have put in stricter checks 8 months ago and not gotten here.

Another option is to use checkstyle for javadoc checking. That's what i do on my other repos that I maintain. It's far more configurable, and even catches some cases the existing setup misses. I had avoided it before because the main purpose of checkstyle is duplicated by spotless and it's a heavy dependency, but I'd rather use a screwdriver on a swiss army knife than a single purpose sledgehammer. :)

saratvemulapalli · 2023-02-28T18:58:32Z

There are no errors. These are warnings.

Haha thats true. Its all warnings, I prefer to get the warnings away :).

Switching to a strict mode to force all these to errors will result in a lot of painful

That makes sense, I agree we could discuss this and vote for options folks would prefer.

vyvy3 · 2023-03-08T13:39:57Z

We can use --dont-require-trivial-properties tag for RequireJavaDoc as far as I concerned, so it won't fail on getters/setters/constructors as you mentioned @dbwiddis . I think there's much more configurational stuff for it, the doc is here

If you agree I would like to take this task so it won't throw any warnings while running in --dont-require-trivial-properties mode?

dbwiddis · 2023-03-08T16:43:45Z

We can use --dont-require-trivial-properties tag for RequireJavaDoc as far as I concerned, so it won't fail on getters/setters/constructors as you mentioned @dbwiddis .

We already do that (in fact I'm the one who asked for that feature!)

opensearch-sdk-java/build.gradle

Lines 129 to 133 in 30759d2

    
           args "src/main/java" 
        
           // javadocs on private methods optional 
        
           args "--dont-require-private=true" 
        
           // javadocs on trivial getters/setters optional 
        
           args "--dont-require-trivial-properties"

Unfortunately, RequireJavaDoc only tests for the presence or absence of a javadoc. We still need to use the JDK's doclint tool to test for the correct javadoc. And all this works better on earlier JDKs but starting with JDK 18, the doclint was changed to add more warnings, which is what we are seeing.

If you agree I would like to take this task so it won't throw any warnings while running in --dont-require-trivial-properties mode?

If you'd like to take on this task, that's fine, but I think as-of JDK 18, we might want to remove the RequireJavadoc and doclint configuration and use a completely different setup. Checkstyle does javadoc checking (and a ton of other stuff) and is a lot more configurable, inclding allowing suppression of "trivial" properties. See https://checkstyle.sourceforge.io/config_javadoc.html

I use it on another project I maintain, sample config here (not suggesting to copy it directly, but just an example): https://github.com/oshi/oshi/blob/ccce731ddae489d1541da31d678d3e2e290d985c/config/checkstyle.xml#L314-L320

dbwiddis · 2023-03-08T16:47:34Z

We could also use checkstyle to fail on star imports that we've also hacked into spotless.

owaiskazi19 · 2023-03-08T20:13:23Z

We could also use checkstyle to fail on star imports that we've also hacked into spotless.

We have removed CheckStyle from OpenSearch to have just formatting tool i.e. Spotless. IMO we should avoid doing the same for SDK.

dbwiddis · 2023-03-08T20:52:16Z

We have removed CheckStyle from OpenSearch to have just formatting tool i.e. Spotless.

I'd like a better reason to not use CheckStyle than "have only spotless" because in fact, OpenSearch has multiple tools:

Spotless for formatting
LineLint for end-of-file checking (but doens't fix it) which is really annoying since spotless can do that
missing-doclet "tool" to check javadocs. This is even worse because:
- it's local to the repo, not published anywhere, so other repos have to copy all the code to use it
- it's code copied a few years ago from Solr/Lucene, and not kept up to date with upstream fixes
- it enforces a long list of boilerplate changes like this. The act of creating such boilerplate (creating a no-arg constrictor just to add a javadoc to it) is so painful that NOBODY is fixing Javadocs on OpenSearch.

Spotless doesn't do Javadocs. We need a tool to do javadocs.

We are currently using a mix of tools that this issue has identified as insufficient.

I'm not suggesting to use Checkstyle for formatting. I'm suggesting using it to plug in the gaps where Spotless falls short, which include:

configurable javadoc checks that avoid painful, stupid, useless boilerplate on trivial things like non-existent no-arg constructors, and trivial getters and setters
javadoc checks that make sure all params are included and have the same name as the param (current checks here and at OpenSearch don't)
prohibiting star imports
possibly a few other code style things that spotless doesn't do, like if (foo == true)

Other than "we only want one formatting tool" what is the opposition to using checkstyle for things that aren't formatting?

dbwiddis · 2023-03-08T22:42:53Z

Requirement	Spotless	Linelint	RequireJavadoc	JDK doclint	Checkstyle
Checks formatting	Built in				Built in
Auto-fixes formatting	Built in
Checks CR on EOF	Built in	Built in			Built in
Auto fixes CR on EOF	Built in
Checks Star Import	With hack				Built in
Checks existence of Javadoc			Built in		Built in
Allows skipping trivial javadocs			Built in		Built in
Checks Javaoc param/return				Built in	Built in

owaiskazi19 · 2023-03-13T18:42:34Z

Since we are not using CheckStyle for formatting and only for styling purpose, I'm fine with it. Security plugin is doing the same.

dbwiddis · 2023-03-13T18:48:45Z

Since we are not using CheckStyle for formatting and only for styling purpose, I'm fine with it. Security plugin is doing the same.

And a lot more checks I see. We can consider adding some of those in over time. And also consider spotbugs (leaning yes) and/or PMD (leaning big no) at some point.

owaiskazi19 · 2023-03-13T19:52:03Z

And a lot more checks I see. We can consider adding some of those in over time. And also consider spotbugs (leaning yes) and/or PMD (leaning big no) at some point.

I like spotbugs!

saratvemulapalli added bug Something isn't working untriaged labels Feb 28, 2023

saratvemulapalli added good first issue Good for newcomers and removed untriaged labels Feb 28, 2023

saratvemulapalli mentioned this issue Feb 28, 2023

Force javadoc warnings as errors #497

Closed

dbwiddis added documentation Improvements or additions to documentation discuss and removed bug Something isn't working good first issue Good for newcomers labels Feb 28, 2023

dbwiddis mentioned this issue Mar 8, 2023

Added Changes on SDK Side to register its Implemented Interfaces #508

Merged

dbwiddis self-assigned this Mar 12, 2023

dbwiddis mentioned this issue Mar 12, 2023

Use checkstyle for javadoc checks #551

Merged

dbwiddis closed this as completed in #551 Mar 14, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BUG] Java Doc warnings with gradle check #496

[BUG] Java Doc warnings with gradle check #496

saratvemulapalli commented Feb 28, 2023 •

edited

Loading

owaiskazi19 commented Feb 28, 2023

dbwiddis commented Feb 28, 2023

What is the expected behavior?

dbwiddis commented Feb 28, 2023

dbwiddis commented Feb 28, 2023

saratvemulapalli commented Feb 28, 2023

vyvy3 commented Mar 8, 2023 •

edited

Loading

dbwiddis commented Mar 8, 2023 •

edited

Loading

dbwiddis commented Mar 8, 2023

owaiskazi19 commented Mar 8, 2023 •

edited

Loading

dbwiddis commented Mar 8, 2023

dbwiddis commented Mar 8, 2023

owaiskazi19 commented Mar 13, 2023

dbwiddis commented Mar 13, 2023

owaiskazi19 commented Mar 13, 2023

[BUG] Java Doc warnings with gradle check #496

[BUG] Java Doc warnings with gradle check #496

Comments

saratvemulapalli commented Feb 28, 2023 • edited Loading

What is the bug?

How can one reproduce the bug?

What is the expected behavior?

What is your proposed fix?

owaiskazi19 commented Feb 28, 2023

dbwiddis commented Feb 28, 2023

What is the expected behavior?

dbwiddis commented Feb 28, 2023

dbwiddis commented Feb 28, 2023

saratvemulapalli commented Feb 28, 2023

vyvy3 commented Mar 8, 2023 • edited Loading

dbwiddis commented Mar 8, 2023 • edited Loading

dbwiddis commented Mar 8, 2023

owaiskazi19 commented Mar 8, 2023 • edited Loading

dbwiddis commented Mar 8, 2023

dbwiddis commented Mar 8, 2023

owaiskazi19 commented Mar 13, 2023

dbwiddis commented Mar 13, 2023

owaiskazi19 commented Mar 13, 2023

saratvemulapalli commented Feb 28, 2023 •

edited

Loading

vyvy3 commented Mar 8, 2023 •

edited

Loading

dbwiddis commented Mar 8, 2023 •

edited

Loading

owaiskazi19 commented Mar 8, 2023 •

edited

Loading