AuditActionContext assertNotNulls could yield more helpful IllegalArgumentExceptions

[apetro]

Saw this in providing some help with CAS today:

AuditActionContext constructor IllegalArgumentExceptions-out when arguments, e.g. resourceOperatedUpon, are null

assertNotNull(resourceOperatedUpon, "resourceOperatedUpon cannot be null");

The resulting IllegalArgumentException would be more helpful if it included the rest of the arguments to the constructor, to make it easier for the adopter encountering this exception to figure out what context gave rise to it.

Not sure whether and how to accomplish this. Catch the IllegalArgumentException and wrap in an IllegalArgumentException with the fuller context?

public AuditActionContext(final String principal, final String resourceOperatedUpon, final String actionPerformed, final String applicationCode, final Date whenActionWasPerformed, final String clientIpAddress, final String serverIpAddress) {

  try {
    assertNotNull(principal, "principal cannot be null");
    assertNotNull(resourceOperatedUpon, "resourceOperatedUpon cannot be null");
    assertNotNull(actionPerformed, "actionPerformed cannot be null.");
    assertNotNull(applicationCode, "applicationCode cannot be null.");
    assertNotNull(whenActionWasPerformed, "whenActionPerformed cannot be null.");
    assertNotNull(clientIpAddress, "clientIpAddress cannot be null.");
    assertNotNull(serverIpAddress, "serverIpAddress cannot be null.");
  } catch (IllegalArgumentException illegalArgException) {

    throw new IllegalArgumentException("One or more arguments to AuditActionContext constructor were invalid: " +
     "principal: " + principal +
     "resourceOperatedUpon: " + resourceOperatedUpon +
     "actionPerformed: " + actionPerformed +
     "applicationCode: " + applicationCode +
     "whenActionWasPerformed: " + whenActionWasPerformed + 
     "clientIpAddress: " + clientIpAddress + 
     "serverIpAddress: " + serverIpAddress,
     illegalArgException );

  }
  ...
}

Enhance the message in each assertNotNull ?

Something else?

[dima767]

I'd vote for enhancing the assert messages.

[kostyamakarov]

(reposting as myself :) here is a stack trace from my production tomcat localhost.log I believe Andrew created this issue after trying to help me make sense of this entry.

Nov 9, 2011 12:00:05 AM org.apache.catalina.core.StandardWrapperValve invoke SEVERE: Servlet.service() for servlet [cas] in context with path [/cas] threw exception [Request processing failed; nested exception is java.lang.IllegalArgumentException: resourceOperatedUpon cannot be null] with root cause java.lang.IllegalArgumentException: resourceOperatedUpon cannot be null at com.github.inspektr.audit.AuditActionContext.assertNotNull(AuditActionContext.java:81) at com.github.inspektr.audit.AuditActionContext.(AuditActionContext.java:64) at com.github.inspektr.audit.AuditTrailManagementAspect.executeAuditCode(AuditTrailManagementAspect.java:148) at com.github.inspektr.audit.AuditTrailManagementAspect.handleAuditTrail(AuditTrailManagementAspect.java:139) at sun.reflect.GeneratedMethodAccessor39.invoke(Unknown Source) at sun.reflect.DelegatingMethodAccessorImpl.invoke(Unknown Source) at java.lang.reflect.Method.invoke(Unknown Source) .....

please let me know if you need more info

[apetro]

Enhancing the assert messages sounds fine to me. Dima, do you want to code that change, or do you want me to take a shot at that and submit a pull request implementing it?

[dima767]

Is that a pressing issue at this moment? Are you busy with other stuff? I could get to it tomorrow, or you could do the change if you're not extremely busy, etc. If you want to make a change, sure do that, and also this way we get to practice the git workflow :-) Let me know what's easier for you.

[SCSU]

This is not a pressing issue. It doesn't stop CAS from authenticating users, so I'd say tomorrow is just fine or whenever you have the time. I'm working with Drew on uPortal stuff for our school. We are still at the unconference site.

Thanks! Kostya

-----Original Message----- From: Dmitriy Kopylenko [mailto:reply@reply.github.com] Sent: Thursday, November 10, 2011 4:40 PM To: Makarov, Konstantin V. Subject: Re: [inspektr] AuditActionContext assertNotNulls could yield more helpful IllegalArgumentExceptions (#13)

Is that a pressing issue at this moment? Are you busy with other stuff? I could get to it tomorrow, or you could do the change if you're not extremely busy, etc. If you want to make a change, sure do that, and also this way we get to practice the git workflow :-) Let me know what's easier for you.

---

Reply to this email directly or view it on GitHub: https://github.com/dima767/inspektr/issues/13#issuecomment-2701549

[battags]

The proposed enhancement is less clear. Please do not implement the proposed enhancement as is.

[battags]

Which part of "resourceOperatedUpon cannot be null" is not clear?

[battags]

The message here is misleading: " throw new IllegalArgumentException("One or more arguments to AuditActionContext constructor were invalid: " + "principal: " + principal + "resourceOperatedUpon: " + resourceOperatedUpon + "actionPerformed: " + actionPerformed + "applicationCode: " + applicationCode + "whenActionWasPerformed: " + whenActionWasPerformed + "clientIpAddress: " + clientIpAddress + "serverIpAddress: " + serverIpAddress, illegalArgException );"

Only one item was validated, and you don't know which one it was.

[apetro]

I don't think my newly thrown IllegalArgumentException message is misleading. When it is thrown, it will be true that one or more arguments were invalid. One of the arguments was certainly invalid (that gave rise to the underlying IllegalArgumentException). Possibly other, not-validated-due-to-failing-fast, arguments were also invalid. Hence, "One or more". It chains the underlying IllegalArgumentException so as to retain informing about exactly which argument gave rise to the failure.

No part of "resourceOperatedUpon cannot be null" is not clear as far as it goes. What I'm suggesting is that it doesn't go far enough -- that in practice, seeing this failure in the logs, it is needlessly difficult to figure out in what context resourceOperatedOn was null. At the time the exception is thrown, AuditActionContext has handy additional context (the other arguments to the constructor) that inform about the context in which resourceOperatedUpon was null. I'd like the exception that AuditActionContext raises to include this additional context.

[battags]

I think it is misleading because it implies we checked the other values (which we didn't). I'd actually propose the following:

1. We properly validate ALL arguments at once and return one message with the details

OR

2. Provide that context in the original assert (similar to yours but making it clear we only validated one item)

AND/OR

1. Revisit whether the resource can actually be allowed to be null. (which we may open as a separate issue if we agree that it should be allowed to be null).

[battags]

Apparently I can't number. That last one should be a 3. ;-)

[dima767]

I'll implement the 'providing more context' in the asserts. I'll create a topic branch for that.

[battags]

Cool. Any thoughts about #3?

[dima767]

Sure, we could think about it, no problem, but that should be a "larger" design/thought. In the mean time I'll improve the assert messages, so we could put it in the minor release, or something to that nature.

[dima767]

OK, here's the diagnostic facility for simple audit context asserts: https://github.com/dima767/inspektr/commit/a7c9187fc59db572478929508e0f90bb2ec12da6

You could test it by installing it into local maven repo: 'mvn clean install' The version is 1.0.6.GA-SNAPSHOT

Also here's a fork of Spring MVC Showcase (why re-invent the wheel?) to play with Inspektr:

https://github.com/dima767/spring-mvc-showcase-with-inspektr/blob/master/src/main/webapp/WEB-INF/spring/root-context.xml

Let me know if this is adequate, so I could merge it into the 'master'

Cheers, Dima.