Merged
Conversation
bkeryan
commented
Aug 6, 2025
bkeryan
commented
Aug 6, 2025
Collaborator
FYI, the |
bkeryan
commented
Aug 6, 2025
jasonmreding
reviewed
Aug 6, 2025
Merged
…8 depending on feature toggle
44778c6 to
202ebe3
Compare
bkeryan
commented
Aug 8, 2025
bkeryan
commented
Aug 8, 2025
bkeryan
commented
Aug 8, 2025
bkeryan
commented
Aug 8, 2025
jasonmreding
approved these changes
Aug 8, 2025
Collaborator
Author
|
FYI @jasonmreding Some last-minute changes compared to what you reviewed:
|
jasonmreding
approved these changes
Aug 11, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this Pull Request accomplish?
Split string/bytes handling:
data_utf8Stringsfeature toggle.SetLVBytesandGetLVBytes. These do not do any string conversion/verification.SetLVStringandGetLVStringto convert to/from UTF-8 usingConvertSystemStringToUTF8andConvertUTF8StringToSystem.SetLVBytesandSetLVStringto pass the string as astd::string_view.SetLVRTModulePathto passstd::stringby const reference.LVBytesMessageValueandLVRepeatedBytesMessageValue, which use bytes-specific functionality likeWriteBytes,BytesSize,WireFormatLite::TYPE_BYTES, etc.ParseBytesmethods to use the new classes.LVMessageMetadataType::BytesValuecases to use bytes-specific functionality.Verify string encoding:
data_verifyStringEncodingfeature toggle.<string_utils.h>containingVerifyAsciiStringandVerifyUtf8Stringfunctions.WireFormatLite::VerifyUtf8Stringfunction.data_verifyStringEncodingfeature toggle is disabled, these functions always succeed.SetLVAsciiStringandGetLVAsciiString. These callVerifyAsciiStringand throw an exception if it returns false.any_support.ccandproto_parser.ccto callSetLVAsciiString,GetLVAsciiString, orVerifyAsciiStringParseStringmethods to callVerifyUtf8Stringand throwstd::runtime_errorif it fails. This logs a message like:Serializemethods to callVerifyUtf8String. This logs but does not return an error.Add four test cases:
Test_HelloWorld_Internationalization.vitests the HelloWorld client+server with internationalized strings. Unfortunately, this test still passes without the fix because both the client and server use the same incorrect string encoding.Test_HelloWorld_Utf8Encoding.vipacks ahelloworld.HelloRequestto Any, searches for the UTF-8 encoded string, and unpacks it and compares the result. This test fails without the fix.Test_HelloWorld_InvalidAsciiEncoding.vipacks an Any using a non-ASCII message name and verifies that it fails.-Test_HelloWorld_InvalidUtf8Encoding.vipacks an Any, replaces an ASCII sequence with Latin-1 (which is invalid UTF-8) and verifies that unpacking it fails.Why should this Pull Request be merged?
Closes #461
What testing has been done?
Default feature toggles:
tests/run_tests.pywith LV 2019 x86 and release DLL (including new tests).tests/New_ATS/pylib/run_tests.pywith LV 2019 x86 and release DLL.é) in the service display namegrpcurl -plaintext localhost:50051 listgrpcurl -plaintext localhost:50051 describe helloworld.Greetergrpcurl -plaintext -d '{\"name\": \"Brad\"}' localhost:50051 helloworld.Greeter.SayHellodata_verifyStringEncoding= false:tests/run_tests.pywith LV 2019 x86 and release DLL.Test_HelloWorld_InvalidUtf8Encoding.vifails.Test_HelloWorld_InvalidAsciiEncoding.vistill passes because the invalid ASCII name is not found in the map.data_utf8Strings= false:tests/run_tests.pywith LV 2019 x86 and release DLL.Test_HelloWorld_InvalidUtf8Encoding.vifails.Test_HelloWorld_Utf8Encoding.vifails.data_utf8Strings= false anddata_verifyStringEncoding= false:tests/New_ATS/pylib/run_tests.pywith LV 2019 x86 and release DLL.