diff --git a/ApplicationLibCode/UnitTests/CMakeLists.txt b/ApplicationLibCode/UnitTests/CMakeLists.txt index 69cd1c87c6..b699ad400d 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..37d40fb672 --- /dev/null +++ b/ApplicationLibCode/UnitTests/RiaGrpcFieldSerialization-Test.cpp @@ -0,0 +1,271 @@ +#include "gtest/gtest.h" + +#include "cafFilePath.h" +#include "cafPdmFieldScriptingCapability.h" +#include "cafPdmScriptIOMessages.h" + +#include "cafPdmFieldScriptingCapabilityCvfVec3d.h" + +#include +#include + +#include +#include + +namespace +{ +std::vector parseStringList( const QString& text, bool stringsAreQuoted ) +{ + QString source = text; + QTextStream stream( &source ); + std::vector destination; + caf::PdmScriptIOMessages messages; + + caf::PdmFieldScriptingCapabilityIOHandler>::writeToField( destination, stream, &messages, stringsAreQuoted ); + + return destination; +} +} // namespace + +//-------------------------------------------------------------------------------------------------- +/// 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. + auto values = parseStringList( R"(["Coal,Calcite", Channel])", false ); + + ASSERT_EQ( size_t( 2 ), values.size() ); + EXPECT_STREQ( "Coal,Calcite", values[0].toStdString().c_str() ); + EXPECT_STREQ( "Channel", values[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 ) +{ + auto values = parseStringList( R"(["Coal,Calcite", "Channel"])", true ); + + ASSERT_EQ( size_t( 2 ), values.size() ); + EXPECT_STREQ( "Coal,Calcite", values[0].toStdString().c_str() ); + EXPECT_STREQ( "Channel", values[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() ); +} + +//-------------------------------------------------------------------------------------------------- +/// Text values are allowed to contain parentheses, also unbalanced ones. These characters must not +/// be interpreted as nested containers for text values. +/// +/// NOTE: An unquoted text value can not contain the array end character ']', as this has always been +/// used to terminate the array. Text values containing brackets are quoted by the Python client. +//-------------------------------------------------------------------------------------------------- +TEST( RiaGrpcFieldSerialization, StringListWithBracketsAndParenthesesInString ) +{ + { + auto values = parseStringList( "[WELL-A (main), WELL-B (side), WELL-C]", false ); + + ASSERT_EQ( size_t( 3 ), values.size() ); + EXPECT_STREQ( "WELL-A (main)", values[0].toStdString().c_str() ); + EXPECT_STREQ( "WELL-B (side)", values[1].toStdString().c_str() ); + EXPECT_STREQ( "WELL-C", values[2].toStdString().c_str() ); + } + + { + // Unbalanced parentheses, both inside and in front of the value + auto values = parseStringList( "[(WELL-A, WELL-B(1, WELL-C]", false ); + + ASSERT_EQ( size_t( 3 ), values.size() ); + EXPECT_STREQ( "(WELL-A", values[0].toStdString().c_str() ); + EXPECT_STREQ( "WELL-B(1", values[1].toStdString().c_str() ); + EXPECT_STREQ( "WELL-C", values[2].toStdString().c_str() ); + } + + { + // Values containing brackets are quoted by the Python client + auto values = parseStringList( R"(["WELL-A [side]", WELL-B])", false ); + + ASSERT_EQ( size_t( 2 ), values.size() ); + EXPECT_STREQ( "WELL-A [side]", values[0].toStdString().c_str() ); + EXPECT_STREQ( "WELL-B", values[1].toStdString().c_str() ); + } +} + +//-------------------------------------------------------------------------------------------------- +/// A quote is only given special meaning when it is the first character of a value. Text values +/// containing a quote are quoted and escaped by the Python client. +//-------------------------------------------------------------------------------------------------- +TEST( RiaGrpcFieldSerialization, StringListWithQuoteInsideString ) +{ + auto values = parseStringList( R"(["12\" pipe", "8\" pipe"])", false ); + + ASSERT_EQ( size_t( 2 ), values.size() ); + EXPECT_STREQ( "12\" pipe", values[0].toStdString().c_str() ); + EXPECT_STREQ( "8\" pipe", values[1].toStdString().c_str() ); +} + +//-------------------------------------------------------------------------------------------------- +/// White space inside a text value must be preserved +//-------------------------------------------------------------------------------------------------- +TEST( RiaGrpcFieldSerialization, StringListWithWhiteSpaceInString ) +{ + auto values = parseStringList( "[Well A, Well B ]", false ); + + ASSERT_EQ( size_t( 2 ), values.size() ); + EXPECT_STREQ( "Well A", values[0].toStdString().c_str() ); + EXPECT_STREQ( "Well B", values[1].toStdString().c_str() ); + + // Leading and trailing white space is preserved for quoted values + auto quotedValues = parseStringList( R"([" Well A ", Well B])", false ); + + ASSERT_EQ( size_t( 2 ), quotedValues.size() ); + EXPECT_STREQ( " Well A ", quotedValues[0].toStdString().c_str() ); + EXPECT_STREQ( "Well B", quotedValues[1].toStdString().c_str() ); +} + +//-------------------------------------------------------------------------------------------------- +/// Empty lists and lists with empty strings +//-------------------------------------------------------------------------------------------------- +TEST( RiaGrpcFieldSerialization, StringListEmpty ) +{ + EXPECT_EQ( size_t( 0 ), parseStringList( "[]", false ).size() ); + EXPECT_EQ( size_t( 0 ), parseStringList( "[ ]", false ).size() ); + EXPECT_EQ( size_t( 1 ), parseStringList( R"([""])", false ).size() ); +} + +//-------------------------------------------------------------------------------------------------- +/// File paths are text values, and must be transferred without modification +//-------------------------------------------------------------------------------------------------- +TEST( RiaGrpcFieldSerialization, FilePathList ) +{ + QString source = R"([C:\Users\file (1).txt, /tmp/my data/case.EGRID])"; + QTextStream stream( &source ); + std::vector destination; + caf::PdmScriptIOMessages messages; + + caf::PdmFieldScriptingCapabilityIOHandler>::writeToField( destination, stream, &messages, false ); + + ASSERT_EQ( size_t( 2 ), destination.size() ); + EXPECT_STREQ( "C:\\Users\\file (1).txt", destination[0].path().toStdString().c_str() ); + EXPECT_STREQ( "/tmp/my data/case.EGRID", destination[1].path().toStdString().c_str() ); +} + +//-------------------------------------------------------------------------------------------------- +/// Numeric lists are unaffected +//-------------------------------------------------------------------------------------------------- +TEST( RiaGrpcFieldSerialization, NumberLists ) +{ + { + QString source = "[1, 2, 3]"; + QTextStream stream( &source ); + std::vector destination; + caf::PdmScriptIOMessages messages; + + caf::PdmFieldScriptingCapabilityIOHandler>::writeToField( destination, stream, &messages, false ); + + ASSERT_EQ( size_t( 3 ), destination.size() ); + EXPECT_EQ( 3, destination[2] ); + } + + { + QString source = "[1.5,2.5 , -3.5]"; + QTextStream stream( &source ); + std::vector destination; + caf::PdmScriptIOMessages messages; + + caf::PdmFieldScriptingCapabilityIOHandler>::writeToField( destination, stream, &messages, false ); + + ASSERT_EQ( size_t( 3 ), destination.size() ); + EXPECT_DOUBLE_EQ( 1.5, destination[0] ); + EXPECT_DOUBLE_EQ( 2.5, destination[1] ); + EXPECT_DOUBLE_EQ( -3.5, destination[2] ); + } +} + +//-------------------------------------------------------------------------------------------------- +/// Nested containers must be parsed as one item each +//-------------------------------------------------------------------------------------------------- +TEST( RiaGrpcFieldSerialization, NestedContainerLists ) +{ + { + QString source = "[[1, 2], [3, 4]]"; + QTextStream stream( &source ); + std::vector> destination; + caf::PdmScriptIOMessages messages; + + caf::PdmFieldScriptingCapabilityIOHandler>>::writeToField( destination, stream, &messages, false ); + + ASSERT_EQ( size_t( 2 ), destination.size() ); + ASSERT_EQ( size_t( 2 ), destination[0].size() ); + EXPECT_DOUBLE_EQ( 2.0, destination[0][1] ); + EXPECT_DOUBLE_EQ( 3.0, destination[1][0] ); + } + + { + QString source = "[(true, 1.0), (false, 2.0)]"; + QTextStream stream( &source ); + std::vector> destination; + caf::PdmScriptIOMessages messages; + + caf::PdmFieldScriptingCapabilityIOHandler>>::writeToField( destination, stream, &messages, false ); + + ASSERT_EQ( size_t( 2 ), destination.size() ); + EXPECT_TRUE( destination[0].first ); + EXPECT_DOUBLE_EQ( 1.0, destination[0].second ); + EXPECT_FALSE( destination[1].first ); + EXPECT_DOUBLE_EQ( 2.0, destination[1].second ); + } +} + +//-------------------------------------------------------------------------------------------------- +/// A list of 3D vectors is parsed by a dedicated handler that reads the inner arrays from the same +/// stream. Verify that the stream position handling is intact. +//-------------------------------------------------------------------------------------------------- +TEST( RiaGrpcFieldSerialization, Vec3dList ) +{ + QString source = "[[1, 2, 3], [4, 5, 6]]"; + QTextStream stream( &source ); + std::vector> destination; + caf::PdmScriptIOMessages messages; + + caf::PdmFieldScriptingCapabilityIOHandler>>::writeToField( destination, stream, &messages, false ); + + ASSERT_EQ( size_t( 2 ), destination.size() ); + EXPECT_DOUBLE_EQ( 1.0, destination[0].x() ); + EXPECT_DOUBLE_EQ( 3.0, destination[0].z() ); + EXPECT_DOUBLE_EQ( 4.0, destination[1].x() ); + EXPECT_DOUBLE_EQ( 6.0, destination[1].z() ); + EXPECT_TRUE( messages.m_messages.empty() ); +} diff --git a/Fwk/AppFwk/cafPdmScripting/cafPdmFieldScriptingCapability.h b/Fwk/AppFwk/cafPdmScripting/cafPdmFieldScriptingCapability.h index 3d8eadd974..38fed7b433 100644 --- a/Fwk/AppFwk/cafPdmScripting/cafPdmFieldScriptingCapability.h +++ b/Fwk/AppFwk/cafPdmScripting/cafPdmFieldScriptingCapability.h @@ -201,6 +201,8 @@ constexpr bool isCamelCase( std::string_view str ) namespace caf { +class FilePath; + template struct PdmFieldScriptingCapabilityIOHandler { @@ -489,39 +491,90 @@ struct PdmFieldScriptingCapabilityIOHandler> QChar chr = errorMessageContainer->readCharWithLineNumberCount( inputStream ); if ( chr == QChar( '[' ) ) { + // Split on commas, but ignore commas inside quoted strings and inside nested brackets or + // parentheses. A quote or an opening bracket is only given special meaning when it is the + // first character of an item, as text values are allowed to contain any character. + // + // Nested containers are not possible for text based values, and brackets and parentheses are + // treated as ordinary characters for these types. + const bool nestedContainersAreSupported = !std::is_same::value && !std::is_same::value; + 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 ); + + const bool isStartOfValue = currentValue.isEmpty(); + + if ( escapeNextChar ) { - nextChar = errorMessageContainer->readCharWithLineNumberCount( inputStream ); + currentValue += currentChar; + escapeNextChar = false; + } + else if ( isInsideQuotes ) + { + if ( currentChar == QChar( '\\' ) ) escapeNextChar = true; + if ( currentChar == QChar( '"' ) ) isInsideQuotes = false; + currentValue += currentChar; + } + else if ( currentChar == QChar( '"' ) && isStartOfValue ) + { + isInsideQuotes = true; + currentValue += currentChar; + } + else if ( ( currentChar == QChar( '[' ) || currentChar == QChar( '(' ) ) && + nestedContainersAreSupported && ( isStartOfValue || nestingDepth > 0 ) ) + { + nestingDepth++; + currentValue += currentChar; + } + else if ( nestingDepth > 0 && ( currentChar == QChar( ']' ) || currentChar == QChar( ')' ) ) ) + { + nestingDepth--; + currentValue += currentChar; + } + else if ( currentChar == QChar( ']' ) ) + { + // End of the array break; } - else if ( nextChar == QChar( ',' ) ) + else if ( currentChar == QChar( ',' ) && nestingDepth == 0 ) { - nextChar = errorMessageContainer->readCharWithLineNumberCount( inputStream ); - errorMessageContainer->skipWhiteSpaceWithLineNumberCount( inputStream ); - if ( !currentValue.isEmpty() ) allValues.push_back( currentValue ); + QString trimmedValue = currentValue.trimmed(); + if ( !trimmedValue.isEmpty() ) allValues.push_back( trimmedValue ); currentValue = ""; } + else if ( currentChar.isSpace() && isStartOfValue ) + { + // 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 +595,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 49f74b432a..02db02302c 100644 --- a/GrpcInterface/Python/rips/pdmobject.py +++ b/GrpcInterface/Python/rips/pdmobject.py @@ -239,7 +239,7 @@ def __convert_from_grpc_value(self, value: str) -> Value: 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" @@ -251,15 +251,31 @@ def __convert_to_grpc_value(self, value: Any) -> str: 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: + # Tuple items are not quoted, as the tuple parser on the ResInsight side does not + # support quoted strings. list_of_values.append(self.__convert_to_grpc_value(val)) 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: + # Characters used as separators by the text based parser on the ResInsight side, and + # leading/trailing white space, which would otherwise be stripped. + return any(ch in value for ch in ',"[]') or value != value.strip() + + 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)