-
Notifications
You must be signed in to change notification settings - Fork 60
AuditActionContext assertNotNulls could yield more helpful IllegalArgumentExceptions #13
Comments
I'd vote for enhancing the assert messages. |
(reposting as myself :) Nov 9, 2011 12:00:05 AM org.apache.catalina.core.StandardWrapperValve invoke please let me know if you need more info |
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? |
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. |
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. Thanks! -----Original Message----- 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: |
The proposed enhancement is less clear. Please do not implement the proposed enhancement as is. |
Which part of "resourceOperatedUpon cannot be null" is not clear? |
The message here is misleading: Only one item was validated, and you don't know which one it was. |
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. |
I think it is misleading because it implies we checked the other values (which we didn't). I'd actually propose the following:
OR AND/OR
|
Apparently I can't number. That last one should be a 3. ;-) |
I'll implement the 'providing more context' in the asserts. I'll create a topic branch for that. |
Cool. Any thoughts about #3? |
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. |
OK, here's the diagnostic facility for simple audit context asserts: a7c9187 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: Let me know if this is adequate, so I could merge it into the 'master' Cheers, |
Saw this in providing some help with CAS today:
AuditActionContext constructor IllegalArgumentExceptions-out when arguments, e.g. resourceOperatedUpon, are 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?
Enhance the message in each assertNotNull ?
Something else?
The text was updated successfully, but these errors were encountered: