Add support for @ShouldThrowException(DefinitionException.class)

[hutchig]

Signed-off-by: Gordon Hutchison Gordon.Hutchison@gmail.com

Short description of what this resolves:

Attempts to pass wrapped Definition Exceptions to the client

Changes proposed in this pull request:

The log is examined and if wrapped DefinitionExceptions are found
they client is informed.

Fixes: #2153

@gpoul Gerhard - I think this is ready. Happy to make any other changes etc. :-)

[gpoul]

I assume the text for this message is internationalized; do you think we an restrict this only to the error code without the message or find out what the message should be for the currently active locale?

[hutchig]

Good point.

[gpoul]

We will have to change these to be written to the logger only in log.finest()

[hutchig]

OK.

[gpoul]

I think it might make sense here to throw a DeploymentException in this case; or do you think this can be safely ignored and just logged?

[hutchig]

I think you are right.

[gpoul]

I think it might make sense here to throw a DeploymentException in this case; or do you think this can be safely ignored and just logged?

[hutchig]

Again - I agree.

[hutchig]

I will add some more commits, thanks a lot for reviewing. :-)

[gpoul]

The log location seems to be defined by the attributes logDirectory and messageFileName or the respective properties as defined in the knowledge center at https://www.ibm.com/support/knowledgecenter/en/SSEQTP_8.5.5/com.ibm.websphere.wlp.doc/ae/rwlp_logging.html

Do you think it would be easy to retrieve these? Alternatively we could also introduce a new configuration parameter for the wlp-managed-85 container implementation and rely on the developer to provide it.

[hutchig]

Good suggestion, I will investigate further. I also had another idea to explore if the DefinitionException is nested inside the client observed DeploymentException - and if that might be a better way to detect it. This is why I prefixed the PR as "WIP" as guided by the jboss jira web-page to stand for "Work In Progress" :-) The main reason why I posted the PR now is that I don't want Open Liberty development to be using changed versions of teh container internally without opening them up to upstream. But I agree that this PR should be improved.

[hutchig]

I think I can use the System property values (perhaps set in the bootstrap.properties file) of com.ibm.ws.logging.log.directory and com.ibm.ws.logging.message.file.name or, if these are not set, to use the default values. The default value for the logging directory is "By default, logDirectory is set to the LOG_DIR environment variable. The LOG_DIR environment variable is set to WLP_OUTPUT_DIR/serverName/logs by default." WLP_OUTPUT_DIR can be set in the environment. "If this environment variable is not specified, ${server.output.dir} is the same as ${server.config.dir}."
(https://www.ibm.com/support/knowledgecenter/en/was_beta_liberty/com.ibm.websphere.wlp.nd.multiplatform.doc/ae/twlp_admin_customvars.html)
Then....
server.config.dir is documented at https://www.ibm.com/support/knowledgecenter/en/SSEQTP_8.5.5/com.ibm.websphere.wlp.doc/ae/rwlp_dirs.html as
wlp.install.dir/usr/${wlp.user.dir}/servers/<server_name>/[server specific]${server.config.dir}. Perhaps this will all look clearer in Java!

If might be clearer and less brittle over the long term to let Liberty work out the values and then use something like:

Object serverName = new InitialContext().lookup("com.ibm.ws.logging.message.file.name");

To get the resulting value but this may be hard to do from the container code so not sure I can find a way to do this.

[hutchig]

I also need to replace the Weld DefinitionException with the javax.enterprise.inject.spi.DefinitionException (its parent)

[hutchig]

@gpoul Hi Gerhard do you have a preference for indentation. The existing file has a bit of a mixture and there is also https://github.com/jboss/ide-config#readme which has:
Formatting Java Files
JavaDoc parameter information is on the same line, not indented
Line length is 128 characters
Spaces are used instead of tabs
Indentation size is 4
Switch blocks are indented
Formatting Other Files
Line length is 128 characters
Spaces are used instead of tabs
Indentation size is 4
and seems like a good long term rallying flag.

I am happy to fit in with anything you like and happy just either
make this PRs code fit or get the config file and do Ctrl-Shift-F as you prefer?

My own slight preference would be to follow https://github.com/jboss/ide-config#readme
and format all the java files I touch to that using their config file and auto-format, but as I said happy
to fit in with what you want :-)

[gpoul]

