For exception in de.bytefish.pgbulkinsert.pgsql.handlers.BaseValueHandler

[tangyibo]

In method handle() of class BaseValueHandler, when IOException happend, the exception is not point out explicitly, so I think like this:

public abstract class BaseValueHandler<T> implements IValueHandler<T> {

    @Override
    public void handle(DataOutputStream buffer, final T value) {
		try {
			if (value == null) {
				buffer.writeInt(-1);
				return;
			}
			internalHandle(buffer, value);
		} catch (Exception e) {
			if (e instanceof IOException) {
				if (null != e.getCause()) {
					throw new BinaryWriteFailedException(e.getCause());
				}
			} else {
				throw new BinaryWriteFailedException(e);
			}
		}
	}

    protected abstract void internalHandle(DataOutputStream buffer, final T value) throws Exception;
}

do you agree with me? thanks!

[bytefish]

One problem I see is, that no Exception at all is thrown, when you have an IOException with e.getCause() == null.

The Question is: Why can't we put the Original Exception in the BinaryWriteFailedException(e) and have the caller of the API evaluate the cause? Or is it to complicated to get the information this way?

[tangyibo]

I mean there maybe is IOException which is throwed by function internalHandle()

[bytefish]

Sure, but if an IOException is thrown in this code and e.getCause() is null, then you will fall through all if statements and basically don't rethrow the exception.

[tangyibo]

yes , you are right, may be this?

public abstract class BaseValueHandler<T> implements IValueHandler<T> {

    @Override
    public void handle(DataOutputStream buffer, final T value) {
		try {
			if (value == null) {
				buffer.writeInt(-1);
				return;
			}
			internalHandle(buffer, value);
		} catch (Exception e) {
			if (e instanceof IOException) {
				if (null != e.getCause()) {
					throw new BinaryWriteFailedException(e.getCause());
				}else{
                                       throw new BinaryWriteFailedException(e);
                               }
			} else {
				throw new BinaryWriteFailedException(e);
			}
		}
	}

    protected abstract void internalHandle(DataOutputStream buffer, final T value) throws Exception;
}

[bytefish]

Probably I do not understand something: What's the reason we need to unwrap the Cause from the IOException? I guess it is, so that the Inner Exception of the BinaryWriteFailedException points to the underlying cause directly, right? Is it safe to discard the IOException? Just asking, I am not sure.

[tangyibo]

OK, In abstract method internalHandle() implemnt by subclass, threre many code like this:

buffer.writeInt(...);

and this may throw IOException, but IOException is decoratived for its cause member, so I think maybe like this:

public abstract class BaseValueHandler<T> implements IValueHandler<T> {

	@Override
	public void handle(DataOutputStream buffer, final T value) {
		try {
			if (value == null) {
				buffer.writeInt(-1);
				return;
			}
			internalHandle(buffer, value);
		} catch (IOException e) {
			if (null != e.getCause()) {
				throw new BinaryWriteFailedException(e.getCause());
			} else {
				throw new BinaryWriteFailedException(e);
			}
		}
	}

	protected abstract void internalHandle(DataOutputStream buffer, final T value) throws IOException;
}

[bytefish]

I get it. But does the IOException never contains useful methods + a cause? Because right now, if we are unwrapping the e.getCause(), then the IOException is lost.

[tangyibo]

Of course, What you said is right, and when I see class of org.postgresql.copy.PGCopyOutputStream, I found like this:

  public void write(byte[] buf, int off, int siz) throws IOException {
    checkClosed();
    try {
      writeToCopy(buf, off, siz);
    } catch (SQLException se) {
      IOException ioe = new IOException("Write to copy failed.");
      ioe.initCause(se);
      throw ioe;
    }
  }

  public void close() throws IOException {
    // Don't complain about a double close.
    if (op == null) {
      return;
    }

    try {
      endCopy();
    } catch (SQLException se) {
      IOException ioe = new IOException("Ending write to copy failed.");
      ioe.initCause(se);
      throw ioe;
    }
    op = null;
  }

  public void flush() throws IOException {
    try {
      op.writeToCopy(copyBuffer, 0, at);
      at = 0;
      op.flushCopy();
    } catch (SQLException e) {
      IOException ioe = new IOException("Unable to flush stream");
      ioe.initCause(e);
      throw ioe;
    }
  }

IOException is used for wrapping SQLException , So , I also want you help for what to do for it, thanks!

[bytefish]

Take for example this code:

IOException ioe = new IOException("Write to copy failed.");

If I unwrap the SQLException from the IOException, then the Exception message "Write to copy failed." will be lost for example. I don't think, that is a good thing... what I would do as a user of the API is the following.

I define a class ExceptionUtils:

// Licensed under the MIT license. See LICENSE file in the project root for full license information.

package de.bytefish.pgbulkinsert.util;

import org.checkerframework.checker.nullness.qual.Nullable;

public class ExceptionUtils {

    private ExceptionUtils() {}

    @Nullable
    public static Throwable getRootCause(@Nullable Throwable t) {
        if (t == null) {
            return null;
        }

        Throwable rootCause = null;
        Throwable cause = t.getCause();

        // Now get to the Inner-Most Cause:
        while (cause != null && cause != rootCause) {
            rootCause = cause;
            cause = cause.getCause();
        }

        return rootCause;
    }
}

And if you want to know the root cause of the Exception you can use it like this. I created an Exception Hierarchy equivalent to your code example above and show how to get the SQLException:

// Licensed under the MIT license. See LICENSE file in the project root for full license information.

package de.bytefish.pgbulkinsert.test.util;

import de.bytefish.pgbulkinsert.exceptions.BinaryWriteFailedException;
import de.bytefish.pgbulkinsert.util.ExceptionUtils;
import org.junit.Assert;
import org.junit.Test;

import java.io.IOException;
import java.sql.SQLException;

public class ExceptionUtilsTest {

    @Test
    public void testExceptionUtils() {

        // Exception Hierarchy:
        SQLException sqlException = new SQLException("My SQLException");
        IOException ioException = new IOException("My IOException with a SQLException cause", sqlException);
        BinaryWriteFailedException binaryWriteFailedException = new BinaryWriteFailedException(ioException);

        // Get the root cause:
        Throwable innerMostException = ExceptionUtils.getRootCause(binaryWriteFailedException);

        Assert.assertNotNull(innerMostException);
        Assert.assertEquals("My SQLException", innerMostException.getMessage());
    }
}

Would that resolve the Issue?

[tangyibo]

Ok, Good idea, Thanks ,haha!

[bytefish]

@tangyibo 谢谢!

Good! I don't know, if the SQLException is the innermost Exception in this case. Still this might be better, than swallowing an IOException, because I don't know what other IOException can be thrown in the JDBC code below.