SOLR-16271: remove wildcard imports #938

epugh · 2022-07-12T17:53:18Z

https://issues.apache.org/jira/browse/SOLR-16271

Description

Remove wildcard imports from the build. In later issue, will add a spotless based check.

Solution

made the changes.

Tests

manually run tests.

Checklist

Please review the following and check all that apply:

I have reviewed the guidelines for How to Contribute and my code conforms to the standards described there to the best of my ability.
I have created a Jira issue and added the issue ID to my pull request title.
I have given Solr maintainers access to contribute to my PR branch. (optional but recommended)
I have developed this patch against the main branch.
I have run ./gradlew check.
I have added tests for my changes.
I have added documentation for the Reference Guide

epugh · 2022-07-12T17:54:21Z

solr/core/src/test/org/apache/solr/handler/TestHealthCheckHandlerLegacyMode.java

@@ -16,7 +16,13 @@
 */
 package org.apache.solr.handler;

-import static org.apache.solr.handler.ReplicationTestHelper.*;


@dsmiley what do you think of this style?

dsmiley · 2022-07-12T20:09:29Z

Thanks for giving this a shot @epugh!

I now remember what I found when Googling how to get Spotless to do this some months ago. It's this technique used on another project (Geode): https://apache.googlesource.com/geode/+/refs/tags/rel/v1.11.0/gradle/spotless.gradle I like that it's done as a build hook into Spotless's flexibility as opposed to our home-grown validateSourcePatterns. Maybe we don't need validateSourcePatterns if we have Spotless?

CC @uschindler -- when discussing source validations in general, I recall you have strong opinions

epugh · 2022-07-13T12:27:17Z

Just pushed up the spotless version. We don't get the nice report, since it just stops on the first one with an exception:

> Task :solr:core:spotlessJava FAILED
Step 'Refuse wildcard imports' found problem in 'src/test/org/apache/solr/handler/TestReplicationHandler.java':
Do not use wildcard imports.  'spotlessApply' cannot resolve this issue.
java.lang.AssertionError: Do not use wildcard imports.  'spotlessApply' cannot resolve this issue.
        at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance0(Native Method)
        at java.base/jdk.internal.reflect.NativeConstructorAccessorImpl.newInstance(NativeConstructorAccessorImpl.java:62)
        at java.base/jdk.internal.reflect.DelegatingConstructorAccessorImpl.newInstance(DelegatingConstructorAccessorImpl.java:45)
        at java.base/java.lang.reflect.Constructor.newInstance(Constructor.java:490)
        at org.codehaus.groovy.reflection.CachedConstructor.invoke(CachedConstructor.java:72)
        at org.codehaus.groovy.runtime.callsite.ConstructorSite$ConstructorSiteNoUnwrapNoCoerce.callConstructor(ConstructorSite.java:105)
        at org.codehaus.groovy.runtime.callsite.CallSiteArray.defaultCallConstructor(CallSiteArray.java:59)
        at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callConstructor(AbstractCallSite.java:263)
        at org.codehaus.groovy.runtime.callsite.AbstractCallSite.callConstructor(AbstractCallSite.java:277)
        at spotless_9enkbgsd2kcvij6tpbbr3dzhe$_run_closure1$_closure2$_closure8$_closure10$_closure11.doCall(/Users/epugh/Documents/projects/solr-epugh-2/gradle/validation/spotless.gradle:66)

epugh · 2022-07-13T14:22:44Z

@dsmiley before I go futhur, what do you think of the way I fixed the imports? Does this pattern for both regular and statics make sense?

dsmiley · 2022-07-13T18:35:57Z

No wildcards -- not because of me or you or whatever IntelliJ or other IDE ships by default, but because we are trying to adhere to a particular style guide: https://google.github.io/styleguide/javaguide.html#s3.3.1-wildcard-imports

epugh · 2022-07-13T18:43:13Z

awesome, so, is everything in this style guide "solr community accepted"? As in, can I reference this to understand what our style is? We should have a FAQ entry about that for the next person ;-)

uschindler · 2022-07-13T18:56:48Z

Lucene agreed to the Spotless code formatting with the Google Java. In Lucene's precommit will fail, if you have code not formatted like that. In general it's plain easy: run "./gradlew tidy" before commit. No need to take care anything. If your IDE has slightly different style it does not matter if you run the command before commit. 👍

uschindler · 2022-07-13T18:58:18Z

I don't think this PR is needed. I'd just go with spotless.

epugh · 2022-07-13T19:17:33Z

I believe that we have to manually deal with the import statements, that spotless can alert, but can't fix it....?

uschindler · 2022-07-13T19:42:19Z

From my knowledge, spotless can deal with imports. It uses javac to scan the code, so it understands how to expand wildcards.

dsmiley · 2022-07-13T20:28:31Z

Our repo already documents adherence to "Google Java Format" -- https://github.com/apache/solr/blob/main/help/formatting.txt but perhaps we should restate this in more places.

Spotless supports multiple formats, including "Google Java Style Guide", but it has limitations.

Also CC @dweiss as you may be interested in this.

