Automated rollback of changelist with RevId 358064676

PiperOrigin-RevId: 359571540
google · May 5, 2021 · 443d17c · 443d17c
1 parent 936aff7
commit 443d17c
Show file tree

Hide file tree

Showing 10 changed files with 42 additions and 49 deletions.
diff --git a/cc/google/fhir/json_util.cc b/cc/google/fhir/json_util.cc
@@ -48,8 +48,9 @@ absl::StatusOr<std::string> ToJsonStringValue(absl::string_view raw_value) {
  if (replacement != CHARACTERS_TO_ESCAPED_CHARACTER_MAP->end()) {
  absl::StrAppend(&result, replacement->second);
  } else if (char_byte < 32) {
- absl::StrAppend(&result, "\\u",
- absl::Hex(static_cast<int>(char_byte), absl::kZeroPad4));
+ return absl::InvalidArgumentError(absl::StrCat(
+ "Invalid control character found in string. Decimal value:",
+ static_cast<int>(char_byte)));
  } else {
  result.push_back(char_byte);
  }

diff --git a/cc/google/fhir/json_util.h b/cc/google/fhir/json_util.h
@@ -33,13 +33,10 @@ namespace google::fhir {
 // this would return the literal:
 // R"("this is \n\"my favorite\" string")"
 //
-// Note that this will print all control characters with their escape
-// counterparts. While the FHIR spec states that "Strings SHOULD not contain
-// Unicode character points below 32, except for u0009 (horizontal tab), u0010
-// (carriage return) and u0013 (line feed)", we take a permissive stance in what
-// it allows, and will not reject or otherwise surpress other control
-// characters.
-// See: https://www.hl7.org/fhir/datatypes.html#string
+// Per FHIR spec, the only valid control characters in the range [0,32)
+// are [\r\n\t]. This returns a status error if it encounters any other
+// characters in that range.
+// See https://www.hl7.org/fhir/datatypes.html#string
 //
 // Note that this does NOT escape the "solidus" `/` as `\/`.
 // Per JSON spec, this optional but not required, so to maintain a minimal

diff --git a/cc/google/fhir/json_util_test.cc b/cc/google/fhir/json_util_test.cc
@@ -48,14 +48,10 @@ TEST(JsonUtilTest, ToJsonStringValueWithMultiByteCharacters) {
  TestToJsonStringValue("André", "\"André\"");
 }
 
-TEST(JsonUtilTest, ToJsonStringValueControlCharactersPrinted) {
- std::string with_null_char{"\r\n\t\0\u0008", 5};
- TestToJsonStringValue(with_null_char, R"("\r\n\t\u0000\u0008")");
-}
-
-TEST(JsonUtilTest, ToJsonStringValueOnlyNullChar) {
- std::string only_null_char{"\0", 1};
- TestToJsonStringValue(only_null_char, R"("\u0000")");
+TEST(JsonUtilTest, ToJsonStringValueInvalidControlCharactersReturnsError) {
+ std::string with_null_char{"foo\0bar", 7};
+ absl::StatusOr<std::string> result = ToJsonStringValue(with_null_char);
+ EXPECT_FALSE(result.ok());
 }
 
 } // namespace

diff --git a/cc/google/fhir/r4/json_format_test.cc b/cc/google/fhir/r4/json_format_test.cc
@@ -1688,10 +1688,11 @@ TEST(JsonFormatR4Test, PrintAndParseAllResources) {
  }
 }
 
-TEST(JsonFormatR4Test, ControlCharacters) {
- TestPairWithFilePaths<Observation>(
- "testdata/jsonformat/observation_control_characters.prototxt",
- "testdata/jsonformat/observation_control_characters.json");
+TEST(JsonFormatR4Test, InvalidControlCharactersReturnsError) {
+ const Observation proto = ReadProto<Observation>(
+ "testdata/jsonformat/observation_invalid_unicode.prototxt");
+
+ ASSERT_FALSE(PrettyPrintFhirToJsonString(proto).ok());
 }
 
 TEST(JsonFormatR4Test, PadsThreeDigitYearToFourCharacters) {

diff --git a/cc/google/fhir/stu3/json_format_test.cc b/cc/google/fhir/stu3/json_format_test.cc
@@ -2199,10 +2199,11 @@ TEST(JsonFormatTest, VisionPrescriptionPrint) {
  TestPrint<VisionPrescription>("VisionPrescription-33124");
 }
 
-TEST(JsonFormatR4Test, ControlCharacters) {
- TestPairWithFilePaths<Observation>(
- "testdata/jsonformat/observation_control_characters.prototxt",
- "testdata/jsonformat/observation_control_characters.json");
+TEST(JsonFormatTest, InvalidControlCharactersReturnsError) {
+ const Observation proto = ReadProto<Observation>(
+ "testdata/jsonformat/observation_invalid_unicode.prototxt");
+
+ ASSERT_FALSE(PrettyPrintFhirToJsonString(proto).ok());
 }
 
 TEST(JsonFormatR4Test, PadsThreeDigitYearToFourCharacters) {

diff --git a/testdata/jsonformat/BUILD b/testdata/jsonformat/BUILD
@@ -6,8 +6,13 @@ licenses(["notice"])
 
 filegroup(
  name = "jsonformat",
- srcs = glob([
- "*.json",
- "*.prototxt",
- ]),
+ srcs = glob(
+ [
+ "*.json",
+ "*.prototxt",
+ ],
+ exclude = [
+ "observation_invalid_unicode.json",
+ ],
+ ),
 )
diff --git a/testdata/jsonformat/observation_control_characters.prototxt b/testdata/jsonformat/observation_control_characters.prototxt
diff --git a/...ormat/observation_control_characters.json → ...onformat/observation_invalid_unicode.json b/...ormat/observation_control_characters.json → ...onformat/observation_invalid_unicode.json
@@ -10,9 +10,5 @@
  }
  ]
  },
- "referenceRange" : [
- { "text": "\u0000" },
- { "text": "\r\n" },
- { "text": "\n" }
- ]
+ "valueString" : "foobarbaz\r\n\tquux"
 }
diff --git a/testdata/jsonformat/observation_invalid_unicode.prototxt b/testdata/jsonformat/observation_invalid_unicode.prototxt
@@ -0,0 +1,11 @@
+status { value: FINAL }
+code {
+ coding {
+ system { value: "foo" }
+ code { value: "bar" }
+ }
+}
+# Note that the only valid control characters are [\r\n\t], these are preserved
+# Other control characters (like \0 and \u0001) are dropped
+value { string_value { value: "foo\0bar\u0001baz\r\n\tquux" } }
+id { value: "123" }
diff --git a/testdata/primitives/String.prototxt b/testdata/primitives/String.prototxt
@@ -6,10 +6,6 @@ valid_pairs {
  json_string: "\"foo\\tbar\\rbaz\\nquux\""
  proto { string_proto { value: "foo\tbar\rbaz\nquux" } }
 }
-valid_pairs {
- json_string: "\"\\u0000\""
- proto { string_proto { value: "\0" } }
-}
 valid_pairs {
  json_string: "\"André 3000\""
  proto { string_proto { value: "Andr\303\251 3000" } }