Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: 2.1
    • Fix Version/s: 2.2
    • Component/s: core/index
    • Labels:
      None
    • Environment:

      2.1.0 release

    • Lucene Fields:
      New

      Description

      org.apache.lucene.index.SegmentInfos is public, and contains public methods (which is good for expert-level index manipulation tools such as Luke). However, SegmentInfo class has package visibility. This leads to a strange result that it's possible to read SegmentInfos, but it's not possible to access its details (SegmentInfos.info(int)) from a user application.

      The solution is to make SegmentInfo class public.

      1. LUCENE-811.patch
        12 kB
        Michael McCandless
      2. LUCENE-811-take2.patch
        4 kB
        Michael McCandless

        Activity

        Hide
        Doug Cutting added a comment -

        Rather I think we should consider making SegmentInfos package-private again. This was made public by LUCENE-701. Is it too late to hide it again? Is this really an API we want to support long-term? If there are individual methods that we wish to expose, perhaps they can instead be exposed through IndexReader or IndexWriter. It is a slippery slope to now start making things it references public.

        Perhaps we should in the future try to separate interfaces and implementations into separate packages. The implementation package could be named something like 'index2impl' with the understanding that its public API will not be supported long-term. But, until we do something like that, I think package-private works well. Tools like Luke can add classes to the package in order to access package-private stuff.

        Show
        Doug Cutting added a comment - Rather I think we should consider making SegmentInfos package-private again. This was made public by LUCENE-701 . Is it too late to hide it again? Is this really an API we want to support long-term? If there are individual methods that we wish to expose, perhaps they can instead be exposed through IndexReader or IndexWriter. It is a slippery slope to now start making things it references public. Perhaps we should in the future try to separate interfaces and implementations into separate packages. The implementation package could be named something like 'index2impl' with the understanding that its public API will not be supported long-term. But, until we do something like that, I think package-private works well. Tools like Luke can add classes to the package in order to access package-private stuff.
        Hide
        Michael McCandless added a comment -

        Indeed I did this as part of LUCENE-701, but I can't remember what on
        earth my reasoning was.

        I agree it's not necessary and we should make it package protected
        again. I can make this fix.

        Could we release a 2.0.1 that would include this (and anything else if
        we need to)? I would volunteer to do the release mechanics since I
        caused this (sorry)

        I will also review what else I made public, and, be more careful in
        the future to not relax protection unless it's absolutely needed.

        Show
        Michael McCandless added a comment - Indeed I did this as part of LUCENE-701 , but I can't remember what on earth my reasoning was. I agree it's not necessary and we should make it package protected again. I can make this fix. Could we release a 2.0.1 that would include this (and anything else if we need to)? I would volunteer to do the release mechanics since I caused this (sorry) I will also review what else I made public, and, be more careful in the future to not relax protection unless it's absolutely needed.
        Hide
        Michael McCandless added a comment -

        Sorry, make that 2.1.1 above!

        Show
        Michael McCandless added a comment - Sorry, make that 2.1.1 above!
        Hide
        Hoss Man added a comment -

        since SegmentInfos is final, and all of it's methods are (at best) package protected i don't think it's a big risk to make it private again (without requiring a bump to lucene-3.0.0)

        I'm not sure if re-privatizing this class really neccessitates a 2.1.1 release ... it doesn't cause any bugs, and it does't really give people very much rope to hang themselves with unless they are going out of their way to put stuff in the org.apache.lucene.index package.

        Show
        Hoss Man added a comment - since SegmentInfos is final, and all of it's methods are (at best) package protected i don't think it's a big risk to make it private again (without requiring a bump to lucene-3.0.0) I'm not sure if re-privatizing this class really neccessitates a 2.1.1 release ... it doesn't cause any bugs, and it does't really give people very much rope to hang themselves with unless they are going out of their way to put stuff in the org.apache.lucene.index package.
        Hide
        Michael McCandless added a comment -

        Attaching proposed patch to change SegmentInfos (and all of its methods/fields) back to package level protection.

        I also found a few other cases (IndexFileNames, IndexFileNamesFilter, SegmentInfo) that I had added public methods which should be kept at package protection.

        Show
        Michael McCandless added a comment - Attaching proposed patch to change SegmentInfos (and all of its methods/fields) back to package level protection. I also found a few other cases (IndexFileNames, IndexFileNamesFilter, SegmentInfo) that I had added public methods which should be kept at package protection.
        Hide
        Doug Cutting added a comment -

        I think making some methods public is fine, so long as the class is kept package-private.

        Show
        Doug Cutting added a comment - I think making some methods public is fine, so long as the class is kept package-private.
        Hide
        Andrzej Bialecki added a comment -

        I'm fine with making these classes package-private - Luke already uses a "gateway" class to access SegmentInfo, IndexFileDeleter & friends.

        Show
        Andrzej Bialecki added a comment - I'm fine with making these classes package-private - Luke already uses a "gateway" class to access SegmentInfo, IndexFileDeleter & friends.
        Hide
        Michael McCandless added a comment -

        > I think making some methods public is fine, so long as the class is
        > kept package-private.

        But, for a package-private final class, marking un-inherited
        methods/fields public doesn't enable any additional access since the
        class itself is only visible within the package (I think)? Or am I
        missing something basic about Java's access protections?

        Show
        Michael McCandless added a comment - > I think making some methods public is fine, so long as the class is > kept package-private. But, for a package-private final class, marking un-inherited methods/fields public doesn't enable any additional access since the class itself is only visible within the package (I think)? Or am I missing something basic about Java's access protections?
        Hide
        Doug Cutting added a comment -

        > for a package-private final class, marking un-inherited
        > methods/fields public doesn't enable any additional access

        Right. You were switching several such methods from public to package-private, which is a no-op and prompted my comment. On the other-hand, if the class is ever made public, it may be useful to have its intended public API pre-declared, and, even if it's never made public, declaring some methods public has some documentation value. In general, the style I prefer is try to declare fields and methods as though each class were public. Package-private methods and fields are really only required for classes that actually are public but that wish to hide some things from all but others in their package.

        This is a fine point, and doesn't warrant too much analysis. I probably should have just held my tongue.

        Show
        Doug Cutting added a comment - > for a package-private final class, marking un-inherited > methods/fields public doesn't enable any additional access Right. You were switching several such methods from public to package-private, which is a no-op and prompted my comment. On the other-hand, if the class is ever made public, it may be useful to have its intended public API pre-declared, and, even if it's never made public, declaring some methods public has some documentation value. In general, the style I prefer is try to declare fields and methods as though each class were public. Package-private methods and fields are really only required for classes that actually are public but that wish to hide some things from all but others in their package. This is a fine point, and doesn't warrant too much analysis. I probably should have just held my tongue.
        Hide
        Michael McCandless added a comment -

        Ahhh, OK, I see. I like that style (declaring fields/methods public
        even when the class is package-private). I will re-work that patch
        based on this.

        Show
        Michael McCandless added a comment - Ahhh, OK, I see. I like that style (declaring fields/methods public even when the class is package-private). I will re-work that patch based on this.
        Hide
        Daniel John Debrunner added a comment -

        Doesn't making such methods public mean they can be called trough reflection?

        Show
        Daniel John Debrunner added a comment - Doesn't making such methods public mean they can be called trough reflection?
        Hide
        Michael McCandless added a comment -

        OK fixed the patch to leave some fields/methods public.

        Show
        Michael McCandless added a comment - OK fixed the patch to leave some fields/methods public.
        Hide
        Michael McCandless added a comment -

        This is resolved. If we do a 2.1.1 then I will backport to the 2.1 branch at that time.

        Show
        Michael McCandless added a comment - This is resolved. If we do a 2.1.1 then I will backport to the 2.1 branch at that time.

          People

          • Assignee:
            Michael McCandless
            Reporter:
            Andrzej Bialecki
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development