From 78c9b9260a5287d09c87b407e396021590714513 Mon Sep 17 00:00:00 2001 From: Darius Jokilehto Date: Fri, 4 Feb 2022 16:13:27 +0000 Subject: [PATCH] Stop initCause throwing in readAdvertisedRefs BasePackConnection::readAdvertisedRefsImpl was creating an exception by calling `noRepository`, and then blindly calling `initCause` on it. As `noRepository` can be overridden, it's not guaranteed to be missing a cause. BasePackPushConnection overrides `noRepository` and initiates a fetch, which may throw a `NoRemoteRepositoryException` with a cause. In this case calling `initCause` threw an `IllegalStateException`. In order to throw the correct exception, we now return the BasePackPushConnection exception and suppress the one thrown by BasePackConnection Bug: 578511 Change-Id: Ic1018b214be1e83d895979ee6c7cbce3f6765f6f --- .../transport/BasePackConnectionTest.java | 24 ++++- .../transport/BasePackPushConnectionTest.java | 96 +++++++++++++++++++ org.eclipse.jgit/.settings/.api_filters | 8 ++ .../errors/NoRemoteRepositoryException.java | 15 +++ .../jgit/transport/BasePackConnection.java | 11 ++- .../transport/BasePackPushConnection.java | 16 ++-- 6 files changed, 156 insertions(+), 14 deletions(-) create mode 100644 org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/BasePackPushConnectionTest.java diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/BasePackConnectionTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/BasePackConnectionTest.java index 7d438c1dd..505f33490 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/BasePackConnectionTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/BasePackConnectionTest.java @@ -15,11 +15,15 @@ import static org.hamcrest.Matchers.not; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThrows; +import java.io.ByteArrayInputStream; +import java.io.EOFException; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.Map; +import org.eclipse.jgit.errors.NoRemoteRepositoryException; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; import org.eclipse.jgit.lib.ObjectIdRef; @@ -29,6 +33,16 @@ public class BasePackConnectionTest { + @Test + public void testReadAdvertisedRefsShouldThrowExceptionWithOriginalCause() { + try (FailingBasePackConnection basePackConnection = + new FailingBasePackConnection()) { + Exception result = assertThrows(NoRemoteRepositoryException.class, + basePackConnection::readAdvertisedRefs); + assertEquals(EOFException.class, result.getCause().getClass()); + } + } + @Test public void testUpdateWithSymRefsAdds() { final Ref mainRef = new ObjectIdRef.Unpeeled(Ref.Storage.LOOSE, @@ -244,4 +258,12 @@ public void testUpdateWithSymRefsFillInHead() { assertEquals(oidName, headRef.getObjectId().name()); assertEquals(oidName, mainRef.getObjectId().name()); } -} \ No newline at end of file + + private static class FailingBasePackConnection extends BasePackConnection { + FailingBasePackConnection() { + super(new TransportLocal(new URIish(), + new java.io.File(""))); + pckIn = new PacketLineIn(new ByteArrayInputStream(new byte[0])); + } + } +} diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/BasePackPushConnectionTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/BasePackPushConnectionTest.java new file mode 100644 index 000000000..cf8c5ffe7 --- /dev/null +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/BasePackPushConnectionTest.java @@ -0,0 +1,96 @@ +/* + * Copyright (c) 2022 Darius Jokilehto + * Copyright (c) 2022 Antonio Barone + * + * This program and the accompanying materials are made available under the + * terms of the Eclipse Distribution License v. 1.0 which is available at + * https://www.eclipse.org/org/documents/edl-v10.php. + * + * SPDX-License-Identifier: BSD-3-Clause + */ + +package org.eclipse.jgit.transport; + +import org.eclipse.jgit.errors.NoRemoteRepositoryException; +import org.eclipse.jgit.errors.TransportException; +import org.eclipse.jgit.internal.JGitText; +import org.junit.Test; + +import java.io.ByteArrayInputStream; +import java.io.EOFException; +import java.io.IOException; +import java.util.Arrays; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.endsWith; +import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.instanceOf; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertThrows; + +public class BasePackPushConnectionTest { + @Test + public void testNoRemoteRepository() { + NoRemoteRepositoryException openFetchException = + new NoRemoteRepositoryException( new URIish(), "not found"); + IOException ioException = new IOException("not read"); + + try (FailingBasePackPushConnection fbppc = + new FailingBasePackPushConnection(openFetchException)) { + TransportException result = fbppc.noRepository(ioException); + + assertEquals(openFetchException, result); + assertThat(Arrays.asList(result.getSuppressed()), + hasItem(ioException)); + } + } + + @Test + public void testPushNotPermitted() { + URIish uri = new URIish(); + TransportException openFetchException = new TransportException(uri, + "a transport exception"); + IOException ioException = new IOException("not read"); + + try (FailingBasePackPushConnection fbppc = + new FailingBasePackPushConnection(openFetchException)) { + TransportException result = fbppc.noRepository(ioException); + + assertEquals(TransportException.class, result.getClass()); + assertThat(result.getMessage(), + endsWith(JGitText.get().pushNotPermitted)); + assertEquals(openFetchException, result.getCause()); + assertThat(Arrays.asList(result.getSuppressed()), + hasItem(ioException)); + } + } + + @Test + public void testReadAdvertisedRefPropagatesCauseAndSuppressedExceptions() { + IOException ioException = new IOException("not read"); + try (FailingBasePackPushConnection basePackConnection = + new FailingBasePackPushConnection( + new NoRemoteRepositoryException( + new URIish(), "not found", ioException))) { + Exception result = assertThrows(NoRemoteRepositoryException.class, + basePackConnection::readAdvertisedRefs); + assertEquals(ioException, result.getCause()); + assertThat(Arrays.asList(result.getSuppressed()), + hasItem(instanceOf(EOFException.class))); + } + } + + private static class FailingBasePackPushConnection + extends BasePackPushConnection { + FailingBasePackPushConnection(TransportException openFetchException) { + super(new TransportLocal(new URIish(), + new java.io.File("")) { + @Override public FetchConnection openFetch() + throws TransportException { + throw openFetchException; + } + }); + pckIn = new PacketLineIn(new ByteArrayInputStream(new byte[0])); + } + } +} diff --git a/org.eclipse.jgit/.settings/.api_filters b/org.eclipse.jgit/.settings/.api_filters index 83a83817f..acceac05d 100644 --- a/org.eclipse.jgit/.settings/.api_filters +++ b/org.eclipse.jgit/.settings/.api_filters @@ -8,6 +8,14 @@ + + + + + + + + diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/errors/NoRemoteRepositoryException.java b/org.eclipse.jgit/src/org/eclipse/jgit/errors/NoRemoteRepositoryException.java index 58f70f5d4..1dd976cec 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/errors/NoRemoteRepositoryException.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/errors/NoRemoteRepositoryException.java @@ -29,4 +29,19 @@ public class NoRemoteRepositoryException extends TransportException { public NoRemoteRepositoryException(URIish uri, String s) { super(uri, s); } + + /** + * Constructs an exception indicating a repository does not exist. + * + * @param uri + * URI used for transport + * @param s + * message + * @param cause + * root cause exception + * @since 5.13 + */ + public NoRemoteRepositoryException(URIish uri, String s, Throwable cause) { + super(uri, s, cause); + } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackConnection.java index 3826bf740..09c559d7b 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackConnection.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackConnection.java @@ -210,9 +210,7 @@ private boolean readAdvertisedRefsImpl() throws IOException { try { line = readLine(); } catch (EOFException e) { - TransportException noRepo = noRepository(); - noRepo.initCause(e); - throw noRepo; + throw noRepository(e); } if (line != null && VERSION_1.equals(line)) { // Same as V0, except for this extra line. We shouldn't get @@ -567,11 +565,14 @@ static void updateWithSymRefs(Map refMap, Map symRe * * Subclasses may override this method to provide better diagnostics. * + * @param cause + * root cause exception * @return a TransportException saying a repository cannot be found and * possibly why. */ - protected TransportException noRepository() { - return new NoRemoteRepositoryException(uri, JGitText.get().notFound); + protected TransportException noRepository(Throwable cause) { + return new NoRemoteRepositoryException(uri, JGitText.get().notFound, + cause); } /** diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackPushConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackPushConnection.java index eb1d2ac0a..b87a85d93 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackPushConnection.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackPushConnection.java @@ -139,7 +139,7 @@ public void push(final ProgressMonitor monitor, /** {@inheritDoc} */ @Override - protected TransportException noRepository() { + protected TransportException noRepository(Throwable cause) { // Sadly we cannot tell the "invalid URI" case from "push not allowed". // Opening a fetch connection can help us tell the difference, as any // useful repository is going to support fetch if it also would allow @@ -147,18 +147,18 @@ protected TransportException noRepository() { // URI is wrong. Otherwise we can correctly state push isn't allowed // as the fetch connection opened successfully. // + TransportException te; try { transport.openFetch().close(); - } catch (NotSupportedException e) { - // Fall through. + te = new TransportException(uri, JGitText.get().pushNotPermitted); } catch (NoRemoteRepositoryException e) { // Fetch concluded the repository doesn't exist. - // - return e; - } catch (TransportException e) { - // Fall through. + te = e; + } catch (NotSupportedException | TransportException e) { + te = new TransportException(uri, JGitText.get().pushNotPermitted, e); } - return new TransportException(uri, JGitText.get().pushNotPermitted); + te.addSuppressed(cause); + return te; } /**