7
\$\begingroup\$

I know that there are several ways to handle exceptions and probably a good way is to keep the handling consistently and use the pattern which feels comfortable for himself, but I would like to get some advice or hints, what would you do in following situation or how I could improve such a case.

I created a custom URLClassLoader which checks the certification and some advanced stuff. With following function I load the X509 certificate into the instance:

public boolean loadX509Certificate(File file) {
    try {
        CertificateFactory cf = CertificateFactory.getInstance("X.509");
        InputStream is = new FileInputStream(file);
        this.setCertificate(cf.generateCertificate(is));
        is.close();
    } catch (CertificateException e) {
        return false;
    } catch (FileNotFoundException e) {
        return false;
    } catch (SecurityException e) {
        return false;
    } catch (IOException e) {
        return false;
    }
    return true;
}

In that function I catch four exceptions, what would you do in this situation? Return false if the certificate cannot load, rethrow the exception or create a custom exception which tells the user what went wrong?

In generally the Class will work without the certification, but if the value doesn't exist, the class will not check the classes and resources signature.

\$\endgroup\$
  • 1
    \$\begingroup\$ You may want to wrap is.close() in a try-catch to separate important I/O errors while reading vs. those you can ignore when closing the stream. \$\endgroup\$ – David Harkness Dec 1 '13 at 19:50
5
\$\begingroup\$

First, I would separate loading the certificate from storing it.

  • They are independent concerns.
  • It allows multiple ways to load a certificate.
  • It improves testability.

As Kinjal suggests, logging the error is a good alternative when failure is recoverable. However, I would throw a custom exception (or use CertificateException) and let the caller decide to log and continue or terminate. I really don't like returning success codes when I can avoid it because it muddies the code with checks.

Finally, instead of setting a certificate or leaving the field blank consider modeling a certificate checking strategy. This would require a simple one-method interface CertificateCheck with two implementations: NullCertificateCheck and BasicCertificateCheck. Again this improves testability by providing a seam for mocking, separating concerns, etc. It also removes the need for if (this.certificate != null) checks and allows for more strategies and certificate types.

public void setupCertificate() {
    try {
        certificateCheck = loadX590Certificate(...);
    }
    catch (InvalidCertificateException e) {
        LOG.warn("Skipping certificate check", e);
        certificateCheck = new NullCertificateCheck();
    }
}

private CertificateCheck loadX509Certificate(File file) throws CertificateException {
    InputStream is = null;
    try {
        CertificateFactory cf = CertificateFactory.getInstance("X.509");
        is = new FileInputStream(file);
        return new BasicCertificateCheck(cf.generateCertificate(is));
    }
    catch (FileNotFoundException e) {
        throw new CertificateException("Certificate file not found", e);
    }
    ... more exceptions ...
    finally {
        IOUtils.closeQuietly(is);
    }
}

Update: Since you're storing the certificate (or not) so you can check its validity, I assumed you were checking it at a later time and ideally multiple times. If this is the case, extract the logic into a CertificateCheck hierarchy using the Strategy pattern. If the certificate is checked once in isolation of any other data, this isn't necessary and I see no reason to store the certificate in an instance field.

Note: I'm using Checkable as a stand-in for whatever contains the information that must be checked against the certificate. Your implementation would need to pass whatever is appropriate.

public interface CertificateCheck {
    boolean matches(Checkable thing);
}

// in the absence of a certificate, everything matches
public class NullCertificateCheck implements CertificateCheck {
    public boolean matches(Checkable thing) {
        return true;
    }
}

public class BasicCertificateCheck implements CertificateCheck {
    private final Certificate cert;

    public BasicCertificateCheck(Certificate cert) {
        this.cert = cert;
    }

    public boolean matches(Checkable thing) {
        // move the current checks you have here
    }
}
\$\endgroup\$
  • \$\begingroup\$ Thanks for your reply! Your suggestion looks interesting, but I'm not sure what NullCertificateCheck and BasicCertificateCheck should implement? I guess cf.generateCertificate(is) returns a valid certificate. It would be nice if you could concrete your suggestion. \$\endgroup\$ – Andre Hofmeister Dec 1 '13 at 20:49
  • \$\begingroup\$ @hofmeister - See my update for the details of CertificateCheck. \$\endgroup\$ – David Harkness Dec 2 '13 at 0:14
  • \$\begingroup\$ Thanks for your update! I didn't know about the strategy pattern, very nice. I assume the method in CertificateCheck is called matches. I like to lern new pattern, thanks! \$\endgroup\$ – Andre Hofmeister Dec 2 '13 at 7:13
  • \$\begingroup\$ @hofmeister - Fixed, thanks. HTML forms don't make very good refactoring tools. :D \$\endgroup\$ – David Harkness Dec 2 '13 at 17:15
2
\$\begingroup\$

For Java 7 source code type You could use multicatch. http://www.baptiste-wicht.com/2010/05/better-exception-handling-in-java-7-multicatch-and-final-rethrow/

Personally I would log this error and return false.

\$\endgroup\$
  • 1
    \$\begingroup\$ I recommend changing the reference document to official, and more complete reference document: docs.oracle.com/javase/7/docs/technotes/guides/language/… \$\endgroup\$ – rolfl Dec 1 '13 at 23:55
  • 1
    \$\begingroup\$ In fact, I disagree with some of the semantics in that referenced article... catching Throwable ... really? \$\endgroup\$ – rolfl Dec 1 '13 at 23:58
  • \$\begingroup\$ Why is it bad practice? \$\endgroup\$ – fane89 Dec 2 '13 at 6:55
  • \$\begingroup\$ Because it will catch things like OutOfMemoryError, ThreadDeath, AssertionError, VirtualMachineError.... and these are things you most certainly do not want to affect. You should only catch exceptions you know how to handle. \$\endgroup\$ – rolfl Dec 2 '13 at 6:58
  • \$\begingroup\$ I agree with You. I think it is useful when You are debugging. \$\endgroup\$ – fane89 Dec 2 '13 at 7:02
2
\$\begingroup\$

It depends on your decision if you want to move further when certification fails. You can throw exception if certification is "must" and you do not want to proceed if there is any issue with it.

In other case, if you still want the execution to move further when there is any problem with certification then just logging the message would be fine.

Here I have modified your code --

public boolean loadX509Certificate(File file) {
  InputStream is = null;
  boolean isCertificateLoaded = false;

  try {
    CertificateFactory cf = CertificateFactory.getInstance("X.509");
    is = new FileInputStream(file);

    this.setCertificate(cf.generateCertificate(is));

    isCertificateLoaded = true;
  } catch (CertificateException e) {
    // log certification error 
  } catch (FileNotFoundException e) {
    // log file not found error 
  } catch (SecurityException e) {
    // log security error 
  } catch (IOException e) {
    // log i/o error 
  } finally {
    if (null != is) { is.close(); }
  }

  return isCertificateLoaded;
}
\$\endgroup\$

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.