Lucene - Core
  1. Lucene - Core
  2. LUCENE-2088

AttributeSource.addAttribute should only accept interfaces, the missing test leads to problems with Token.TOKEN_ATTRIBUTE_FACTORY

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 2.9, 2.9.1, 3.0
    • Fix Version/s: 3.0
    • Component/s: None
    • Labels:
      None
    • Lucene Fields:
      New

      Description

      This is a blocker, because you can call addAttribute(Token.class) without getting an error message.

      I will commit the fix and restart the vote for 3.0. This also applies to 2.9, but there is no Token Attribute Factory. But I will merge to 2.9, too, if a 2.9.2 comes.

      1. LUCENE-2088.patch
        2 kB
        Uwe Schindler
      2. LUCENE-2088-test.patch
        1.0 kB
        Uwe Schindler

        Activity

        Hide
        Uwe Schindler added a comment -

        Here the patch, will commit soon and respawn 3.0.

        I will also merge to 2.9 branch.

        Show
        Uwe Schindler added a comment - Here the patch, will commit soon and respawn 3.0. I will also merge to 2.9 branch.
        Hide
        Earwin Burrfoot added a comment -

        && Attribute.class.isAssignableFrom(attClass)

        What is this for? This line

        public <A extends Attribute> A addAttribute(Class<A> attClass) {

        ensures the same at compile time.

        Show
        Earwin Burrfoot added a comment - && Attribute.class.isAssignableFrom(attClass) What is this for? This line public <A extends Attribute> A addAttribute(Class<A> attClass) { ensures the same at compile time.
        Hide
        Uwe Schindler added a comment -

        If you use it type unsafe without generics, it will break. And we need it for 2.9.

        You can break this if you do:
        addAttribute((Class) List.class)

        I was thinking about both variants and thought it would be better to leave it in. I will merge this now to 2.9, too, where we need it in all cases.

        Show
        Uwe Schindler added a comment - If you use it type unsafe without generics, it will break. And we need it for 2.9. You can break this if you do: addAttribute((Class) List.class) I was thinking about both variants and thought it would be better to leave it in. I will merge this now to 2.9, too, where we need it in all cases.
        Hide
        Uwe Schindler added a comment -

        This patch shows how you can break.

        As Shai said, the problem is not only that it may have no effect, it completely breaks the behaviour of AttributeSource when you do this. Because of that the extra check is needed.

        Show
        Uwe Schindler added a comment - This patch shows how you can break. As Shai said, the problem is not only that it may have no effect, it completely breaks the behaviour of AttributeSource when you do this. Because of that the extra check is needed.
        Hide
        Uwe Schindler added a comment -

        Thinking about it more and reading http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6461827 maybe we should remove it again. But addAttributeImpl already does a lot of isAssignableFrom checks (but cached) so maybe we should remove it for 3.0/3.1. In 2.9 it must stay alive.

        What do others think?

        Show
        Uwe Schindler added a comment - Thinking about it more and reading http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=6461827 maybe we should remove it again. But addAttributeImpl already does a lot of isAssignableFrom checks (but cached) so maybe we should remove it for 3.0/3.1. In 2.9 it must stay alive. What do others think?
        Hide
        Uwe Schindler added a comment -

        But its no problem anymore, the sun bug is fixed since:
        Release Fixed 6u2(b01), 5.0u12(b02) (Bug ID:2144702) , hs10(b07) (Bug ID:2146432) , 7(b07) (Bug ID:2176843)

        Let's keep it in.

        Show
        Uwe Schindler added a comment - But its no problem anymore, the sun bug is fixed since: Release Fixed 6u2(b01), 5.0u12(b02) (Bug ID:2144702) , hs10(b07) (Bug ID:2146432) , 7(b07) (Bug ID:2176843) Let's keep it in.
        Hide
        Uwe Schindler added a comment -

        Forgot to resolve this.

        Show
        Uwe Schindler added a comment - Forgot to resolve this.

          People

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

            Dates

            • Created:
              Updated:
              Resolved:

              Development