@hutchig yes, we're indeed supposed to be following that guideline you found. If you're reformatting existing code, please just make sure to make that a separate commit or PR to make it easier to review, because otherwise everything gets marked as changed.

[gpoul]

@hutchig Can you please just confirm whether you've addressed everything you wanted to address? I'd like to get this merged soon. Oh, and sorry for the delay. :(

[hutchig]

Apologies I do have some more commits to push in and I am planned in to address these (by co-incidence) on Monday. I had missunderstood that container can pass back a wrapped exception that can be picked up by a @ShouldThrow. I have the new code:

   private void throwWrappedExceptionIfFoundInLog(String applicationName) throws IOException, DeploymentException
   {
	   BufferedReader br = null;
	   String messagesFile = null;
	   
      try {
    	     messagesFile = getLogsDirectory() + "/messages.log";
    	     br = new BufferedReader(new InputStreamReader(new FileInputStream(messagesFile)));
             String line;
             while ((line = br.readLine()) != null) {
                 if (line.contains("CWWKZ0002") && line.contains("DefinitionException") && line.contains(applicationName)) {
                        log.finest("DefinitionException found in line" + line + " of file " + messagesFile );   
                        DefinitionException cause = new javax.enterprise.inject.spi.DefinitionException(line);
                        throw new DeploymentException( "Failed to deploy " + applicationName + " on " + containerConfiguration.getServerName(), cause );
                  } else { 
            	    if ( line.contains("DeploymentException") && line.contains(applicationName)) {
                        log.finest("DeploymentException found in line" + line + " of file " + messagesFile );   
                        javax.enterprise.inject.spi.DeploymentException cause = new javax.enterprise.inject.spi.DeploymentException(line);
                        throw new DeploymentException( "Failed to deploy " + applicationName + " on " + containerConfiguration.getServerName(), cause );
                     }
                  }
             }
      } catch (IOException e) {
         log.warning("Exception while reading messages.log: " + messagesFile + e.toString());
         throw e;
      } finally {
            if (br != null) {
               br.close();
            }
      }
   }

(and have rebased to "liberty-managed" structure but just need to do some more testing )
If I was finished by early next week would that be OK?

[gpoul]

that is perfectly fine; we were planning on making a final release this month and I want your PR to be part of that.

…

On Fri, Mar 9, 2018 at 4:15 PM Gordon Hutchison ***@***.***> wrote: Apologies I do have some more commits to push in and I am planned in to address these (by co-incidence) on Monday. I had missunderstood that container can pass back a wrapped exception that can be picked up by a @shouldthrow. I have the new code: private void throwWrappedExceptionIfFoundInLog(String applicationName) throws IOException, DeploymentException { BufferedReader br = null; String messagesFile = null; try { messagesFile = getLogsDirectory() + "/messages.log"; br = new BufferedReader(new InputStreamReader(new FileInputStream(messagesFile))); String line; while ((line = br.readLine()) != null) { if (line.contains("CWWKZ0002") && line.contains("DefinitionException") && line.contains(applicationName)) { log.finest("DefinitionException found in line" + line + " of file " + messagesFile ); DefinitionException cause = new javax.enterprise.inject.spi.DefinitionException(line); throw new DeploymentException( "Failed to deploy " + applicationName + " on " + containerConfiguration.getServerName(), cause ); } else { if ( line.contains("DeploymentException") && line.contains(applicationName)) { log.finest("DeploymentException found in line" + line + " of file " + messagesFile ); javax.enterprise.inject.spi.DeploymentException cause = new javax.enterprise.inject.spi.DeploymentException(line); throw new DeploymentException( "Failed to deploy " + applicationName + " on " + containerConfiguration.getServerName(), cause ); } } } } catch (IOException e) { log.warning("Exception while reading messages.log: " + messagesFile + e.toString()); throw e; } finally { if (br != null) { br.close(); } } } (and have rebased to "liberty-managed" structure but just need to do some more testing ) If I was finshed by early next week would that be OK? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#33 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJ3WmgDhp6UF5Gx1VIIGrBc1XoADZYRks5tcpx0gaJpZM4Qg3Bl> .

[gpoul]

and thanks for the rebase, btw :)

…

