Lucene - Core
  1. Lucene - Core
  2. LUCENE-6651

Remove private field reflection (setAccessible) in AttributeImpl#reflectWith

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 5.2.1
    • Fix Version/s: 5.3, 6.0
    • Component/s: core/other
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      In AttributeImpl we currently have a "default" implementation of reflectWith (which is used by toString() and other methods) that uses reflection to list all private fields of the implementation class and reports them to the AttributeReflector (used by Solr and Elasticsearch to show analysis output).

      Unfortunately this default implementation needs to access private fields of a subclass, which does not work without doing Field#setAccessible(true). And this is done without AccessController#doPrivileged()!

      There are 2 solutions to solve this:

      • Reimplement the whole thing with MethodHandles. MethodHandles allow to access private fields, if you have a MethodHandles.Lookup object created from inside the subclass. The idea is to add a protected constructor taking a Lookup object (must come from same class). This Lookup object is then used to build methodHandles that can be executed to report the fields. Backside: We have to require subclasses that want this "automatic" reflection to pass a Lookup object in ctor's super(MethodHandles.lookup()) call. This breaks backwards for implementors of AttributeImpls
      • The second idea is to remove the whole reflectWith default impl and make the method abstract. This would require a bit more work in tons of AttributeImpl classes, but you already have to implement something like this for equals/hashCode, so its just listing all fields. This would of couse break backwards, too. So my plan would be to implement the missing methods everywhere (as if it were abstract), but keep the default implementation in 5.x. We just would do AccessController.doPrivileged().
      1. LUCENE-6651.patch
        23 kB
        Uwe Schindler
      2. LUCENE-6651.patch
        22 kB
        Uwe Schindler
      3. LUCENE-6651-5x.patch
        26 kB
        Uwe Schindler
      4. LUCENE-6651-MethodHandles.patch
        38 kB
        Uwe Schindler

        Issue Links

          Activity

          Hide
          Uwe Schindler added a comment -

          I just spend a lot of time to implement the first approach - and works quite good. But I dont think we should go that way. I have the feeling, it was a cool experience with MethodHandles (see AttributeImpl#makeReflectors), but too complicated for nothing.

          I will provide another patch using the reflectWith() -> abstract path.

          Any comments?

          Show
          Uwe Schindler added a comment - I just spend a lot of time to implement the first approach - and works quite good. But I dont think we should go that way. I have the feeling, it was a cool experience with MethodHandles (see AttributeImpl#makeReflectors), but too complicated for nothing. I will provide another patch using the reflectWith() -> abstract path. Any comments?
          Hide
          Uwe Schindler added a comment -

          I opened LUCENE-6652 to remove a lot of duplicate BytesRefAttributes in tests. This should be done before this, because it makes the patch a pain.

          Show
          Uwe Schindler added a comment - I opened LUCENE-6652 to remove a lot of duplicate BytesRefAttributes in tests. This should be done before this, because it makes the patch a pain.
          Hide
          Robert Muir added a comment -

          I like the idea of the simple solution! I agree with you, it is better to be simple here.

          Show
          Robert Muir added a comment - I like the idea of the simple solution! I agree with you, it is better to be simple here.
          Hide
          Uwe Schindler added a comment -

          So coming back to this one. After cleanup of TermToBytesRefAttribute and test code duplication (LUCENE-6652, LUCENE-6653), I can fix the attributes to implement reflectWith() and make the base method abstract in trunk.

          Show
          Uwe Schindler added a comment - So coming back to this one. After cleanup of TermToBytesRefAttribute and test code duplication ( LUCENE-6652 , LUCENE-6653 ), I can fix the attributes to implement reflectWith() and make the base method abstract in trunk.
          Hide
          Uwe Schindler added a comment -

          Patch.

          This also adds a missing clone() to one of the attributes encountered.

          Show
          Uwe Schindler added a comment - Patch. This also adds a missing clone() to one of the attributes encountered.
          Hide
          Robert Muir added a comment -

          +1

          Show
          Robert Muir added a comment - +1
          Hide
          Uwe Schindler added a comment - - edited

          Minor updates, adding Java 8 @FunctionalInterface to AttributeReflector.

          Wil commit this soon and provide a patch for branch_5x, with all implementations but preserving the reflection inside a AccessController.doPrivileged() for backwards compatibility.

          Show
          Uwe Schindler added a comment - - edited Minor updates, adding Java 8 @FunctionalInterface to AttributeReflector. Wil commit this soon and provide a patch for branch_5x, with all implementations but preserving the reflection inside a AccessController.doPrivileged() for backwards compatibility.
          Hide
          ASF subversion and git services added a comment -

          Commit 1688855 from Uwe Schindler in branch 'dev/trunk'
          [ https://svn.apache.org/r1688855 ]

          LUCENE-6651: AttributeImpl#reflectWith(AttributeReflector) was made abstract and has no reflection-based default implementation anymore.

          Show
          ASF subversion and git services added a comment - Commit 1688855 from Uwe Schindler in branch 'dev/trunk' [ https://svn.apache.org/r1688855 ] LUCENE-6651 : AttributeImpl#reflectWith(AttributeReflector) was made abstract and has no reflection-based default implementation anymore.
          Hide
          Mike Drob added a comment -

          Nit: ComputedRangesAttributeImpl c = (ComputedRangesAttributeImpl) super.clone();; double semi-colon.

          Why remove UniqueFieldAttributeImpl.toString? Seems like it would be safe to leave it even if it is not strictly necessary, yes?

          Show
          Mike Drob added a comment - Nit: ComputedRangesAttributeImpl c = (ComputedRangesAttributeImpl) super.clone();; double semi-colon. Why remove UniqueFieldAttributeImpl.toString ? Seems like it would be safe to leave it even if it is not strictly necessary, yes?
          Hide
          Uwe Schindler added a comment -

          Patch for 5.x, using AccessController.doPrivileged.

          If needed for Elasticsearch, I can backport just the changes in AttributeImpl to 5.2.2!

          Show
          Uwe Schindler added a comment - Patch for 5.x, using AccessController.doPrivileged. If needed for Elasticsearch, I can backport just the changes in AttributeImpl to 5.2.2!
          Hide
          Uwe Schindler added a comment -

          toString() should not be implemented, because its done automatically using reflectWith().

          Show
          Uwe Schindler added a comment - toString() should not be implemented, because its done automatically using reflectWith().
          Hide
          Uwe Schindler added a comment -

          The only special case is CharTermAttributeImpl, because it needs to comply with CharSequence API. This is also one reason why we added explicit attribute reflection back in 3.x.

          Show
          Uwe Schindler added a comment - The only special case is CharTermAttributeImpl, because it needs to comply with CharSequence API. This is also one reason why we added explicit attribute reflection back in 3.x.
          Hide
          ASF subversion and git services added a comment -

          Commit 1688863 from Uwe Schindler in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1688863 ]

          LUCENE-6651: AttributeImpl#reflectWith(AttributeReflector)'s default Impl was "deprecated" in 5.x. All code should implement this. In addition the default impl is now using AccessController.doPrivileged() to do the accessibility changes.

          Show
          ASF subversion and git services added a comment - Commit 1688863 from Uwe Schindler in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1688863 ] LUCENE-6651 : AttributeImpl#reflectWith(AttributeReflector)'s default Impl was "deprecated" in 5.x. All code should implement this. In addition the default impl is now using AccessController.doPrivileged() to do the accessibility changes.
          Hide
          Uwe Schindler added a comment -

          Please repoen for backport of AttributeImpl#reflectWith(AttributeReflector) to 5.2.2!

          Show
          Uwe Schindler added a comment - Please repoen for backport of AttributeImpl#reflectWith(AttributeReflector) to 5.2.2!
          Hide
          ASF subversion and git services added a comment -

          Commit 1688898 from Uwe Schindler in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1688898 ]

          LUCENE-6651: Add a test for the old reflector, also without rights

          Show
          ASF subversion and git services added a comment - Commit 1688898 from Uwe Schindler in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1688898 ] LUCENE-6651 : Add a test for the old reflector, also without rights
          Hide
          Uwe Schindler added a comment -

          I added a test for the old reflector code, so we ensure that it works. It is also tested with lower rights.

          Show
          Uwe Schindler added a comment - I added a test for the old reflector code, so we ensure that it works. It is also tested with lower rights.
          Hide
          ASF subversion and git services added a comment -

          Commit 1688899 from Uwe Schindler in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1688899 ]

          LUCENE-6651: Remove useless class descriptor

          Show
          ASF subversion and git services added a comment - Commit 1688899 from Uwe Schindler in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1688899 ] LUCENE-6651 : Remove useless class descriptor
          Hide
          ASF subversion and git services added a comment -

          Commit 1688903 from Uwe Schindler in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1688903 ]

          LUCENE-6651: Try to fix test (somehow empty permissions grant all)

          Show
          ASF subversion and git services added a comment - Commit 1688903 from Uwe Schindler in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1688903 ] LUCENE-6651 : Try to fix test (somehow empty permissions grant all)
          Hide
          ASF subversion and git services added a comment -

          Commit 1688905 from Uwe Schindler in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1688905 ]

          LUCENE-6651: Comment out test for now (fails only on Linux, no idea why!)

          Show
          ASF subversion and git services added a comment - Commit 1688905 from Uwe Schindler in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1688905 ] LUCENE-6651 : Comment out test for now (fails only on Linux, no idea why!)
          Hide
          ASF subversion and git services added a comment -

          Commit 1688913 from Uwe Schindler in branch 'dev/branches/branch_5x'
          [ https://svn.apache.org/r1688913 ]

          LUCENE-6651: Remove test completely: Cannot work (inner doPrivileged reverts back to codebase granted permissions; no idea why it worked for me)

          Show
          ASF subversion and git services added a comment - Commit 1688913 from Uwe Schindler in branch 'dev/branches/branch_5x' [ https://svn.apache.org/r1688913 ] LUCENE-6651 : Remove test completely: Cannot work (inner doPrivileged reverts back to codebase granted permissions; no idea why it worked for me)
          Hide
          Shalin Shekhar Mangar added a comment -

          Bulk close for 5.3.0 release

          Show
          Shalin Shekhar Mangar added a comment - Bulk close for 5.3.0 release

            People

            • Assignee:
              Uwe Schindler
              Reporter:
              Uwe Schindler
            • Votes:
              0 Vote for this issue
              Watchers:
              3 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development