diff --git a/ApplicationLibCode/UnitTests/CMakeLists.txt b/ApplicationLibCode/UnitTests/CMakeLists.txt index 7722e1f2d7..bc0e88b516 100644 --- a/ApplicationLibCode/UnitTests/CMakeLists.txt +++ b/ApplicationLibCode/UnitTests/CMakeLists.txt @@ -14,6 +14,7 @@ set(SOURCE_UNITTEST_FILES ${CMAKE_CURRENT_LIST_DIR}/RigBoundingBoxIjk-Test.cpp ${CMAKE_CURRENT_LIST_DIR}/RigContourMapGrid-Test.cpp ${CMAKE_CURRENT_LIST_DIR}/RifcCommandCore-Test.cpp + ${CMAKE_CURRENT_LIST_DIR}/RiaGrpcFieldSerialization-Test.cpp ${CMAKE_CURRENT_LIST_DIR}/RifEclipseInputFileTools-Test.cpp ${CMAKE_CURRENT_LIST_DIR}/RifEclipseOutputFileTools-Test.cpp ${CMAKE_CURRENT_LIST_DIR}/RifOpmFlowDeckFile-Test.cpp diff --git a/ApplicationLibCode/UnitTests/RiaGrpcFieldSerialization-Test.cpp b/ApplicationLibCode/UnitTests/RiaGrpcFieldSerialization-Test.cpp new file mode 100644 index 0000000000..4a4049434b --- /dev/null +++ b/ApplicationLibCode/UnitTests/RiaGrpcFieldSerialization-Test.cpp @@ -0,0 +1,83 @@ +#include "gtest/gtest.h" + +#include "cafPdmFieldScriptingCapability.h" +#include "cafPdmScriptIOMessages.h" + +#include +#include + +#include + +//-------------------------------------------------------------------------------------------------- +/// https://github.com/OPM/ResInsight/issues/14648 +/// +/// A string list sent from Python (via gRPC) is serialized as "[a, b, c]" and parsed with +/// stringsAreQuoted = false. Strings containing a comma are quoted by the Python client, and must +/// not be split into multiple items. If they are, set_discrete_property_category_names() fails with +/// "CategoryValues and CategoryNames must have matching sizes" +//-------------------------------------------------------------------------------------------------- +TEST( RiaGrpcFieldSerialization, StringListUnquotedWithCommaInString ) +{ + // This is the text produced by rips when sending ["Coal,Calcite", "Channel"]. Only strings + // containing separator characters are quoted. + QString source = R"(["Coal,Calcite", Channel])"; + + QTextStream stream( &source ); + std::vector destination; + + caf::PdmScriptIOMessages messages; + const bool stringsAreQuoted = false; + + caf::PdmFieldScriptingCapabilityIOHandler>::writeToField( destination, stream, &messages, stringsAreQuoted ); + + ASSERT_EQ( size_t( 2 ), destination.size() ); + EXPECT_STREQ( "Coal,Calcite", destination[0].toStdString().c_str() ); + EXPECT_STREQ( "Channel", destination[1].toStdString().c_str() ); +} + +//-------------------------------------------------------------------------------------------------- +/// https://github.com/OPM/ResInsight/issues/14648 +/// +/// Quoted strings containing a comma must be kept as one item +//-------------------------------------------------------------------------------------------------- +TEST( RiaGrpcFieldSerialization, StringListQuotedWithCommaInString ) +{ + QString source = R"(["Coal,Calcite", "Channel"])"; + + QTextStream stream( &source ); + std::vector destination; + + caf::PdmScriptIOMessages messages; + const bool stringsAreQuoted = true; + + caf::PdmFieldScriptingCapabilityIOHandler>::writeToField( destination, stream, &messages, stringsAreQuoted ); + + ASSERT_EQ( size_t( 2 ), destination.size() ); + EXPECT_STREQ( "Coal,Calcite", destination[0].toStdString().c_str() ); + EXPECT_STREQ( "Channel", destination[1].toStdString().c_str() ); +} + +//-------------------------------------------------------------------------------------------------- +/// https://github.com/OPM/ResInsight/issues/14648 +/// +/// Serialize and parse a string list containing commas +//-------------------------------------------------------------------------------------------------- +TEST( RiaGrpcFieldSerialization, StringListWithCommaRoundTrip ) +{ + const std::vector source = { "Coal,Calcite", "Channel" }; + + QString serialized; + QTextStream outputStream( &serialized ); + caf::PdmFieldScriptingCapabilityIOHandler>::readFromField( source, outputStream, true, false ); + + QTextStream inputStream( &serialized ); + std::vector destination; + caf::PdmScriptIOMessages messages; + const bool stringsAreQuoted = true; + + caf::PdmFieldScriptingCapabilityIOHandler>::writeToField( destination, inputStream, &messages, stringsAreQuoted ); + + ASSERT_EQ( source.size(), destination.size() ); + EXPECT_STREQ( "Coal,Calcite", destination[0].toStdString().c_str() ); + EXPECT_STREQ( "Channel", destination[1].toStdString().c_str() ); +} diff --git a/Fwk/AppFwk/cafPdmScripting/cafPdmFieldScriptingCapability.h b/Fwk/AppFwk/cafPdmScripting/cafPdmFieldScriptingCapability.h index 3d8eadd974..2c5f8a7f60 100644 --- a/Fwk/AppFwk/cafPdmScripting/cafPdmFieldScriptingCapability.h +++ b/Fwk/AppFwk/cafPdmScripting/cafPdmFieldScriptingCapability.h @@ -489,39 +489,80 @@ struct PdmFieldScriptingCapabilityIOHandler> QChar chr = errorMessageContainer->readCharWithLineNumberCount( inputStream ); if ( chr == QChar( '[' ) ) { + // Split on commas, but ignore commas inside quoted strings and inside nested + // brackets/parentheses. A quoted string can contain escaped characters. std::vector allValues; QString currentValue; + bool isInsideQuotes = false; + bool escapeNextChar = false; + int nestingDepth = 0; + while ( !inputStream.atEnd() ) { - errorMessageContainer->skipWhiteSpaceWithLineNumberCount( inputStream ); - QChar nextChar = errorMessageContainer->peekNextChar( inputStream ); - if ( nextChar == QChar( ']' ) ) + QChar currentChar = errorMessageContainer->readCharWithLineNumberCount( inputStream ); + + if ( escapeNextChar ) + { + currentValue += currentChar; + escapeNextChar = false; + } + else if ( isInsideQuotes ) + { + if ( currentChar == QChar( '\\' ) ) escapeNextChar = true; + if ( currentChar == QChar( '"' ) ) isInsideQuotes = false; + currentValue += currentChar; + } + else if ( currentChar == QChar( '"' ) ) + { + isInsideQuotes = true; + currentValue += currentChar; + } + else if ( currentChar == QChar( '[' ) || currentChar == QChar( '(' ) ) + { + nestingDepth++; + currentValue += currentChar; + } + else if ( currentChar == QChar( ']' ) && nestingDepth == 0 ) { - nextChar = errorMessageContainer->readCharWithLineNumberCount( inputStream ); break; } - else if ( nextChar == QChar( ',' ) ) + else if ( currentChar == QChar( ']' ) || currentChar == QChar( ')' ) ) { - nextChar = errorMessageContainer->readCharWithLineNumberCount( inputStream ); - errorMessageContainer->skipWhiteSpaceWithLineNumberCount( inputStream ); - if ( !currentValue.isEmpty() ) allValues.push_back( currentValue ); + nestingDepth--; + currentValue += currentChar; + } + else if ( currentChar == QChar( ',' ) && nestingDepth == 0 ) + { + QString trimmedValue = currentValue.trimmed(); + if ( !trimmedValue.isEmpty() ) allValues.push_back( trimmedValue ); currentValue = ""; } + else if ( currentChar.isSpace() && currentValue.isEmpty() ) + { + // Skip white space in front of a value + } else { - currentValue += errorMessageContainer->readCharWithLineNumberCount( inputStream ); + currentValue += currentChar; } } - if ( !currentValue.isEmpty() ) allValues.push_back( currentValue ); + + QString lastValue = currentValue.trimmed(); + if ( !lastValue.isEmpty() ) allValues.push_back( lastValue ); for ( QString textValue : allValues ) { + // A quoted item is always parsed as a quoted string, also when the surrounding text is + // unquoted. This makes it possible to transfer strings containing commas. + bool itemIsQuoted = stringsAreQuoted || ( textValue.size() > 1 && textValue.startsWith( QChar( '"' ) ) && + textValue.endsWith( QChar( '"' ) ) ); + QTextStream singleValueStream( &textValue, QIODevice::ReadOnly ); T singleValue; PdmFieldScriptingCapabilityIOHandler::writeToField( singleValue, singleValueStream, errorMessageContainer, - stringsAreQuoted, + itemIsQuoted, allowExtraCharacters ); fieldValue.push_back( singleValue ); } @@ -542,7 +583,7 @@ struct PdmFieldScriptingCapabilityIOHandler> outputStream << "["; for ( size_t i = 0; i < fieldValue.size(); ++i ) { - PdmFieldScriptingCapabilityIOHandler::readFromField( fieldValue[i], outputStream, quoteNonBuiltins ); + PdmFieldScriptingCapabilityIOHandler::readFromField( fieldValue[i], outputStream, quoteStrings, quoteNonBuiltins ); if ( i < fieldValue.size() - 1 ) { outputStream << ", "; diff --git a/Fwk/AppFwk/cafPdmScripting/cafPdmScripting_UnitTests/cafPdmFieldSerializationTest.cpp b/Fwk/AppFwk/cafPdmScripting/cafPdmScripting_UnitTests/cafPdmFieldSerializationTest.cpp index a60fef2dc7..2deca7610b 100644 --- a/Fwk/AppFwk/cafPdmScripting/cafPdmScripting_UnitTests/cafPdmFieldSerializationTest.cpp +++ b/Fwk/AppFwk/cafPdmScripting/cafPdmScripting_UnitTests/cafPdmFieldSerializationTest.cpp @@ -97,6 +97,54 @@ TEST( PdmFieldSerialization, StringListQuoted ) EXPECT_STREQ( "B-4H", destination[1].toStdString().c_str() ); } +//-------------------------------------------------------------------------------------------------- +// https://github.com/OPM/ResInsight/issues/14648 +// A quoted string item containing a comma must not be split into multiple items +//-------------------------------------------------------------------------------------------------- +TEST( PdmFieldSerialization, StringListWithCommaInsideString ) +{ + QString source = R"(["Coal,Calcite", "Channel"])"; + + QTextStream stream( &source ); + std::vector destination; + + caf::PdmScriptIOMessages messages; + bool stringsAreQuoted = true; + + caf::PdmFieldScriptingCapabilityIOHandler>::writeToField( destination, + stream, + &messages, + stringsAreQuoted ); + + ASSERT_EQ( (size_t)2, destination.size() ); + EXPECT_STREQ( "Coal,Calcite", destination[0].toStdString().c_str() ); + EXPECT_STREQ( "Channel", destination[1].toStdString().c_str() ); +} + +//-------------------------------------------------------------------------------------------------- +// https://github.com/OPM/ResInsight/issues/14648 +// Strings coming from Python are not quoted, except when they contain separator characters +//-------------------------------------------------------------------------------------------------- +TEST( PdmFieldSerialization, StringListPartiallyQuotedWithCommaInsideString ) +{ + QString source = R"(["Coal,Calcite", Channel])"; + + QTextStream stream( &source ); + std::vector destination; + + caf::PdmScriptIOMessages messages; + bool stringsAreQuoted = false; + + caf::PdmFieldScriptingCapabilityIOHandler>::writeToField( destination, + stream, + &messages, + stringsAreQuoted ); + + ASSERT_EQ( (size_t)2, destination.size() ); + EXPECT_STREQ( "Coal,Calcite", destination[0].toStdString().c_str() ); + EXPECT_STREQ( "Channel", destination[1].toStdString().c_str() ); +} + //-------------------------------------------------------------------------------------------------- //-------------------------------------------------------------------------------------------------- TEST( PdmFieldSerialization, StringListWithBackslashes ) diff --git a/GrpcInterface/Python/rips/category_mapping.py b/GrpcInterface/Python/rips/category_mapping.py index 189d510ceb..6520e94702 100644 --- a/GrpcInterface/Python/rips/category_mapping.py +++ b/GrpcInterface/Python/rips/category_mapping.py @@ -62,8 +62,7 @@ def set_discrete_property_category_names( Arguments: property_name (str): Name of the discrete property result. value_names (Dict[int, str]): Mapping from integer value to label. - Labels must not contain commas. An empty dict removes any - existing mapping for this property. + An empty dict removes any existing mapping for this property. value_colors (Optional[Dict[int, str]]): Optional per-value colors as strings accepted by QColor (e.g. "red", "#ff8800"). Values without a color entry get an auto-assigned palette color. diff --git a/GrpcInterface/Python/rips/pdmobject.py b/GrpcInterface/Python/rips/pdmobject.py index a8c2fd252c..0f605944d7 100644 --- a/GrpcInterface/Python/rips/pdmobject.py +++ b/GrpcInterface/Python/rips/pdmobject.py @@ -257,7 +257,7 @@ class PdmObjectBase: return self.__maketuple(value) return self.__unescape_string(value) - def __convert_to_grpc_value(self, value: Any) -> str: + def __convert_to_grpc_value(self, value: Any, quote_strings: bool = False) -> str: if isinstance(value, bool): if value: return "true" @@ -269,15 +269,29 @@ class PdmObjectBase: if isinstance(value, list): list_of_values = [] for val in value: - list_of_values.append(self.__convert_to_grpc_value(val)) + list_of_values.append( + self.__convert_to_grpc_value(val, quote_strings=True) + ) return "[" + ", ".join(list_of_values) + "]" if isinstance(value, tuple): list_of_values = [] for val in value: - list_of_values.append(self.__convert_to_grpc_value(val)) + list_of_values.append( + self.__convert_to_grpc_value(val, quote_strings=True) + ) return "(" + ", ".join(list_of_values) + ")" + if quote_strings and isinstance(value, str) and self.__requires_quoting(value): + # Quote and escape strings containing characters used as separators, to be able + # to transfer strings containing commas inside a list or tuple. + return '"' + self.__escape_string(value) + '"' return str(value) + def __requires_quoting(self, value: str) -> bool: + return "," in value or '"' in value + + def __escape_string(self, value: str) -> str: + return value.replace("\\", "\\\\").replace('"', '\\"') + def __get_grpc_value(self, camel_keyword: str) -> Value: return self.__convert_from_grpc_value( self._pb2_object.parameters[camel_keyword] diff --git a/GrpcInterface/Python/rips/tests/test_color_legend.py b/GrpcInterface/Python/rips/tests/test_color_legend.py index 758d4c8bf9..ced34616e4 100644 --- a/GrpcInterface/Python/rips/tests/test_color_legend.py +++ b/GrpcInterface/Python/rips/tests/test_color_legend.py @@ -103,6 +103,22 @@ def test_discrete_property_category_round_trip(rips_instance, initialize_test): assert case.discrete_property_category_colors("FACIES") == {} +def test_discrete_property_category_names_with_comma(rips_instance, initialize_test): + # https://github.com/OPM/ResInsight/issues/14648 + # A category name containing a comma must not be split into several names + case_path = dataroot.PATH + "/TEST10K_FLT_LGR_NNC/TEST10K_FLT_LGR_NNC.EGRID" + case = rips_instance.project.load_case(path=case_path) + assert case is not None + + expected_names = {0: "Coal,Calcite", 1: "Channel"} + + case.set_discrete_property_category_names( + property_name="EXAMPLE", value_names=expected_names + ) + + assert case.discrete_property_category_names("EXAMPLE") == expected_names + + def test_discrete_property_category_no_duplicate_legend(rips_instance, initialize_test): case_path = dataroot.PATH + "/TEST10K_FLT_LGR_NNC/TEST10K_FLT_LGR_NNC.EGRID" case = rips_instance.project.load_case(path=case_path)