diff --git a/ApplicationLibCode/Commands/EclipseCommands/RicImportEclipseCaseFeature.cpp b/ApplicationLibCode/Commands/EclipseCommands/RicImportEclipseCaseFeature.cpp index 43d1c06b30..92dda8f50e 100644 --- a/ApplicationLibCode/Commands/EclipseCommands/RicImportEclipseCaseFeature.cpp +++ b/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(); - surfColl->importSurfacesFromFiles( pvdFilesToImport ); + // Failed files are logged by importSurfacesFromFiles + if ( !surfColl->importSurfacesFromFiles( pvdFilesToImport ) ) return; // Get newly imported surfaces auto allSurfacesAfter = surfColl->descendantsIncludingThisOfType(); diff --git a/ApplicationLibCode/Commands/SurfaceCommands/RicImportSurfacesFeature.cpp b/ApplicationLibCode/Commands/SurfaceCommands/RicImportSurfacesFeature.cpp index fae8651875..f0ce05dafc 100644 --- a/ApplicationLibCode/Commands/SurfaceCommands/RicImportSurfacesFeature.cpp +++ b/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() ); } } diff --git a/ApplicationLibCode/FileInterface/RifSurfio.cpp b/ApplicationLibCode/FileInterface/RifSurfio.cpp index ed7575dd0c..a9fd0bfabe 100644 --- a/ApplicationLibCode/FileInterface/RifSurfio.cpp +++ b/ApplicationLibCode/FileInterface/RifSurfio.cpp @@ -78,22 +78,27 @@ std::expected>, 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 ); } //-------------------------------------------------------------------------------------------------- diff --git a/ApplicationLibCode/ProjectDataModel/Surfaces/RimSurfaceCollection.cpp b/ApplicationLibCode/ProjectDataModel/Surfaces/RimSurfaceCollection.cpp index b6bbb3ac6f..fdee3654df 100644 --- a/ApplicationLibCode/ProjectDataModel/Surfaces/RimSurfaceCollection.cpp +++ b/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 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 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(); } //-------------------------------------------------------------------------------------------------- diff --git a/ApplicationLibCode/ProjectDataModel/Surfaces/RimSurfaceCollection.h b/ApplicationLibCode/ProjectDataModel/Surfaces/RimSurfaceCollection.h index fa718460f6..d3ea09deb8 100644 --- a/ApplicationLibCode/ProjectDataModel/Surfaces/RimSurfaceCollection.h +++ b/ApplicationLibCode/ProjectDataModel/Surfaces/RimSurfaceCollection.h @@ -19,6 +19,8 @@ #include "cafPdmNestedCollection.h" +#include + class RimSurface; class RimEnsembleSurface; class RimCase; @@ -41,7 +43,9 @@ public: void addEnsembleSurface( RimEnsembleSurface* ensembleSurface ); std::vector 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 importSurfacesFromFiles( const QStringList& fileNames, bool showLegend = true ); + RimSurface* addGridCaseSurface( RimCase* sourceCase, int oneBasedSliceIndex = 1 ); RimSurface* copySurfaces( std::vector surfaces ); RimSurface* addSurfacesAtIndex( int index, std::vector surfaces ); diff --git a/ApplicationLibCode/ProjectDataModelCommands/RimcSurfaceCollection.cpp b/ApplicationLibCode/ProjectDataModelCommands/RimcSurfaceCollection.cpp index e148be878a..18e47a54e4 100644 --- a/ApplicationLibCode/ProjectDataModelCommands/RimcSurfaceCollection.cpp +++ b/ApplicationLibCode/ProjectDataModelCommands/RimcSurfaceCollection.cpp @@ -63,7 +63,9 @@ RimcSurfaceCollection_importSurface::RimcSurfaceCollection_importSurface( caf::P std::expected RimcSurfaceCollection_importSurface::execute() { RimSurfaceCollection* coll = self(); - 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 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( 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 RimcSurfaceCollection_newSurface::execute() { RimSurfaceCollection* coll = self(); - 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; } //-------------------------------------------------------------------------------------------------- diff --git a/GrpcInterface/Python/rips/tests/test_surfaces.py b/GrpcInterface/Python/rips/tests/test_surfaces.py index 858bae04ee..32836f340b 100644 --- a/GrpcInterface/Python/rips/tests/test_surfaces.py +++ b/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" diff --git a/GrpcInterface/RiaGrpcPdmObjectService.cpp b/GrpcInterface/RiaGrpcPdmObjectService.cpp index d32257a100..853a5114bd 100644 --- a/GrpcInterface/RiaGrpcPdmObjectService.cpp +++ b/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() ); } }