> On March 7, 2016, 6:33 p.m., Jinmei Liao wrote: > > geode-core/src/main/java/com/gemstone/gemfire/security/GemFireSecurityException.java, > > line 38 > > <https://reviews.apache.org/r/44402/diff/1/?file=1281204#file1281204line38> > > > > Probably I don't understand the purpose of this yet. Why we would need > > to have a cause variable? Is the cuase in the parent class not sufficient? > > Kirk Lund wrote: > Adding a cause variable with custom serialization is how Spring works > around the NamingException (cause) with an unserializable field (resolvedObj > which is frequently LdapCtxt which is unserializable). You can't override the > way in which super.cause is serialized, so the Exception has to have its own > cause field -- super.cause remains null. During serialization, if cause is a > NamingException and if it has an unserializable field in resolvedObj, then > the Exception temporarily nulls it out and immediately restores the field > after serializing itself. > > I copied the approach from Spring Ldap which wraps all NamingExceptions, > but implemented it a little differently specific to our need. > > Jinmei Liao wrote: > I see. Thanks! If this is used to handle serialization problem, would > GemfireException be a better place for it? I saw it has some commented out > code about "cause".....
I took a look at GemFireException. That dead code is from early GemFire when the JDK Throwable didn't support cause. We should delete the deadcode. I'd prefer to keep the custom serialization for NamingException as close to the code that needs it as possible. It's only needed by our security exceptions that wrap ldap NamingException. If we ever find ourselves using non-security exceptions to wrap NamingException, then we could always move the custom cause to a different exception class. I think that if security is the only code section that needs this then we should solve the problem there. - Kirk ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/44402/#review122325 ----------------------------------------------------------- On March 4, 2016, 7:28 p.m., Kirk Lund wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/44402/ > ----------------------------------------------------------- > > (Updated March 4, 2016, 7:28 p.m.) > > > Review request for geode, Jens Deppe and Jinmei Liao. > > > Bugs: GEODE-949 > https://issues.apache.org/jira/browse/GEODE-949 > > > Repository: geode > > > Description > ------- > > GEODE-949: fix serialization of GemFireSecurityExceptions to improve debugging > > * add workarounds to security exceptions for unserializable fields (inspired > by similiar code in Spring ldap) > * add cause for security exceptions in test code instead of eating exceptions > * cleanup javadocs of security exceptions > * introduce unit tests > * replace hardcoded class strings with class getName in security test code to > help facilitate repackaging > > > Diffs > ----- > > > geode-core/src/main/java/com/gemstone/gemfire/security/GemFireSecurityException.java > 1f97420 > > geode-core/src/main/java/com/gemstone/gemfire/security/NotAuthorizedException.java > c6165a6 > > geode-core/src/test/java/com/gemstone/gemfire/security/GemFireSecurityExceptionTest.java > PRE-CREATION > > geode-core/src/test/java/com/gemstone/gemfire/security/NotAuthorizedExceptionTest.java > PRE-CREATION > geode-core/src/test/java/security/AuthzCredentialGenerator.java e15a60a > geode-core/src/test/java/security/CredentialGenerator.java 7a430f1 > geode-core/src/test/java/security/DummyAuthzCredentialGenerator.java > 7e40d13 > geode-core/src/test/java/security/DummyCredentialGenerator.java 86b26a7 > geode-core/src/test/java/security/LdapUserCredentialGenerator.java 12bcb62 > geode-core/src/test/java/security/PKCSCredentialGenerator.java 24c0100 > geode-core/src/test/java/security/SSLCredentialGenerator.java 29a1a30 > geode-core/src/test/java/security/UserPasswordWithExtraPropsAuthInit.java > a41f73a > geode-core/src/test/java/security/XmlAuthzCredentialGenerator.java 929eafb > geode-core/src/test/java/templates/security/DummyAuthenticator.java 5d33f22 > geode-core/src/test/java/templates/security/DummyAuthorization.java fe8e908 > geode-core/src/test/java/templates/security/FunctionSecurityPrmsHolder.java > 76827bb > geode-core/src/test/java/templates/security/LdapUserAuthenticator.java > db55219 > geode-core/src/test/java/templates/security/PKCSAuthInit.java d43b78e > geode-core/src/test/java/templates/security/PKCSAuthenticator.java d3610c4 > geode-core/src/test/java/templates/security/PKCSPrincipal.java 563689b > geode-core/src/test/java/templates/security/PKCSPrincipalTest.java > PRE-CREATION > geode-core/src/test/java/templates/security/UserPasswordAuthInit.java > f4b6eec > geode-core/src/test/java/templates/security/UsernamePrincipal.java 739dd52 > geode-core/src/test/java/templates/security/UsernamePrincipalTest.java > PRE-CREATION > geode-core/src/test/java/templates/security/XmlAuthorization.java 1ed0142 > geode-core/src/test/java/templates/security/XmlErrorHandler.java 5da8e09 > > geode-core/src/test/resources/com/gemstone/gemfire/codeAnalysis/sanctionedSerializables.txt > f3c1c5d > > Diff: https://reviews.apache.org/r/44402/diff/ > > > Testing > ------- > > precheckin > > > Thanks, > > Kirk Lund > >
