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

          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.
          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 -

          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
          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.
          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.
          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 -

          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
          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.
          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
          Doug Cutting added a comment -

          Oops. Here's the right patch.

          Show
          Doug Cutting added a comment - Oops. Here's the right patch.
          Hide
          Doug Cutting added a comment -

          I committed this. Thanks, George!

          Show
          Doug Cutting added a comment - I committed this. Thanks, George!

            People

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

              Dates

              • Created:
                Updated:
                Resolved:

                Development