epugh · 2022-07-13T22:46:07Z

From my knowledge, spotless can deal with imports. It uses javac to scan the code, so it understands how to expand wildcards.

From my investigation, spotless can reorder imports, but not expand them.... Any links/docs on how to enable it?

dsmiley · 2022-07-15T03:30:28Z

Just a quick tip: IntelliJ can do all the work here across the project; no need to open any source file at all. Just select the root of the project in the navigation tree and choose "Optimize Imports" in the Code menu. It might be influenced by having the "Google-java-format" plugin enabled.

dsmiley · 2022-07-15T03:32:26Z

To keep the build changes clearly visible from a massive commit that makes us adhere, I propose doing this in two PRs; first actually optimizes imports via the IDE (a different PR), and the second (this PR) then enforces the rule.

epugh · 2022-07-15T14:28:14Z

To keep the build changes clearly visible from a massive commit that makes us adhere, I propose doing this in two PRs; first actually optimizes imports via the IDE (a different PR), and the second (this PR) then enforces the rule.

Could we flip it? Only that this PR says "remove wildcard imports", and I'll add another that is "Check and prevent wildcard imports" ???

epugh · 2022-07-15T14:28:40Z

Just a quick tip: IntelliJ can do all the work here across the project; no need to open any source file at all. Just select the root of the project in the navigation tree and choose "Optimize Imports" in the Code menu. It might be influenced by having the "Google-java-format" plugin enabled.

Trying it now! Still learning my way around intellij, thanks for sharing!

epugh · 2022-07-15T14:35:44Z

@dsmiley the organize imports work great, however intellij had different order then spotless, so re-ran spotless to restore orginal order ,and now I am about to push up the explicit import changes.

Will move the enforcement code to another issue.

madrob · 2022-07-15T14:35:47Z

Just a note that some imports exist because of javadoc references and I think automated tools sometimes has issues in the past with removing them too aggressively. Worth a check after changes instead of blindly committing, and if you see it hopefully this is enough warning that you won't be surprised :)

epugh · 2022-07-15T14:52:48Z

@madrob I did notice the // jdoc got removed.... I never did figure out what they did!

epugh · 2022-07-15T15:00:10Z

I ran gw clean and then gw assemble and the ref guide seemed okay. Linked to the solr javadocs.... The solrj pulling in code in the ref guide seemed good as well.

Other suggestions on what to verify?

cpoerschke

Thanks for tackling this!

119 / 119 files viewed

cpoerschke · 2022-07-15T15:15:38Z

solr/core/src/java/org/apache/solr/parser/QueryParser.java

+import org.apache.lucene.analysis.Analyzer;
+import org.apache.lucene.queryparser.charstream.CharStream;
+import org.apache.lucene.queryparser.charstream.FastCharStream;
+import org.apache.lucene.search.BooleanClause;
+import org.apache.lucene.search.Query;
+import org.apache.solr.search.QParser;
+import org.apache.solr.search.SyntaxError;
+
 import java.io.StringReader;
 import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Set;

-import org.apache.lucene.analysis.Analyzer;
-import org.apache.lucene.search.BooleanClause;
-import org.apache.lucene.search.Query;
-import org.apache.solr.search.SyntaxError;
-import org.apache.solr.search.QParser;
-
-import org.apache.lucene.queryparser.charstream.CharStream;
-import org.apache.lucene.queryparser.charstream.FastCharStream;
-


minor: technically not wildcard import removal

solr/core/src/test/org/apache/solr/handler/admin/SolrEnvironmentTest.java

madrob · 2022-07-15T15:24:55Z

I did notice the // jdoc got removed.... I never did figure out what they did!

It looks like the comment was the only thing removed, not the import itself. If we can leave the comment somehow then I'd try to keep it, otherwise somebody in the future is inevitably going to try to flag those as unused imports and then stub their toe on it (that person is going to be me, I am the one that will stub his toe)

epugh · 2022-07-15T15:41:12Z

I did notice the // jdoc got removed.... I never did figure out what they did!

It looks like the comment was the only thing removed, not the import itself. If we can leave the comment somehow then I'd try to keep it, otherwise somebody in the future is inevitably going to try to flag those as unused imports and then stub their toe on it (that person is going to be me, I am the one that will stub his toe)

are you saying the text // jdoc is literally the comment? COuld we expand it to be something like // import to support Javadocs or something like that? // jdoc just didn't really mean anything to me!

epugh · 2022-07-15T15:41:57Z

I imagined // jdoc to not be a comment, but to be some sort of crazy annotation or maybe a reminder to someone to come back and "jdoc" that line (whatever that meant!)... So a more verbose comment would make it clearer...

…ntTest.java Co-authored-by: Christine Poerschke <cpoerschke@apache.org>

epugh · 2022-07-15T15:47:28Z

Here is what I found for the text jdoc in the source:

epugh · 2022-07-15T15:52:15Z

Okay, I think I am starting to understand... For the example of SwitchQParserPlugin.java, we reference SearchHandler in the javadocs, and so the import is needed. If I remove the import, then IntelliJ does flag it that it is needed....

