#14821 Python: Report specific errors when import_surface fails

- Return std::expected from RimSurfaceCollection::importSurfacesFromFiles
  with a message naming the files that could not be imported
- Return specific errors from ImportSurface and NewSurface scripting
  methods instead of nullptr
- Import before deleting the surface to overwrite, so a failed overwrite
  keeps the existing surface
- Catch surfio exceptions in RifSurfio to avoid a crash on corrupt files
- Include the method name in the generic "no result" gRPC error
- Add Python tests for import_surface error handling

ApplicationLibCode/Commands/EclipseCommands/RicImportEclipseCaseFeature.cpp

@@ -96,7 +96,8 @@ void RicImportEclipseCaseFeature::importPvdSurfacesForGridFiles( const QStringLi
             // Get count of existing surfaces before import
             auto allSurfacesBefore = surfColl->descendantsIncludingThisOfType<RimSurface>();
 
-            surfColl->importSurfacesFromFiles( pvdFilesToImport );
+            // Failed files are logged by importSurfacesFromFiles
+            if ( !surfColl->importSurfacesFromFiles( pvdFilesToImport ) ) return;
 
             // Get newly imported surfaces
             auto                     allSurfacesAfter = surfColl->descendantsIncludingThisOfType<RimSurface>();

ApplicationLibCode/Commands/SurfaceCommands/RicImportSurfacesFeature.cpp

@@ -80,13 +80,13 @@ void RicImportSurfacesFeature::onActionTriggered( bool isChecked )
 
     if ( !surfColl ) return;
 
-    RimSurface* lastCreatedOrUpdated = surfColl->importSurfacesFromFiles( fileNames );
+    auto lastCreatedOrUpdated = surfColl->importSurfacesFromFiles( fileNames );
 
     if ( lastCreatedOrUpdated )
     {
         RicNewGenericDataViewFeature::createInitialViewIfNeeded();
 
-        Riu3DMainWindowTools::selectAsCurrentItem( lastCreatedOrUpdated );
+        Riu3DMainWindowTools::selectAsCurrentItem( lastCreatedOrUpdated.value() );
     }
 }
 

ApplicationLibCode/FileInterface/RifSurfio.cpp

@@ -78,22 +78,27 @@ std::expected<std::pair<RigRegularSurfaceData, std::vector<float>>, std::string>
         return std::make_pair( surfaceData, transposedValues );
     };
 
-    if ( extension == ".gri" )
+    try
     {
-        auto fileData = surfio::irap::from_binary_file( filename );
+        if ( extension == ".gri" )
+        {
+            auto fileData = surfio::irap::from_binary_file( filename );
 
-        return convertToRegularSurface( fileData );
-    }
-    else if ( extension == ".irap" )
-    {
-        auto fileData = surfio::irap::from_ascii_file( filename );
+            return convertToRegularSurface( fileData );
+        }
+        else if ( extension == ".irap" )
+        {
+            auto fileData = surfio::irap::from_ascii_file( filename );
 
-        return convertToRegularSurface( fileData );
+            return convertToRegularSurface( fileData );
+        }
     }
-    else
+    catch ( const std::exception& e )
     {
-        return std::unexpected( "File is not a valid IRAP or GRI file: " + filename );
+        return std::unexpected( "Failed to read surface file " + filename + ": " + e.what() );
     }
+
+    return std::unexpected( "File is not a valid IRAP or GRI file: " + filename );
 }
 
 //--------------------------------------------------------------------------------------------------

ApplicationLibCode/ProjectDataModel/Surfaces/RimSurfaceCollection.cpp

