Let package_ucp::ContentProvider::createPackage throw exceptions on failure

...instead of returning a null XHierarchicalNameAccess.  Otherwise, UCB's
globalTransfer from a vnd.sun.star.package URL that denotes a file that is not a
zip file to a file URL folder (i.e., to extract the zip content) silently
succeeds but just creates an empty file in the target folder.  That, in turn,
causes "unopkg add foo.oxt", where foo.oxt is a file that is not a zip file, to
silently succeed and add an "empty" extension.

This change is somewhat bold in that it changes createPackage from "can return
empty reference" to "never returns empty reference, but can throw
RuntimeException."  Especially, it considers "empty name" as a (silent)
violation of its contract with its caller now.  I hope this does not affect any
legitimate scenarios---at least, it does not break a "make check" here.  (In
turn, the two package_ucp::Content::getPackage overloads change to never return
a null reference, either.  And I changed the parameters of createPackage, seeing
that all call-sites pass in some PackageUri's getPackage()/getParam() pair.)

Change-Id: I0eab5f8059dfedefc7030da38da528ba21ea8d79

3 files changed, 24 insertions, 62 deletions

diff --git a/ucb/source/ucp/package/pkgcontent.cxx b/ucb/source/ucp/package/pkgcontent.cxx
index d19cccb956e1..be6cdbe5cafa 100644
--- a/ucb/source/ucp/package/pkgcontent.cxx
+++ b/ucb/source/ucp/package/pkgcontent.cxx
@@ -217,7 +217,7 @@ Content* Content::create(
 
     uno::Reference< container::XHierarchicalNameAccess > xPackage;
 
-    xPackage = pProvider->createPackage( aURI.getPackage(), aURI.getParam() );
+    xPackage = pProvider->createPackage( aURI );
 
     uno::Reference< ucb::XContentIdentifier > xId
         = new ::ucbhelper::ContentIdentifier( aURI.getUri() );
@@ -2217,12 +2217,12 @@ uno::Reference< container::XHierarchicalNameAccess > Content::getPackage(
     if ( rURI.getPackage() == m_aUri.getPackage() )
     {
         if ( !m_xPackage.is() )
-            m_xPackage = m_pProvider->createPackage( m_aUri.getPackage(), m_aUri.getParam() );
+            m_xPackage = m_pProvider->createPackage( m_aUri );
 
         return m_xPackage;
     }
 
-    return m_pProvider->createPackage( rURI.getPackage(), rURI.getParam() );
+    return m_pProvider->createPackage( rURI );
 }
 
 //=========================================================================
@@ -2238,10 +2238,7 @@ sal_Bool Content::hasData(
             const PackageUri& rURI,
             uno::Reference< container::XHierarchicalNameAccess > & rxPackage )
 {
-    rxPackage = pProvider->createPackage( rURI.getPackage(), rURI.getParam() );
-    if ( !rxPackage.is() )
-        return sal_False;
-
+    rxPackage = pProvider->createPackage( rURI );
     return rxPackage->hasByHierarchicalName( rURI.getPath() );
 }
 
@@ -2254,9 +2251,6 @@ sal_Bool Content::hasData( const PackageUri& rURI )
     if ( rURI.getPackage() == m_aUri.getPackage() )
     {
         xPackage = getPackage();
-        if ( !xPackage.is() )
-            return sal_False;
-
         return xPackage->hasByHierarchicalName( rURI.getPath() );
     }
 
@@ -2271,9 +2265,7 @@ sal_Bool Content::loadData(
             ContentProperties& rProps,
             uno::Reference< container::XHierarchicalNameAccess > & rxPackage )
 {
-    rxPackage = pProvider->createPackage( rURI.getPackage(), rURI.getParam() );
-    if ( !rxPackage.is() )
-        return sal_False;
+    rxPackage = pProvider->createPackage( rURI );
 
     if ( rURI.isRootFolder() )
     {
@@ -2469,8 +2461,6 @@ sal_Bool Content::renameData(
     PackageUri aURI( xOldId->getContentIdentifier() );
     uno::Reference< container::XHierarchicalNameAccess > xNA = getPackage(
                                                                         aURI );
-    if ( !xNA.is() )
-        return sal_False;
 
     if ( !xNA->hasByHierarchicalName( aURI.getPath() ) )
         return sal_False;
@@ -2508,8 +2498,6 @@ sal_Bool Content::storeData( const uno::Reference< io::XInputStream >& xStream )
     osl::Guard< osl::Mutex > aGuard( m_aMutex );
 
     uno::Reference< container::XHierarchicalNameAccess > xNA = getPackage();
-    if ( !xNA.is() )
-        return sal_False;
 
     uno::Reference< beans::XPropertySet > xPackagePropSet(
                                                     xNA, uno::UNO_QUERY );
@@ -2740,8 +2728,6 @@ sal_Bool Content::removeData()
     osl::Guard< osl::Mutex > aGuard( m_aMutex );
 
     uno::Reference< container::XHierarchicalNameAccess > xNA = getPackage();
-    if ( !xNA.is() )
-        return sal_False;
 
     PackageUri aParentUri( getParentURL() );
     if ( !xNA->hasByHierarchicalName( aParentUri.getPath() ) )
@@ -2785,8 +2771,6 @@ sal_Bool Content::flushData()
     //       by the single entries. Maybe this has to change...
 
     uno::Reference< container::XHierarchicalNameAccess > xNA = getPackage();
-    if ( !xNA.is() )
-        return sal_False;
 
     uno::Reference< util::XChangesBatch > xBatch( xNA, uno::UNO_QUERY );
     if ( !xBatch.is() )
@@ -2815,8 +2799,6 @@ uno::Reference< io::XInputStream > Content::getInputStream()
 
     uno::Reference< io::XInputStream > xStream;
     uno::Reference< container::XHierarchicalNameAccess > xNA = getPackage();
-    if ( !xNA.is() )
-        return xStream;
 
     if ( !xNA->hasByHierarchicalName( m_aUri.getPath() ) )
         return xStream;
@@ -2854,8 +2836,6 @@ uno::Reference< container::XEnumeration > Content::getIterator()
 
     uno::Reference< container::XEnumeration > xIter;
     uno::Reference< container::XHierarchicalNameAccess > xNA = getPackage();
-    if ( !xNA.is() )
-        return xIter;
 
     if ( !xNA->hasByHierarchicalName( m_aUri.getPath() ) )
         return xIter;
diff --git a/ucb/source/ucp/package/pkgprovider.cxx b/ucb/source/ucp/package/pkgprovider.cxx
index c27e3f27633d..8fdb2aafaa00 100644
--- a/ucb/source/ucp/package/pkgprovider.cxx
+++ b/ucb/source/ucp/package/pkgprovider.cxx
@@ -30,6 +30,7 @@
 #include <cppuhelper/weak.hxx>
 #include <ucbhelper/contentidentifier.hxx>
 #include <com/sun/star/container/XHierarchicalNameAccess.hpp>
+#include <com/sun/star/lang/WrappedTargetRuntimeException.hpp>
 #include "pkgprovider.hxx"
 #include "pkgcontent.hxx"
 #include "pkguri.hxx"
@@ -232,17 +233,11 @@ uno::Reference< ucb::XContent > SAL_CALL ContentProvider::queryContent(
 //=========================================================================
 
 uno::Reference< container::XHierarchicalNameAccess >
-ContentProvider::createPackage( const OUString & rName, const OUString & rParam )
+ContentProvider::createPackage( const PackageUri & rURI )
 {
     osl::MutexGuard aGuard( m_aMutex );
 
-    if ( rName.isEmpty() )
-    {
-        OSL_FAIL( "ContentProvider::createPackage - Invalid URL!" );
-        return uno::Reference< container::XHierarchicalNameAccess >();
-    }
-
-    OUString rURL = rName + rParam;
+    OUString rURL = rURI.getPackage() + rURI.getParam();
 
     if ( m_pPackages )
     {
@@ -257,43 +252,30 @@ ContentProvider::createPackage( const OUString & rName, const OUString & rParam
         m_pPackages = new Packages;
 
     // Create new package...
+    uno::Sequence< uno::Any > aArguments( 1 );
+    aArguments[ 0 ] <<= rURL;
+    uno::Reference< container::XHierarchicalNameAccess > xNameAccess;
     try
     {
-        uno::Sequence< uno::Any > aArguments( 1 );
-        aArguments[ 0 ] <<= rURL;
-
-        uno::Reference< uno::XInterface > xIfc
-            = m_xContext->getServiceManager()->createInstanceWithArgumentsAndContext("com.sun.star.packages.comp.ZipPackage",
-                aArguments, m_xContext );
-
-        if ( xIfc.is() )
-        {
-            uno::Reference<
-                container::XHierarchicalNameAccess > xNameAccess(
-                                                        xIfc, uno::UNO_QUERY );
-
-            OSL_ENSURE( xNameAccess.is(),
-                        "ContentProvider::createPackage - "
-                        "Got no hierarchical name access!" );
-
-            rtl::Reference< Package> xPackage
-                = new Package( rURL, xNameAccess, this );
-
-            (*m_pPackages)[ rURL ] = xPackage.get();
-
-            return xPackage.get();
-        }
+        xNameAccess = uno::Reference< container::XHierarchicalNameAccess >(
+            m_xContext->getServiceManager()->createInstanceWithArgumentsAndContext(
+                "com.sun.star.packages.comp.ZipPackage",
+                aArguments, m_xContext ),
+            css::uno::UNO_QUERY_THROW );
     }
     catch ( uno::RuntimeException const & )
     {
-        // createInstanceWithArguemts
+        throw;
     }
-    catch ( uno::Exception const & )
+    catch ( uno::Exception const & e )
     {
-        // createInstanceWithArguemts
+        throw css::lang::WrappedTargetRuntimeException(
+            e.Message, e.Context, css::uno::makeAny(e));
     }
 
-    return uno::Reference< container::XHierarchicalNameAccess >();
+    rtl::Reference< Package> xPackage = new Package( rURL, xNameAccess, this );
+    (*m_pPackages)[ rURL ] = xPackage.get();
+    return xPackage.get();
 }
 
 //=========================================================================
diff --git a/ucb/source/ucp/package/pkgprovider.hxx b/ucb/source/ucp/package/pkgprovider.hxx
index a2d671f0d142..4913442b16e8 100644
--- a/ucb/source/ucp/package/pkgprovider.hxx
+++ b/ucb/source/ucp/package/pkgprovider.hxx
@@ -87,7 +87,7 @@ public:
 
     ::com::sun::star::uno::Reference<
         ::com::sun::star::container::XHierarchicalNameAccess >
-    createPackage( const OUString & rName, const OUString & rParam );
+    createPackage( const PackageUri & rParam );
     sal_Bool
     removePackage( const OUString & rName );
 };