Solr
  1. Solr
  2. SOLR-8333

fix public methods that take/return private/package-private arguments/results

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 5.5, 6.0
    • Component/s: None
    • Labels:
      None

      Description

      background info: http://mail-archives.apache.org/mod_mbox/lucene-dev/201511.mbox/%3Calpine.DEB.2.11.1511231128450.24330@tray%3E

      A commit that added a package to solrj which already existed in solr-core caused the javadoc link checker to uncover at least 4 instances of private or package-private classes being neccessary to use public APIs.

      we should fix these instances and any other instances of APIs with similar problems that we can find.

      1. SOLR-8333.patch
        8 kB
        Erik Hatcher
      2. SOLR-8333-ConcurrentLFUCache-protected.patch
        7 kB
        Shawn Heisey

        Activity

        Hide
        Hoss Man added a comment -

        Known problems

        • ConcurrentLRUCache -> CacheKey
        • ConcurrentLFUCache -> CacheEntry (not the same CacheKey class)
        • HLL -> ISchemaVersion
        • SimplePostTool -> GlobFileFilter
        Show
        Hoss Man added a comment - Known problems ConcurrentLRUCache -> CacheKey ConcurrentLFUCache -> CacheEntry (not the same CacheKey class) HLL -> ISchemaVersion SimplePostTool -> GlobFileFilter
        Hide
        Erik Hatcher added a comment -

        I think here's a patch that fixes SimplePostTool's public API reasonably:

        Index: solr/core/src/java/org/apache/solr/util/SimplePostTool.java
        ===================================================================
        --- solr/core/src/java/org/apache/solr/util/SimplePostTool.java	(revision 1715806)
        +++ solr/core/src/java/org/apache/solr/util/SimplePostTool.java	(working copy)
        @@ -111,7 +111,7 @@
           private int currentDepth;
         
           static HashMap<String,String> mimeMap;
        -  GlobFileFilter globFileFilter;
        +  FileFilter fileFilter;
           // Backlog for crawling
           List<LinkedHashSet<URL>> backlog = new ArrayList<>();
           Set<URL> visited = new HashSet<>();
        @@ -286,7 +286,7 @@
             this.recursive = recursive;
             this.delay = delay;
             this.fileTypes = fileTypes;
        -    this.globFileFilter = getFileFilterFromFileTypes(fileTypes);
        +    this.fileFilter = getFileFilterFromFileTypes(fileTypes);
             this.out = out;
             this.commit = commit;
             this.optimize = optimize;
        @@ -487,9 +487,9 @@
           private int postDirectory(File dir, OutputStream out, String type) {
             if(dir.isHidden() && !dir.getName().equals("."))
               return(0);
        -    info("Indexing directory "+dir.getPath()+" ("+dir.listFiles(globFileFilter).length+" files, depth="+currentDepth+")");
        +    info("Indexing directory "+dir.getPath()+" ("+dir.listFiles(fileFilter).length+" files, depth="+currentDepth+")");
             int posted = 0;
        -    posted += postFiles(dir.listFiles(globFileFilter), out, type);
        +    posted += postFiles(dir.listFiles(fileFilter), out, type);
             if(recursive > currentDepth) {
               for(File d : dir.listFiles()) {
                 if(d.isDirectory()) {
        @@ -965,7 +965,7 @@
             if (null != dest) dest.flush();
           }
         
        -  public GlobFileFilter getFileFilterFromFileTypes(String fileTypes) {
        +  public FileFilter getFileFilterFromFileTypes(String fileTypes) {
             String glob;
             if(fileTypes.equals("*"))
               glob = ".*";
        
        Show
        Erik Hatcher added a comment - I think here's a patch that fixes SimplePostTool's public API reasonably: Index: solr/core/src/java/org/apache/solr/util/SimplePostTool.java =================================================================== --- solr/core/src/java/org/apache/solr/util/SimplePostTool.java (revision 1715806) +++ solr/core/src/java/org/apache/solr/util/SimplePostTool.java (working copy) @@ -111,7 +111,7 @@ private int currentDepth; static HashMap< String , String > mimeMap; - GlobFileFilter globFileFilter; + FileFilter fileFilter; // Backlog for crawling List<LinkedHashSet<URL>> backlog = new ArrayList<>(); Set<URL> visited = new HashSet<>(); @@ -286,7 +286,7 @@ this .recursive = recursive; this .delay = delay; this .fileTypes = fileTypes; - this .globFileFilter = getFileFilterFromFileTypes(fileTypes); + this .fileFilter = getFileFilterFromFileTypes(fileTypes); this .out = out; this .commit = commit; this .optimize = optimize; @@ -487,9 +487,9 @@ private int postDirectory(File dir, OutputStream out, String type) { if (dir.isHidden() && !dir.getName().equals( "." )) return (0); - info( "Indexing directory " +dir.getPath()+ " (" +dir.listFiles(globFileFilter).length+ " files, depth=" +currentDepth+ ")" ); + info( "Indexing directory " +dir.getPath()+ " (" +dir.listFiles(fileFilter).length+ " files, depth=" +currentDepth+ ")" ); int posted = 0; - posted += postFiles(dir.listFiles(globFileFilter), out, type); + posted += postFiles(dir.listFiles(fileFilter), out, type); if (recursive > currentDepth) { for (File d : dir.listFiles()) { if (d.isDirectory()) { @@ -965,7 +965,7 @@ if ( null != dest) dest.flush(); } - public GlobFileFilter getFileFilterFromFileTypes( String fileTypes) { + public FileFilter getFileFilterFromFileTypes( String fileTypes) { String glob; if (fileTypes.equals( "*" )) glob = ".*" ;
        Hide
        Shawn Heisey added a comment -

        Since ConcurrentLFUCache was my creation (mostly based on ConcurrentLRUCache), I've been thinking about how to fix it. Making CacheEntry public is the simple fix, but since the class should only be used internally and never by users, I wonder if this is better:

        • Move it to org.apache.solr.search
        • Remove the "public" modifier on the class so it's the default package-private
        • Change the problem inner class (CacheEntry) and much of what is currently public to protected.
        Show
        Shawn Heisey added a comment - Since ConcurrentLFUCache was my creation (mostly based on ConcurrentLRUCache), I've been thinking about how to fix it. Making CacheEntry public is the simple fix, but since the class should only be used internally and never by users, I wonder if this is better: Move it to org.apache.solr.search Remove the "public" modifier on the class so it's the default package-private Change the problem inner class (CacheEntry) and much of what is currently public to protected.
        Hide
        Shawn Heisey added a comment -

        Possible patch for ConcurrentLFUCache – this involves a package move and visibility changes. Before this is applied "svn mv" is required to move ConcurrentLFUCache.java.

        Show
        Shawn Heisey added a comment - Possible patch for ConcurrentLFUCache – this involves a package move and visibility changes. Before this is applied "svn mv" is required to move ConcurrentLFUCache.java.
        Hide
        Erik Hatcher added a comment -

        And here's a patch for several items that popped up in my `ant precommit`. The CacheEntry business, no opinion on how that should be handled myself. The DateMathParser stuff is odd and I attempted a fix for that in the attached patch but didn't have success with it.

        Show
        Erik Hatcher added a comment - And here's a patch for several items that popped up in my `ant precommit`. The CacheEntry business, no opinion on how that should be handled myself. The DateMathParser stuff is odd and I attempted a fix for that in the attached patch but didn't have success with it.
        Hide
        ASF subversion and git services added a comment -

        Commit 1715999 from Erik Hatcher in branch 'dev/trunk'
        [ https://svn.apache.org/r1715999 ]

        SOLR-8333: fix public API from referring to inner class implementation

        Show
        ASF subversion and git services added a comment - Commit 1715999 from Erik Hatcher in branch 'dev/trunk' [ https://svn.apache.org/r1715999 ] SOLR-8333 : fix public API from referring to inner class implementation
        Hide
        ASF subversion and git services added a comment -

        Commit 1716000 from Erik Hatcher in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1716000 ]

        SOLR-8333: fix public API from referring to inner class implementation (merged from trunk r1715999)

        Show
        ASF subversion and git services added a comment - Commit 1716000 from Erik Hatcher in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1716000 ] SOLR-8333 : fix public API from referring to inner class implementation (merged from trunk r1715999)
        Hide
        Erik Hatcher added a comment -

        I went ahead and committed the fix to SPT and its test to knock one of these off the list.

        Show
        Erik Hatcher added a comment - I went ahead and committed the fix to SPT and its test to knock one of these off the list.
        Hide
        Erik Hatcher added a comment -

        While these issues the documentation checker found are mostly valid, would the workaround of moving EmptyEntityResolver to org.apache.solr.common package in solrj be desirable?

        Show
        Erik Hatcher added a comment - While these issues the documentation checker found are mostly valid, would the workaround of moving EmptyEntityResolver to org.apache.solr.common package in solrj be desirable?
        Hide
        Hoss Man added a comment -

        erik: please do not conflate/convolute 2 independent issues.

        Regardless of what/how/why we discovered the "public referring to private" problems in the 4 classes I mentioned, they need to be fixed.

        That is 100% independent of the question of where EmptyEntityResolver resolver belongs – it should have no direct bearing on this jira, it should either be tracked in it's own jira, or SOLR-8307 should be re-opened)

        Show
        Hoss Man added a comment - erik: please do not conflate/convolute 2 independent issues. Regardless of what/how/why we discovered the "public referring to private" problems in the 4 classes I mentioned, they need to be fixed. That is 100% independent of the question of where EmptyEntityResolver resolver belongs – it should have no direct bearing on this jira, it should either be tracked in it's own jira, or SOLR-8307 should be re-opened)
        Hide
        Erik Hatcher added a comment -

        I've confirmed that "ant documentation-lint" is happy when:

        A  +    solr/solrj/src/java/org/apache/solr/common/EmptyEntityResolver.java
        D       solr/solrj/src/java/org/apache/solr/util/EmptyEntityResolver.java
        

        This class is final, but if it helps we could copy/paste this same class to core's org.apache.solr.util so that any external references to it don't break (but I doubt this is externally used and if so, that's an ok break with me).

        Show
        Erik Hatcher added a comment - I've confirmed that "ant documentation-lint" is happy when: A + solr/solrj/src/java/org/apache/solr/common/EmptyEntityResolver.java D solr/solrj/src/java/org/apache/solr/util/EmptyEntityResolver.java This class is final, but if it helps we could copy/paste this same class to core's org.apache.solr.util so that any external references to it don't break (but I doubt this is externally used and if so, that's an ok break with me).
        Hide
        Erik Hatcher added a comment -

        SOLR-8307 has been resolved (by moving EmptyEntityResolver to a non-overlapping package), and thus these errors are no longer reported by documentation-lint.

        Show
        Erik Hatcher added a comment - SOLR-8307 has been resolved (by moving EmptyEntityResolver to a non-overlapping package), and thus these errors are no longer reported by documentation-lint.
        Hide
        Hoss Man added a comment -

        ... Making CacheEntry public is the simple fix, but since the class should only be used internally and never by users, I wonder if this is better: ...

        Shawn: I think any API improvements/refacotring/class-moving should be tracked in a dedicated issue, where the questions of backcompat and code structure can be considered appropriately, and decisions can be made about wehter those changes are trunk only or 5x, etc...

        For this issue i really think we should focus on the minimum viable changes that can be made to the existing APIs in terms of class level visibility in order for the APIs to not be broken in 5.x - ideally in such a way that we don't break any existing user plugins that might be using these classes...

        • erik already fixed hte issue with SimplePostTool -> GlobFileFilter by making the public API refer to an appropriate public abstraction/interface rather then the concrete impl.
        • for HLL -> ISchemaVersion we should make ISchemaVersion public
          • it was public in the original java-hll project, i'm not sure why dawid removed that when importing
        • for ConcurrentLRUCache & ConcurrentLFUCache I think we should go ahead and make the respective static inner CacheEntry classes public for now.

        any concerns with these solutions to address the immediate problems?

        (I have yet to find any automated tools that might make it easy to fail the build if any other such API problems exist)

        Show
        Hoss Man added a comment - ... Making CacheEntry public is the simple fix, but since the class should only be used internally and never by users, I wonder if this is better: ... Shawn: I think any API improvements/refacotring/class-moving should be tracked in a dedicated issue, where the questions of backcompat and code structure can be considered appropriately, and decisions can be made about wehter those changes are trunk only or 5x, etc... For this issue i really think we should focus on the minimum viable changes that can be made to the existing APIs in terms of class level visibility in order for the APIs to not be broken in 5.x - ideally in such a way that we don't break any existing user plugins that might be using these classes... erik already fixed hte issue with SimplePostTool -> GlobFileFilter by making the public API refer to an appropriate public abstraction/interface rather then the concrete impl. for HLL -> ISchemaVersion we should make ISchemaVersion public it was public in the original java-hll project, i'm not sure why dawid removed that when importing for ConcurrentLRUCache & ConcurrentLFUCache I think we should go ahead and make the respective static inner CacheEntry classes public for now. any concerns with these solutions to address the immediate problems? (I have yet to find any automated tools that might make it easy to fail the build if any other such API problems exist)
        Hide
        Shawn Heisey added a comment -

        +1 on your plan for the cache classes.

        Show
        Shawn Heisey added a comment - +1 on your plan for the cache classes.
        Hide
        ASF subversion and git services added a comment -

        Commit 1717554 from hossman@apache.org in branch 'dev/trunk'
        [ https://svn.apache.org/r1717554 ]

        SOLR-8333: Several API tweaks so that public APIs were no longer refering to private classes

        Show
        ASF subversion and git services added a comment - Commit 1717554 from hossman@apache.org in branch 'dev/trunk' [ https://svn.apache.org/r1717554 ] SOLR-8333 : Several API tweaks so that public APIs were no longer refering to private classes
        Hide
        ASF subversion and git services added a comment -

        Commit 1717556 from hossman@apache.org in branch 'dev/branches/branch_5x'
        [ https://svn.apache.org/r1717556 ]

        SOLR-8333: Several API tweaks so that public APIs were no longer refering to private classes (merge r1717554)

        Show
        ASF subversion and git services added a comment - Commit 1717556 from hossman@apache.org in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1717556 ] SOLR-8333 : Several API tweaks so that public APIs were no longer refering to private classes (merge r1717554)
        Hide
        Hoss Man added a comment -

        fixed as described.

        NOTE: I didn't see a compelling reason to risk backporting to the 5_4 branch giving the impending RC.

        Show
        Hoss Man added a comment - fixed as described. NOTE: I didn't see a compelling reason to risk backporting to the 5_4 branch giving the impending RC.

          People

          • Assignee:
            Hoss Man
            Reporter:
            Hoss Man
          • Votes:
            1 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development