Skip to content

Commit

Permalink
deprecate old string-only/ambiguous methods; update tests to use non-…
Browse files Browse the repository at this point in the history 
…deprecated versions (except one case, testing the deprecated ones); add test cases specifically for deprecated methods
  • Loading branch information
@jhump
jhump committed Dec 17, 2024
1 parent 07a2f4d commit 6740ac4
Show file tree
Hide file tree
Showing 3 changed files with 73 additions and 31 deletions.
4 changes: 2 additions & 2 deletions lang/c++/impl/NodeImpl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string, std::string>::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);
Expand Down
31 changes: 24 additions & 7 deletions lang/c++/include/avro/CustomAttributes.hh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<std::string> 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<std::string> getAttribute(const std::string &name) const;

// Adds a custom attribute with an arbitrary JSON value. The given string must
Expand All @@ -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<std::string, std::string> &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<std::string, std::string> &attributes() const {
return attributeStrings_;
}
Expand All @@ -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<std::string, std::string> attributeStrings_;

std::map<std::string, std::string> attributeJson_;
Expand Down
69 changes: 47 additions & 22 deletions lang/c++/test/unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -440,18 +440,18 @@ struct TestSchema {
std::vector<GenericDatum> defaultValues;
concepts::MultiAttribute<CustomAttributes> 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,
Expand Down Expand Up @@ -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() {
Expand All @@ -525,7 +549,8 @@ struct TestSchema {

checkNodeRecordWithoutCustomAttribute();
checkNodeRecordWithCustomAttribute();
checkCustomAttributes_getAttribute();
checkCustomAttributes_addAndGetAttributeJson();
checkCustomAttributes_deprecatedAddAndGetAttribute();
}

ValidSchema schema_;
Expand Down

0 comments on commit 6740ac4

Please sign in to comment.