Avro
  1. Avro
  2. AVRO-1036

IDL processing fails with multi-level nested imports

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.6.2
    • Fix Version/s: 1.6.3
    • Component/s: java
    • Labels:
      None
    • Release Note:
      Fixes bug preventing nested IDL imports
    • Tags:
      IDL

      Description

      The change to support finding IDL related files on the classpath in addition to the maven-plugin defined directory caused the context of the "sourceDirectory" to be lost when the InputStream return by findFile() is used to create a new Idl instance.

      1. AVRO-1036.patch
        11 kB
        Doug Cutting
      2. AVRO-1036.patch
        3 kB
        Doug Cutting
      3. AVRO-1036.patch
        3 kB
        Doug Cutting
      4. jira-1036.patch
        10 kB
        George Fletcher

        Issue Links

          Activity

          Transition Time In Source Status Execution Times Last Executer Last Execution Date
          Open Open Patch Available Patch Available
          3h 7m 1 George Fletcher 23/Feb/12 20:56
          Patch Available Patch Available Resolved Resolved
          1d 2h 50m 1 Doug Cutting 24/Feb/12 23:47
          Resolved Resolved Closed Closed
          23d 16h 46m 1 Doug Cutting 19/Mar/12 16:34
          Doug Cutting made changes -
          Status Resolved [ 5 ] Closed [ 6 ]
          Doug Cutting made changes -
          Status Patch Available [ 10002 ] Resolved [ 5 ]
          Resolution Fixed [ 1 ]
          Hide
          Doug Cutting added a comment -

          I committed this. Thanks, George!

          Show
          Doug Cutting added a comment - I committed this. Thanks, George!
          Doug Cutting made changes -
          Attachment AVRO-1036.patch [ 12515863 ]
          Hide
          Doug Cutting added a comment -

          Oops. Here's the right patch.

          Show
          Doug Cutting added a comment - Oops. Here's the right patch.
          Victor Chau made changes -
          Link This issue is related to AVRO-971 [ AVRO-971 ]
          Hide
          Victor Chau added a comment -

          Sorry, I guess my change from AVRO-971 caused this issue. I just tried out the last patch that Doug uploaded here and nested imports areworking on our environment.

          Show
          Victor Chau added a comment - Sorry, I guess my change from AVRO-971 caused this issue. I just tried out the last patch that Doug uploaded here and nested imports areworking on our environment.
          Hide
          Scott Carey added a comment -

          I do not see any tests in the last patch.

          Show
          Scott Carey added a comment - I do not see any tests in the last patch.
          Doug Cutting made changes -
          Attachment AVRO-1036.patch [ 12515855 ]
          Hide
          Doug Cutting added a comment -

          I don't think we should use ClassLoader across the board, since I don't think relative paths would work correctly that way.

          Here's an updated patch that includes my fix with your tests. Tests fail without the fix and pass with it.

          I'll commit this tomorrow unless there are objections.

          Show
          Doug Cutting added a comment - I don't think we should use ClassLoader across the board, since I don't think relative paths would work correctly that way. Here's an updated patch that includes my fix with your tests. Tests fail without the fix and pass with it. I'll commit this tomorrow unless there are objections.
          Hide
          George Fletcher added a comment -

          Yes, that's a good point and I like your patch better. One other thought I had is that we could just add the sourceDirectory to the ClassLoader in the IDLProtocolMojo.java and then just use the ClassLoader mechanism across the board.

          Show
          George Fletcher added a comment - Yes, that's a good point and I like your patch better. One other thought I had is that we could just add the sourceDirectory to the ClassLoader in the IDLProtocolMojo.java and then just use the ClassLoader mechanism across the board.
          Hide
          Doug Cutting added a comment -

          Looks like we patched this in parallel! You added some tests, which is great!

          My patch is a little different. When you include a file in a different directory then any imports it contains should be relative to its directory, not the directory of the original file, no? So this.inputDir is not the right value for the new inputDir, but rather it should come from the imported file.

          Show
          Doug Cutting added a comment - Looks like we patched this in parallel! You added some tests, which is great! My patch is a little different. When you include a file in a different directory then any imports it contains should be relative to its directory, not the directory of the original file, no? So this.inputDir is not the right value for the new inputDir, but rather it should come from the imported file.
          Doug Cutting made changes -
          Assignee Doug Cutting [ cutting ]
          Doug Cutting made changes -
          Attachment AVRO-1036.patch [ 12515830 ]
          Hide
          Doug Cutting added a comment -

          Here's a patch that should fix this.

          Does this fix things for you?

          If so, we still need a unit test that illustrates the problem before we can commit it.

          Show
          Doug Cutting added a comment - Here's a patch that should fix this. Does this fix things for you? If so, we still need a unit test that illustrates the problem before we can commit it.
          George Fletcher made changes -
          Attachment jira-1036.patch [ 12515814 ]
          George Fletcher made changes -
          Field Original Value New Value
          Status Open [ 1 ] Patch Available [ 10002 ]
          Release Note Fixes bug preventing nested IDL imports
          Hide
          George Fletcher added a comment -

          Patch file created with IntelliJ IDE. Please let me know if that causes problems.

          Show
          George Fletcher added a comment - Patch file created with IntelliJ IDE. Please let me know if that causes problems.
          Hide
          George Fletcher added a comment -

          Decided a cleaner solution was to create a new constructor public Idl (InputStream, File, ClassLoader) that chains the source directory and classpath resource loader into the new Idl instance.

          Show
          George Fletcher added a comment - Decided a cleaner solution was to create a new constructor public Idl (InputStream, File, ClassLoader) that chains the source directory and classpath resource loader into the new Idl instance.
          Hide
          George Fletcher added a comment -

          So the bug is that findFile() returns and InputStream and the Idl class constructor that takes an InputStream is created by the javacc processing of the Idl.jj file. This Idl constructor does not set the inputDir field of the Idl class and hence the Idl object created from the stream returned by findFile() has lost the sourceDirectory. This causes an import statement processed by this Idl object to fail, because the file can not be found as the "sourceDirectory is "lost".

          I'm not very happy with my current solution as it seems a little "hacky". Suggestions appreciated.

          Current solution...

          1. Add a setSrcDir() method to the Idl class (added to Idl.jj)
          2. Modify the ImportIdl() method to first create the Idl instance from the InputStream returned by findFile() and then call the setSrcDir() setter chaining the current inputDir into the created Idl instance.

          Show
          George Fletcher added a comment - So the bug is that findFile() returns and InputStream and the Idl class constructor that takes an InputStream is created by the javacc processing of the Idl.jj file. This Idl constructor does not set the inputDir field of the Idl class and hence the Idl object created from the stream returned by findFile() has lost the sourceDirectory. This causes an import statement processed by this Idl object to fail, because the file can not be found as the "sourceDirectory is "lost". I'm not very happy with my current solution as it seems a little "hacky". Suggestions appreciated. Current solution... 1. Add a setSrcDir() method to the Idl class (added to Idl.jj) 2. Modify the ImportIdl() method to first create the Idl instance from the InputStream returned by findFile() and then call the setSrcDir() setter chaining the current inputDir into the created Idl instance.
          George Fletcher created issue -

            People

            • Assignee:
              Doug Cutting
              Reporter:
              George Fletcher
            • Votes:
              1 Vote for this issue
              Watchers:
              2 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development