So, I guess, if we only have those three files that follow this pattern, then we can leave the comment in, but maybe change it from // jdoc to // support javadoc linking ???

Or, maybe since intellij flags the missing import if you don't have it, then we don't need the comment at all?

cpoerschke

LGTM but also okay with jdoc comments being retained or changed to something similar.

This also removes the // jdoc comments, as IntelliJ does properly flag imports missing that are referenced in JavaDocs. Co-authored-by: Christine Poerschke <cpoerschke@apache.org>

epugh added 2 commits July 12, 2022 13:07

add rule to prevent import .* pattern

5f98be6

replace a static wildcard import with explicit imports

1f32e45

epugh requested a review from dsmiley July 12, 2022 17:53

epugh marked this pull request as draft July 12, 2022 17:53

epugh commented Jul 12, 2022

View reviewed changes

spotless version for preventing wildcard imports

8d7b555

epugh added 4 commits July 13, 2022 08:54

ordering updated by spotless successfullly

dbaec43

conversion

0ac4e07

explicit imports

27b1e67

explicit imports

65eb734

dsmiley requested a review from uschindler July 13, 2022 18:36

epugh added 4 commits July 13, 2022 20:25

explicit imports

7c3fe73

remove the check till this work is done, then renable

8fb386f

explicit imports

bab83e9

spotless

e97be6c

epugh added 3 commits July 15, 2022 10:41

explicit imports

83c4d52

rollback to rpevious version

d38693d

rollback

bc75e7c

epugh mentioned this pull request Jul 15, 2022

SOLR-16297: enable check on wildcards #942

Merged

epugh marked this pull request as ready for review July 15, 2022 14:51

cpoerschke reviewed Jul 15, 2022

View reviewed changes

epugh and others added 2 commits July 15, 2022 11:43

Update solr/core/src/test/org/apache/solr/handler/admin/SolrEnvironme…

1bb76af

…ntTest.java Co-authored-by: Christine Poerschke <cpoerschke@apache.org>

respond to feedback

fa810a8

spotless

4c8a9e7

cpoerschke approved these changes Jul 15, 2022

View reviewed changes

uschindler approved these changes Jul 16, 2022

View reviewed changes

epugh merged commit 6e61811 into apache:main Jul 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SOLR-16271: remove wildcard imports #938

SOLR-16271: remove wildcard imports #938

epugh commented Jul 12, 2022 •

edited

Loading

epugh Jul 12, 2022

dsmiley commented Jul 12, 2022

epugh commented Jul 13, 2022

epugh commented Jul 13, 2022

dsmiley commented Jul 13, 2022

epugh commented Jul 13, 2022

uschindler commented Jul 13, 2022

uschindler commented Jul 13, 2022

epugh commented Jul 13, 2022

uschindler commented Jul 13, 2022

dsmiley commented Jul 13, 2022 •

edited

Loading

epugh commented Jul 13, 2022

dsmiley commented Jul 15, 2022

dsmiley commented Jul 15, 2022

epugh commented Jul 15, 2022

epugh commented Jul 15, 2022

epugh commented Jul 15, 2022

madrob commented Jul 15, 2022

epugh commented Jul 15, 2022

epugh commented Jul 15, 2022

cpoerschke left a comment

cpoerschke Jul 15, 2022

madrob commented Jul 15, 2022

epugh commented Jul 15, 2022

epugh commented Jul 15, 2022

epugh commented Jul 15, 2022

epugh commented Jul 15, 2022

cpoerschke left a comment

SOLR-16271: remove wildcard imports #938

SOLR-16271: remove wildcard imports #938

Conversation

epugh commented Jul 12, 2022 • edited Loading

Description

Solution

Tests

Checklist

epugh Jul 12, 2022

Choose a reason for hiding this comment

dsmiley commented Jul 12, 2022

epugh commented Jul 13, 2022

epugh commented Jul 13, 2022

dsmiley commented Jul 13, 2022

epugh commented Jul 13, 2022

uschindler commented Jul 13, 2022

uschindler commented Jul 13, 2022

epugh commented Jul 13, 2022

uschindler commented Jul 13, 2022

dsmiley commented Jul 13, 2022 • edited Loading

epugh commented Jul 13, 2022

dsmiley commented Jul 15, 2022

dsmiley commented Jul 15, 2022

epugh commented Jul 15, 2022

epugh commented Jul 15, 2022

epugh commented Jul 15, 2022

madrob commented Jul 15, 2022

epugh commented Jul 15, 2022

epugh commented Jul 15, 2022

cpoerschke left a comment

Choose a reason for hiding this comment

cpoerschke Jul 15, 2022

Choose a reason for hiding this comment

madrob commented Jul 15, 2022

epugh commented Jul 15, 2022

epugh commented Jul 15, 2022

epugh commented Jul 15, 2022

epugh commented Jul 15, 2022

cpoerschke left a comment

Choose a reason for hiding this comment

epugh commented Jul 12, 2022 •

edited

Loading

dsmiley commented Jul 13, 2022 •

edited

Loading