On Mon, Mar 12, 2018 at 7:45 AM Gerhard Poul ***@***.***> wrote: that is perfectly fine; we were planning on making a final release this month and I want your PR to be part of that. On Fri, Mar 9, 2018 at 4:15 PM Gordon Hutchison ***@***.***> wrote: > Apologies I do have some more commits to push in and I am planned in to > address these (by co-incidence) on Monday. I had missunderstood that > container can pass back a wrapped exception that can be picked up by a > @shouldthrow. I have the new code: > > private void throwWrappedExceptionIfFoundInLog(String applicationName) throws IOException, DeploymentException > { > BufferedReader br = null; > String messagesFile = null; > > try { > messagesFile = getLogsDirectory() + "/messages.log"; > br = new BufferedReader(new InputStreamReader(new FileInputStream(messagesFile))); > String line; > while ((line = br.readLine()) != null) { > if (line.contains("CWWKZ0002") && line.contains("DefinitionException") && line.contains(applicationName)) { > log.finest("DefinitionException found in line" + line + " of file " + messagesFile ); > DefinitionException cause = new javax.enterprise.inject.spi.DefinitionException(line); > throw new DeploymentException( "Failed to deploy " + applicationName + " on " + containerConfiguration.getServerName(), cause ); > } else { > if ( line.contains("DeploymentException") && line.contains(applicationName)) { > log.finest("DeploymentException found in line" + line + " of file " + messagesFile ); > javax.enterprise.inject.spi.DeploymentException cause = new javax.enterprise.inject.spi.DeploymentException(line); > throw new DeploymentException( "Failed to deploy " + applicationName + " on " + containerConfiguration.getServerName(), cause ); > } > } > } > } catch (IOException e) { > log.warning("Exception while reading messages.log: " + messagesFile + e.toString()); > throw e; > } finally { > if (br != null) { > br.close(); > } > } > } > > (and have rebased to "liberty-managed" structure but just need to do some > more testing ) > If I was finshed by early next week would that be OK? > > — > You are receiving this because you were mentioned. > > > Reply to this email directly, view it on GitHub > <#33 (comment)>, > or mute the thread > <https://github.com/notifications/unsubscribe-auth/AAJ3WmgDhp6UF5Gx1VIIGrBc1XoADZYRks5tcpx0gaJpZM4Qg3Bl> > . >

[hutchig]

I will remove the WIP prefix from the title of the PR and remove and add you as a reviewer when I have finished (this round of) my changes.

[gpoul]

@hutchig I think we're missing some pom.xml change as my local build fails with the same issue you can see in the automated builds that the "package javax.enterprise.inject.spi does not exist"

[gpoul]

I got it to work locally by adding this:

<dependency>
    <groupId>javax.enterprise</groupId>
    <artifactId>cdi-api</artifactId>
    <version>2.0</version>
    <scope>provided</scope>
</dependency>

but I'd like to know which ones is the right version and dependency :)

[gpoul]

Otherwise I went through the changes and it looks good to me and I'll merge the change when we figured out the missing dependency.

[hutchig]

Apologies - yes I missed the pom as I had changed gradle co-ordinates for my personal version and forgot the extra dependency - I will just go and get the diff now. Apologies.

[hutchig]

I had

         <dependency>
            <groupId>javax.enterprise</groupId>
            <artifactId>cdi-api</artifactId>
            <version>1.2</version>
        </dependency>

[hutchig]

I will just do another commit now....

[hutchig]

Another topic on my 'things to look at' list is the dependency

    <dependency>
      <groupId>com.sun</groupId>
      <artifactId>tools</artifactId>
      <version>1.6.0</version>
      <scope>system</scope>
      <systemPath>${java.home}/../lib/tools.jar</systemPath>
    </dependency>

Under Java 9.
We are just exploring building in Java 9 now and I have a git issue
as tools.jar is not there. Once I have something more helpful to
say on the topic I will do a jira/PR.

Thanks so much for being the steward of this software which enables
us to have multi-vendor TCKs for the microprofile specs.

[gpoul]

@hutchig feel free to open an issue on this project to track the Java 9 support work; unless there is a ticket in OpenLiberty, though... but you might also want to link them then...

The only thing we use from the tools.jar is the Java Attach API; I think this might be useful, but it will require some investigation and testing: https://stackoverflow.com/questions/35240134/declare-maven-dependency-on-tools-jar-to-work-on-jdk-9

[hutchig]

Thanks for the link Gerhard, I will investigate.