@@ -171,16 +171,24 @@ RimSurface* RimSurfaceCollection::createSurfaceFromFile( const QString& fileName
 //--------------------------------------------------------------------------------------------------
 ///
 //--------------------------------------------------------------------------------------------------
-RimSurface* RimSurfaceCollection::importSurfacesFromFiles( const QStringList& fileNames, bool showLegend /* = true */ )
+std::expected<RimSurface*, QString> RimSurfaceCollection::importSurfacesFromFiles( const QStringList& fileNames, bool showLegend /* = true */ )
 {
-    size_t  newSurfCount      = 0;
-    size_t  existingSurfCount = m_items().size();
-    QString errorMessages;
+    if ( fileNames.isEmpty() ) return std::unexpected( "No surface files to import." );
+
+    size_t      newSurfCount      = 0;
+    size_t      existingSurfCount = m_items().size();
+    QStringList errorMessages;
 
     std::vector<RimSurface*> surfacesToLoad;
 
     for ( const QString& newFileName : fileNames )
     {
+        if ( !QFileInfo::exists( newFileName ) )
+        {
+            errorMessages << QString( "Could not import surface from file '%1'. File does not exist." ).arg( newFileName );
+            continue;
+        }
+
         RimSurface* newSurface = createSurfaceFromFile( newFileName );
 
         auto newColor = RiaColorTables::categoryPaletteColors().cycledColor3f( existingSurfCount + newSurfCount );
@@ -190,7 +198,7 @@ RimSurface* RimSurfaceCollection::importSurfacesFromFiles( const QStringList& fi
         if ( !newSurface->onLoadData() )
         {
             delete newSurface;
-            errorMessages += newFileName + "\n";
+            errorMessages << QString( "Could not import surface from file '%1'." ).arg( newFileName );
         }
         else
         {
@@ -202,21 +210,16 @@ RimSurface* RimSurfaceCollection::importSurfacesFromFiles( const QStringList& fi
 
     if ( !errorMessages.isEmpty() )
     {
-        RiaLogging::warning( "Import Surfaces : Could not import the following files:\n" + errorMessages.toStdString() );
+        RiaLogging::warning( "Import Surfaces : " + errorMessages.join( "\n" ).toStdString() );
     }
 
     updateConnectedEditors();
 
     updateViews( surfacesToLoad, showLegend );
 
-    if ( newSurfCount > 0 && !m_items.empty() )
-    {
-        return m_items[m_items.size() - 1];
-    }
-    else
-    {
-        return nullptr;
-    }
+    if ( surfacesToLoad.empty() ) return std::unexpected( errorMessages.join( "\n" ) );
+
+    return surfacesToLoad.back();
 }
 
 //--------------------------------------------------------------------------------------------------

ApplicationLibCode/ProjectDataModel/Surfaces/RimSurfaceCollection.h

@@ -19,6 +19,8 @@
 
 #include "cafPdmNestedCollection.h"
 
+#include <expected>
+
 class RimSurface;
 class RimEnsembleSurface;
 class RimCase;
@@ -41,7 +43,9 @@ public:
     void                             addEnsembleSurface( RimEnsembleSurface* ensembleSurface );
     std::vector<RimEnsembleSurface*> ensembleSurfaces() const;
 
-    RimSurface* importSurfacesFromFiles( const QStringList& fileNames, bool showLegend = true );
+    // Returns the last imported surface, or an error if no surface could be imported
+    std::expected<RimSurface*, QString> importSurfacesFromFiles( const QStringList& fileNames, bool showLegend = true );
+
     RimSurface* addGridCaseSurface( RimCase* sourceCase, int oneBasedSliceIndex = 1 );
     RimSurface* copySurfaces( std::vector<RimSurface*> surfaces );
     RimSurface* addSurfacesAtIndex( int index, std::vector<RimSurface*> surfaces );

ApplicationLibCode/ProjectDataModelCommands/RimcSurfaceCollection.cpp

@@ -63,7 +63,9 @@ RimcSurfaceCollection_importSurface::RimcSurfaceCollection_importSurface( caf::P
 std::expected<caf::PdmObjectHandle*, QString> RimcSurfaceCollection_importSurface::execute()
 {
     RimSurfaceCollection* coll = self<RimSurfaceCollection>();
-    if ( !coll ) return nullptr;
+    if ( !coll ) return std::unexpected( "Surface collection is null. Cannot import surface." );
+
+    if ( m_fileName().isEmpty() ) return std::unexpected( "File name is empty. Cannot import surface." );
 
     // Imported surfaces are named after the file, see RimSurfaceCollection::importSurfacesFromFiles
     const QString surfaceName = QFileInfo( m_fileName() ).fileName();
@@ -71,14 +73,22 @@ std::expected<caf::PdmObjectHandle*, QString> RimcSurfaceCollection_importSurfac
     auto resolution = RiaNameUniquenessTools::applyConflictPolicy( &coll->itemsField(), surfaceName, m_onNameConflict().value() );
     if ( !resolution.errorMessage.isEmpty() ) return std::unexpected( resolution.errorMessage );
 
+    // Import before deleting a surface to overwrite, so a failed import keeps the existing surface
+    auto result = coll->importSurfacesFromFiles( { m_fileName() } );
+    if ( !result ) return std::unexpected( result.error() );
+
+    RimSurface* newSurface = result.value();
+
     if ( auto* existingSurface = dynamic_cast<RimSurface*>( resolution.objectToReplace ) )
     {
         coll->deleteItem( existingSurface );
     }
 
-    QStringList filelist;
-    filelist << m_fileName();
-    return coll->importSurfacesFromFiles( filelist );
+    // The collection renames the new surface while a surface with the same name exists
+    newSurface->setUserDescription( resolution.nameToUse );
+    coll->updateConnectedEditors();
+
+    return newSurface;
 }
 
 //--------------------------------------------------------------------------------------------------
@@ -107,12 +117,13 @@ RimcSurfaceCollection_newSurface::RimcSurfaceCollection_newSurface( caf::PdmObje
 std::expected<caf::PdmObjectHandle*, QString> RimcSurfaceCollection_newSurface::execute()
 {
     RimSurfaceCollection* coll = self<RimSurfaceCollection>();
-    if ( coll && m_case )
-    {
-        RimSurface* surface = coll->addGridCaseSurface( m_case(), m_kIndex );
-        return surface;
-    }
-    return nullptr;
+    if ( !coll ) return std::unexpected( "Surface collection is null. Cannot create surface." );
+    if ( !m_case ) return std::unexpected( "No case specified. Cannot create surface." );
+
+    RimSurface* surface = coll->addGridCaseSurface( m_case(), m_kIndex );
+    if ( !surface ) return std::unexpected( QString( "Could not create grid case surface for K index %1." ).arg( m_kIndex() ) );
+
+    return surface;
 }
 
 //--------------------------------------------------------------------------------------------------

GrpcInterface/Python/rips/tests/test_surfaces.py

@@ -218,3 +218,68 @@ def test_surface_name_uniqueness(rips_instance, initialize_test):
     folder = surface_collection.add_folder(folder_name="Folder A")
     in_folder = folder.new_regular_surface(name="Top")
     assert in_folder.surface_user_description == "Top"
+
+
+def test_import_surface_errors(rips_instance, initialize_test):
+    case_path = dataroot.PATH + "/Case_with_10_timesteps/Real0/BRUGGE_0000.EGRID"
+    case = rips_instance.project.load_case(path=case_path)
+
+    surface_collection = rips_instance.project.descendants(rips.SurfaceCollection)[0]
+
+    with pytest.raises(rips.RipsError, match="File name is empty"):
+        surface_collection.import_surface(file_name="")
+
+    with tempfile.TemporaryDirectory(prefix="rips") as tmpdirname:
+        missing_path = Path(tmpdirname, "missing.irap").as_posix()
+        with pytest.raises(rips.RipsError, match="missing.irap"):
+            surface_collection.import_surface(file_name=missing_path)
+
+        corrupt_path = Path(tmpdirname, "corrupt.irap")
+        corrupt_path.write_text("this is not a surface file")
+        with pytest.raises(
+            rips.RipsError, match="Could not import surface from file .*corrupt.irap"
+        ):
+            surface_collection.import_surface(file_name=corrupt_path.as_posix())
+
+        surface_path = Path(tmpdirname, "mysurface.ts")
+        surface_collection.new_surface(case, 5).export_to_file(surface_path.as_posix())
+        valid_content = surface_path.read_bytes()
+        imported = surface_collection.import_surface(file_name=surface_path.as_posix())
+        assert imported.surface_user_description == "mysurface.ts"
+
+        def surface_names():
+            return [
+                s.surface_user_description for s in surface_collection.surfaces_field()
+            ]
+
+        # A failed overwrite must keep the existing surface
+        surface_path.unlink()
+        with pytest.raises(rips.RipsError, match="mysurface.ts"):
+            surface_collection.import_surface(
+                file_name=surface_path.as_posix(),
+                on_name_conflict=rips.NameConflictPolicy.OVERWRITE,
+            )
+        assert surface_names().count("mysurface.ts") == 1
+
+        surface_path.write_text("this is not a surface file")
+        with pytest.raises(rips.RipsError, match="mysurface.ts"):
+            surface_collection.import_surface(
+                file_name=surface_path.as_posix(),
+                on_name_conflict=rips.NameConflictPolicy.OVERWRITE,
+            )
+        assert surface_names().count("mysurface.ts") == 1
+
+        # A successful overwrite replaces the surface and keeps the name
+        surface_path.write_bytes(valid_content)
+        overwritten = surface_collection.import_surface(
+            file_name=surface_path.as_posix(),
+            on_name_conflict=rips.NameConflictPolicy.OVERWRITE,
+        )
+        assert overwritten.surface_user_description == "mysurface.ts"
+        assert surface_names().count("mysurface.ts") == 1
+
+        renamed = surface_collection.import_surface(
+            file_name=surface_path.as_posix(),
+            on_name_conflict=rips.NameConflictPolicy.AUTO_RENAME,
+        )
+        assert renamed.surface_user_description == "mysurface.ts_1"

GrpcInterface/RiaGrpcPdmObjectService.cpp

@@ -623,7 +623,8 @@ grpc::Status RiaGrpcPdmObjectService::CallPdmObjectMethod( grpc::ServerContext*
                 telemetryAttributes["pdm.status"] = "failed";
                 telemetryAttributes["pdm.error"]  = "No result returned from Method";
                 RiaOpenTelemetryManager::instance().reportEventAsync( "grpc.pdm_method_call", telemetryAttributes );
-                return grpc::Status( grpc::NOT_FOUND, "No result returned from Method" );
+                return grpc::Status( grpc::NOT_FOUND,
+                                     QString( "Method '%1' returned no result" ).arg( methodKeyword ).toStdString() );
             }
         }