Skip to content

Commit 068f31c

Browse files
bkeryanbkeryan
authored
fix: Encode strings as UTF-8, take 2 (#471)
* lv_interop: Pass std::string by const ref * feature_toggles: Add data_utf8Strings * lv_interop: Add Get/SetLVBytes and update Get/SetLVString to use UTF-8 depending on feature toggle * Split string/bytes handling * tests: Add internationalization tests * Use Get/SetLVBytes for serialized descriptors * Update UTF-8 string code to use new feature toggle accessor * Add verifyStringEncoding feature toggle * Add VerifyAscii/Utf8String * Add Get/SetLVAsciiString and verifyStringEncoding feature toggle * tests: Add empty string test case to Test_HelloWorld_Utf8Encoding.vi * tests: Add Test_HelloWorld_Invalid(Ascii|Utf8)Encoding.vi * docs: Update feature toggle docs * lv_message: Use a C++ exception to report invalid UTF-8 data * tests: Change expected error for InvalidUtf8Encoding and remove IsDebugDLL check
1 parent 398962b commit 068f31c

25 files changed

+521
-47
lines changed

‎CMakeLists.txt‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,7 @@ add_library(labview_grpc_server SHARED
9898
src/message_element_metadata_owner.cc
9999
src/message_metadata.cc
100100
src/path_support.cc
101+
src/string_utils.cc
101102
src/unpacked_fields.cc
102103
src/well_known_messages.cc
103104
)
@@ -116,6 +117,7 @@ add_library(labview_grpc_generator SHARED
116117
src/lv_interop.cc
117118
src/path_support.cc
118119
src/proto_parser.cc
120+
src/string_utils.cc
119121
)
120122
target_link_libraries(labview_grpc_generator
121123
${_REFLECTION}

‎docs/FeatureToggles.md‎

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,10 @@ In the example above, the `EfficientMessageCopy` and `useOccurrence` features ar
1616

1717
### More about the flags
1818

19-
1. `EfficientMessageCopy` - This feature is used to enable or disable the efficient message copy feature. When enabled, the client will use efficient message copy to have throughput. When disabled, the client will use the default message copy.
19+
- `EfficientMessageCopy` - This feature is used to enable or disable the efficient message copy feature. When enabled, the client will use efficient message copy to have throughput. When disabled, the client will use the default message copy.
2020

21-
2. `useOccurrence` - This feature is used to enable or disable the occurrence feature. When enabled, the client will use occurrence to manage synchroniation between LabVIEW execution threads. When disabled, the client will use not use LabVIEW occurrences.
21+
- `useOccurrence` - This feature is used to enable or disable the occurrence feature. When enabled, the client will use occurrence to manage synchroniation between LabVIEW execution threads. When disabled, the client will use not use LabVIEW occurrences.
22+
23+
- `utf8Strings` - This feature is used to enable or disable UTF-8 string support. When enabled, the client will treat strings in protobuf messages as UTF-8, as documented in https://protobuf.dev/programming-guides/encoding/. When disabled, the client will treat strings in protobuf messages as LabVIEW's native string encoding. This feature is enabled by default.
24+
25+
- `verifyStringEncoding` - This feature is used to enable or disable verification of string encoding. When enabled, functions that take protobuf message/field names will error if the names are not ASCII, parsing gRPC string fields will error if they are not valid UTF-8, and writing gRPC string fields will log if they are not valid UTF-8. This feature is enabled by default.

‎src/any_support.cc‎

Lines changed: 34 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
#include <google/protobuf/any.pb.h>
66
#include <serialization_session.h>
77
#include <lv_message.h>
8+
#include <string_utils.h>
89

910
//---------------------------------------------------------------------
1011
//---------------------------------------------------------------------
@@ -45,6 +46,11 @@ LIBRARY_EXPORT int32_t PackToBuffer(grpc_labview::gRPCid* id, const char* messag
4546
return -1;
4647
}
4748

49+
if (!grpc_labview::VerifyAsciiString(messageType))
50+
{
51+
return -2;
52+
}
53+
4854
auto metadata = metadataOwner->FindMetadata(messageType);
4955
if (metadata == nullptr)
5056
{
@@ -80,13 +86,18 @@ LIBRARY_EXPORT int32_t PackToAny(grpc_labview::gRPCid* id, const char* messageTy
8086
return -1;
8187
}
8288

89+
if (!grpc_labview::VerifyAsciiString(messageType))
90+
{
91+
return -2;
92+
}
93+
8394
auto metadata = metadataOwner->FindMetadata(messageType);
8495
if (metadata == nullptr)
8596
{
8697
return -2;
8798
}
8899

89-
SetLVString(&anyCluster->TypeUrl, "/" + metadata->typeUrl);
100+
SetLVAsciiString(&anyCluster->TypeUrl, "/" + metadata->typeUrl);
90101
return PackToBuffer(id, messageType, cluster, &anyCluster->Bytes);
91102
} catch (const std::exception&) {
92103
return grpc_labview::TranslateException();
@@ -104,6 +115,11 @@ LIBRARY_EXPORT int32_t UnpackFromBuffer(grpc_labview::gRPCid* id, grpc_labview::
104115
return -1;
105116
}
106117

118+
if (!grpc_labview::VerifyAsciiString(messageType))
119+
{
120+
return -2;
121+
}
122+
107123
auto metadata = metadataOwner->FindMetadata(messageType);
108124
if (metadata == nullptr)
109125
{
@@ -146,13 +162,18 @@ LIBRARY_EXPORT int32_t TryUnpackFromAny(grpc_labview::gRPCid* id, grpc_labview::
146162
return -1;
147163
}
148164

165+
if (!grpc_labview::VerifyAsciiString(messageType))
166+
{
167+
return -2;
168+
}
169+
149170
auto metadata = metadataOwner->FindMetadata(messageType);
150171
if (metadata == nullptr)
151172
{
152173
return -2;
153174
}
154175

155-
if (grpc_labview::GetLVString(anyCluster->TypeUrl) != messageType)
176+
if (grpc_labview::GetLVAsciiString(anyCluster->TypeUrl) != messageType)
156177
{
157178
return -1;
158179
}
@@ -173,13 +194,18 @@ LIBRARY_EXPORT int32_t IsAnyOfType(grpc_labview::gRPCid* id, grpc_labview::AnyCl
173194
return -1;
174195
}
175196

197+
if (!grpc_labview::VerifyAsciiString(messageType))
198+
{
199+
return -2;
200+
}
201+
176202
auto metadata = metadataOwner->FindMetadata(messageType);
177203
if (metadata == nullptr)
178204
{
179205
return -2;
180206
}
181207

182-
if (grpc_labview::GetLVString(anyCluster->TypeUrl) != metadata->typeUrl)
208+
if (grpc_labview::GetLVAsciiString(anyCluster->TypeUrl) != metadata->typeUrl)
183209
{
184210
return -1;
185211
}
@@ -300,6 +326,8 @@ LIBRARY_EXPORT int32_t AnyBuilderBuildToBuffer(grpc_labview::gRPCid* builderId,
300326
return -1;
301327
}
302328

329+
// The typeUrl parameter is unused.
330+
303331
grpc_labview::gPointerManager.UnregisterPointer(builderId);
304332
std::string buffer;
305333
if (message->SerializeToString(&buffer))
@@ -327,7 +355,9 @@ LIBRARY_EXPORT int32_t AnyBuilderBuild(grpc_labview::gRPCid* builderId, const ch
327355
return -1;
328356
}
329357

330-
grpc_labview::SetLVString(&anyCluster->TypeUrl, message->_metadata->typeUrl);
358+
// The typeUrl parameter is unused.
359+
360+
grpc_labview::SetLVAsciiString(&anyCluster->TypeUrl, message->_metadata->typeUrl);
331361
return AnyBuilderBuildToBuffer(builderId, typeUrl, &anyCluster->Bytes);
332362
} catch (const std::exception&) {
333363
return grpc_labview::TranslateException();

‎src/cluster_copier.cc‎

Lines changed: 57 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
#include <cluster_copier.h>
44
#include <well_known_messages.h>
55
#include <lv_message.h>
6+
#include <feature_toggles.h>
67

78
namespace grpc_labview {
89

@@ -283,7 +284,33 @@ namespace grpc_labview {
283284
//---------------------------------------------------------------------
284285
void ClusterDataCopier::CopyBytesToCluster(const std::shared_ptr<const MessageElementMetadata> metadata, int8_t* start, const std::shared_ptr<const LVMessageValue>& value)
285286
{
286-
CopyStringToCluster(metadata, start, value);
287+
if (!FeatureConfig::getInstance().AreUtf8StringsEnabled()) {
288+
CopyStringToCluster(metadata, start, value);
289+
return;
290+
}
291+
292+
if (metadata->isRepeated)
293+
{
294+
auto repeatedBytes = static_cast<const LVRepeatedBytesMessageValue&>(*value);
295+
if (repeatedBytes._value.size() != 0)
296+
{
297+
NumericArrayResize(GetTypeCodeForSize(sizeof(LStrHandle)), 1, start, repeatedBytes._value.size());
298+
auto array = *(LV1DArrayHandle*)start;
299+
(*array)->cnt = repeatedBytes._value.size();
300+
int x = 0;
301+
auto lvBytes = (*array)->bytes<LStrHandle>();
302+
for (auto& bytes : repeatedBytes._value)
303+
{
304+
*lvBytes = nullptr;
305+
SetLVBytes(lvBytes, bytes);
306+
lvBytes += 1;
307+
}
308+
}
309+
}
310+
else
311+
{
312+
SetLVBytes((LStrHandle*)start, ((LVBytesMessageValue*)value.get())->_value);
313+
}
287314
}
288315

289316
//---------------------------------------------------------------------
@@ -696,7 +723,35 @@ namespace grpc_labview {
696723
//---------------------------------------------------------------------
697724
void ClusterDataCopier::CopyBytesFromCluster(const std::shared_ptr<const MessageElementMetadata> metadata, int8_t* start, LVMessage& message)
698725
{
699-
CopyStringFromCluster(metadata, start, message);
726+
if (!FeatureConfig::getInstance().AreUtf8StringsEnabled()) {
727+
CopyStringFromCluster(metadata, start, message);
728+
return;
729+
}
730+
731+
if (metadata->isRepeated)
732+
{
733+
auto array = *(LV1DArrayHandle*)start;
734+
auto arraySize = (array && *array) ? (*array)->cnt : 0;
735+
if (arraySize != 0)
736+
{
737+
auto repeatedBytesValue = std::make_shared<LVRepeatedBytesMessageValue>(metadata->protobufIndex);
738+
message._values.emplace(metadata->protobufIndex, repeatedBytesValue);
739+
auto lvBytes = (*array)->bytes<LStrHandle>();
740+
repeatedBytesValue->_value.Reserve(arraySize);
741+
for (int x = 0; x < arraySize; ++x)
742+
{
743+
auto bytes = GetLVBytes(*lvBytes);
744+
repeatedBytesValue->_value.Add(std::move(bytes));
745+
lvBytes += 1;
746+
}
747+
}
748+
}
749+
else
750+
{
751+
auto bytes = GetLVBytes(*(LStrHandle*)start);
752+
auto bytesValue = std::make_shared<LVBytesMessageValue>(metadata->protobufIndex, bytes);
753+
message._values.emplace(metadata->protobufIndex, bytesValue);
754+
}
700755
}
701756

702757
//---------------------------------------------------------------------

‎src/feature_toggles.cc‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,8 @@ namespace grpc_labview {
3232

3333
efficientMessageCopy = IsFeatureEnabled(kFeatureEfficientMessageCopy);
3434
useOccurrence = IsFeatureEnabled(kFeatureUseOccurrence);
35+
utf8Strings = IsFeatureEnabled(kFeatureUtf8Strings);
36+
verifyStringEncoding = IsFeatureEnabled(kFeatureVerifyStringEncoding);
3537
}
3638

3739
// Function to check if a feature is enabled

‎src/feature_toggles.h‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ namespace grpc_labview {
77
static constexpr const char* kFeatureFileFound = "featureFileFound";
88
static constexpr const char* kFeatureEfficientMessageCopy = "data_EfficientMessageCopy";
99
static constexpr const char* kFeatureUseOccurrence = "data_useOccurrence";
10+
static constexpr const char* kFeatureUtf8Strings = "data_utf8Strings";
11+
static constexpr const char* kFeatureVerifyStringEncoding = "data_verifyStringEncoding";
1012

1113
class FeatureConfig {
1214
public:
@@ -25,13 +27,17 @@ namespace grpc_labview {
2527
// Functions to check specific feature toggles
2628
bool IsEfficientMessageCopyEnabled() const { return efficientMessageCopy; }
2729
bool IsUseOccurrenceEnabled() const { return useOccurrence; }
30+
bool AreUtf8StringsEnabled() const { return utf8Strings; }
31+
bool IsVerifyStringEncodingEnabled() const { return verifyStringEncoding; }
2832

2933
private:
3034
std::unordered_map<std::string, bool> featureFlags;
3135
const std::string defaultConfigFileName = "feature_config.ini";
3236

3337
bool efficientMessageCopy;
3438
bool useOccurrence;
39+
bool utf8Strings;
40+
bool verifyStringEncoding;
3541

3642
// This stores the default feature configuration.
3743
// During ReloadFeaturesFromFile(), this configuration is always applied first prior to reading the
@@ -40,11 +46,15 @@ namespace grpc_labview {
4046
featureFlags[kFeatureFileFound] = false; // Used to indicate if the feature file was found/used during initialization
4147
featureFlags[kFeatureEfficientMessageCopy] = false;
4248
featureFlags[kFeatureUseOccurrence] = true;
49+
featureFlags[kFeatureUtf8Strings] = true;
50+
featureFlags[kFeatureVerifyStringEncoding] = true;
4351
}
4452

4553
FeatureConfig() :
4654
efficientMessageCopy(false),
47-
useOccurrence(false)
55+
useOccurrence(false),
56+
utf8Strings(false),
57+
verifyStringEncoding(false)
4858
{
4959
// Read the default configuration file during initialization
5060
ReloadFeaturesFromFile("");

‎src/grpc_interop.cc‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ namespace grpc_labview
2929
eventData.methodData = data;
3030
eventData.methodName = nullptr;
3131

32-
SetLVString(&eventData.methodName, eventMethodName);
32+
SetLVAsciiString(&eventData.methodName, eventMethodName);
3333

3434
auto error = PostUserEvent(event, &eventData);
3535

@@ -112,9 +112,9 @@ namespace grpc_labview
112112
{
113113
std::shared_ptr<EnumMetadata> enumMetadata(new EnumMetadata());
114114

115-
enumMetadata->messageName = GetLVString(lvMetadata->messageName);
116-
enumMetadata->typeUrl = GetLVString(lvMetadata->typeUrl);
117-
enumMetadata->elements = GetLVString(lvMetadata->elements);
115+
enumMetadata->messageName = GetLVAsciiString(lvMetadata->messageName);
116+
enumMetadata->typeUrl = GetLVAsciiString(lvMetadata->typeUrl);
117+
enumMetadata->elements = GetLVAsciiString(lvMetadata->elements);
118118
enumMetadata->allowAlias = lvMetadata->allowAlias;
119119

120120
// Create the map between LV enum and proto enum values

0 commit comments

Comments
 (0)