From 6740ac4989bd55c8ebaf29228994a8ccd6e50a69 Mon Sep 17 00:00:00 2001 From: Josh Humphries <2035234+jhump@users.noreply.github.com> Date: Tue, 17 Dec 2024 11:14:14 -0500 Subject: [PATCH] deprecate old string-only/ambiguous methods; update tests to use non-deprecated versions (except one case, testing the deprecated ones); add test cases specifically for deprecated methods --- lang/c++/impl/NodeImpl.cc | 4 +- lang/c++/include/avro/CustomAttributes.hh | 31 +++++++--- lang/c++/test/unittest.cc | 69 +++++++++++++++-------- 3 files changed, 73 insertions(+), 31 deletions(-) diff --git a/lang/c++/impl/NodeImpl.cc b/lang/c++/impl/NodeImpl.cc index a17732bcc51..b2e57d5b868 100644 --- a/lang/c++/impl/NodeImpl.cc +++ b/lang/c++/impl/NodeImpl.cc @@ -86,8 +86,8 @@ std::ostream &operator<<(std::ostream &os, indent x) { void printCustomAttributes(const CustomAttributes &customAttributes, size_t depth, std::ostream &os) { std::map::const_iterator iter = - customAttributes.attributes().begin(); - while (iter != customAttributes.attributes().end()) { + customAttributes.jsonAttributes().begin(); + while (iter != customAttributes.jsonAttributes().end()) { os << ",\n" << indent(depth); customAttributes.printJson(os, iter->first); diff --git a/lang/c++/include/avro/CustomAttributes.hh b/lang/c++/include/avro/CustomAttributes.hh index a4b4b9b8111..69a77c79ce9 100644 --- a/lang/c++/include/avro/CustomAttributes.hh +++ b/lang/c++/include/avro/CustomAttributes.hh @@ -36,13 +36,17 @@ class AVRO_DECL CustomAttributes { public: // Retrieves the custom attribute value for the given name as a JSON document. // Returns an empty value if the attribute doesn't exist. Unlike getAttribute, - // string values will be quoted and escaped. + // string values will always be quoted and escaped. std::optional getAttributeJson(const std::string &name) const; // Retrieves the custom attribute string for the given name. Returns an empty // value if the attribute doesn't exist. If the attribute value is not a string // (i.e. it's a number, boolean, array, or object), the stringified form (the - // value encoded as a JSON document) will be returned. + // value encoded as a JSON document) will be returned. This makes it ambiguous + // to the caller as to whether the value was a string or not. So getAttributeJson + // should be used instead, which does not have this ambiguity. This method is + // present only for backward compatibility. + [[deprecated("use getAttributeJson instead")]] std::optional getAttribute(const std::string &name) const; // Adds a custom attribute with an arbitrary JSON value. The given string must @@ -56,12 +60,25 @@ public: // Adds a custom attribute with a string value. If the attribute already exists, // throw an exception. Unlike with addAttributeJson, the string value must not - // be encoded (no quoting or escaping). + // be encoded (no quoting or escaping). This is only present for backward + // compatibility since it does not allow setting non-string values. So + // addAttributeJson should be used instead, which allows setting any kind of + // value. + [[deprecated("use addAttributeJson instead")]] void addAttribute(const std::string &name, const std::string &value); // Provides a way to iterate over the custom attributes or check attribute size. - // To query for the json element value of an entry, you can iterate over the keys - // of this map and then call getAttributeJson for each one. + // All values are encoded to JSON. So string values will be quoted and escaped. + const std::map &jsonAttributes() const { + return attributeJson_; + } + + // Provides a way to iterate over the custom attributes or check attribute size. + // The values in this map are the same as those returned from getAttribute. So + // string values are returned as-is but non-string values are encoded to JSON + // first. That means it is ambiguous as to whether a value is a string or not, + // so callers should prefer jsonAttributes instead. + [[deprecated("use jsonAttributes instead")]] const std::map &attributes() const { return attributeStrings_; } @@ -70,8 +87,8 @@ public: void printJson(std::ostream &os, const std::string &name) const; private: - // We have to keep a separate attribute strings map just to implement the - // attributes() method. This is just for API backwards-compatibility. + // We have to maintain a separate map in order to implement the + // attributes() method. This is just for API backward compatibility. std::map attributeStrings_; std::map attributeJson_; diff --git a/lang/c++/test/unittest.cc b/lang/c++/test/unittest.cc index c83cbb77d19..82359a97328 100644 --- a/lang/c++/test/unittest.cc +++ b/lang/c++/test/unittest.cc @@ -74,11 +74,11 @@ struct TestSchema { RecordSchema record("RootRecord"); CustomAttributes customAttributeLong; - customAttributeLong.addAttribute("extra_info_mylong", std::string("it's a long field")); + customAttributeLong.addAttributeJson("extra_info_mylong", std::string("\"it's a long field\"")); // Validate that adding a custom attribute with same name is not allowed bool caught = false; try { - customAttributeLong.addAttribute("extra_info_mylong", std::string("duplicate")); + customAttributeLong.addAttributeJson("extra_info_mylong", std::string("\"duplicate\"")); } catch (Exception &e) { std::cout << "(intentional) exception: " << e.what() << '\n'; caught = true; @@ -140,10 +140,10 @@ struct TestSchema { BOOST_CHECK_EQUAL(caught, true); CustomAttributes customAttributeLong2; - customAttributeLong2.addAttribute("extra_info_mylong2", - std::string("it's a long field")); - customAttributeLong2.addAttribute("more_info_mylong2", - std::string("it's still a long field")); + customAttributeLong2.addAttributeJson("extra_info_mylong2", + std::string("\"it's a long field\"")); + customAttributeLong2.addAttributeJson("more_info_mylong2", + std::string("\"it's still a long field\"")); record.addField("mylong2", LongSchema(), customAttributeLong2); record.addField("anotherint", intSchema); @@ -440,18 +440,18 @@ struct TestSchema { std::vector defaultValues; concepts::MultiAttribute customAttributes; - CustomAttributes cf; - cf.addAttribute("stringField", std::string("foobar")); - cf.addAttribute("stringFieldComplex", std::string("\" a field value with \"double quotes\" \"")); - cf.addAttributeJson("stringFieldComplex2", std::string("\"\\\" a field value with \\\"double quotes\\\" \\\"\"")); - cf.addAttributeJson("booleanField", std::string("true")); - cf.addAttributeJson("numberField", std::string("1.23")); - cf.addAttributeJson("nullField", std::string("null")); - cf.addAttributeJson("arrayField", std::string("[1]")); - cf.addAttributeJson("mapField", std::string("{\"key1\":\"value1\", \"key2\":\"value2\"}")); + CustomAttributes ca; + ca.addAttribute("stringField", std::string("foobar")); + ca.addAttribute("stringFieldComplex", std::string("\" a field value with \"double quotes\" \"")); + ca.addAttributeJson("stringFieldComplex2", std::string("\"\\\" a field value with \\\"double quotes\\\" \\\"\"")); + ca.addAttributeJson("booleanField", std::string("true")); + ca.addAttributeJson("numberField", std::string("1.23")); + ca.addAttributeJson("nullField", std::string("null")); + ca.addAttributeJson("arrayField", std::string("[1]")); + ca.addAttributeJson("mapField", std::string("{\"key1\":\"value1\", \"key2\":\"value2\"}")); fieldNames.add("f1"); fieldValues.add(NodePtr(new NodePrimitive(Type::AVRO_LONG))); - customAttributes.add(cf); + customAttributes.add(ca); NodeRecord nodeRecordWithCustomAttribute(nameConcept, fieldValues, fieldNames, fieldAliases, defaultValues, @@ -493,12 +493,36 @@ struct TestSchema { expectedJsonWithoutCustomAttribute); } - void checkCustomAttributes_getAttribute() { - CustomAttributes cf; - cf.addAttribute("field1", std::string("1")); + void checkCustomAttributes_addAndGetAttributeJson() { + CustomAttributes ca; + ca.addAttributeJson("field1", std::string("true")); - BOOST_CHECK_EQUAL(std::string("1"), *cf.getAttribute("field1")); - BOOST_CHECK_EQUAL(false, cf.getAttribute("not_existing").has_value()); + BOOST_CHECK_EQUAL(std::string("true"), *ca.getAttributeJson("field1")); + BOOST_CHECK_EQUAL(false, ca.getAttributeJson("not_existing").has_value()); + } + + void checkCustomAttributes_deprecatedAddAndGetAttribute() { + CustomAttributes ca; + ca.addAttribute("field1", std::string("foo bar")); + ca.addAttributeJson("field2", std::string("true")); + + BOOST_CHECK_EQUAL(std::string("foo bar"), *ca.getAttribute("field1")); + BOOST_CHECK_EQUAL(std::string("true"), *ca.getAttribute("field2")); + BOOST_CHECK_EQUAL(false, ca.getAttribute("not_existing").has_value()); + // non-deprecated version quotes strings + BOOST_CHECK_EQUAL(std::string("\"foo bar\""), *ca.getAttributeJson("field1")); + + // also check the deprecated map accessor + auto map = ca.attributes(); + BOOST_CHECK_EQUAL(2, map.size()); + auto iter = map.begin(); + BOOST_CHECK_EQUAL("field1", iter->first); + BOOST_CHECK_EQUAL("foo bar", iter->second); + iter++; + BOOST_CHECK_EQUAL("field2", iter->first); + BOOST_CHECK_EQUAL("true", iter->second); + iter++; + BOOST_CHECK(iter == map.end()); } void test() { @@ -525,7 +549,8 @@ struct TestSchema { checkNodeRecordWithoutCustomAttribute(); checkNodeRecordWithCustomAttribute(); - checkCustomAttributes_getAttribute(); + checkCustomAttributes_addAndGetAttributeJson(); + checkCustomAttributes_deprecatedAddAndGetAttribute(); } ValidSchema schema_;