AVRO-3601: CustomAttributes#getAttribute() now returns boost::optional

[martin-g]

Add unit tests for CustomAttributes#getAttribute(string)

Jira

• https://issues.apache.org/jira/browse/AVRO-3601

Tests

• My PR adds unit tests

Commits

• My commits all reference Jira issues in their subject lines. In addition, my commits follow the guidelines from "How to write a good git commit message":
  1. Subject is separated from body by a blank line
  2. Subject is limited to 50 characters (not including Jira issue reference)
  3. Subject does not end with a period
  4. Subject uses the imperative mood ("add", not "adding")
  5. Body wraps at 72 characters
  6. Body explains "what" and "why", not "how"

Documentation

• In case of new functionality, my PR adds documentation that describes how to use it.
  • All the public functions and the classes in the PR contain Javadoc that explain what it does

[KalleOlaviNiemitalo]

As in #1821 (comment), I think this should be

Suggested change

	"[{\"name\": \"f1\", \"type\": \"long\", "
	"\"arrayField\": \"[1]\", "
	"\"booleanField\": \"true\", "
	"\"mapField\": \"{\\\"key1\\\":\\\"value1\\\", \\\"key2\\\":\\\"value2\\\"}\", "
	"\"nullField\": \"null\", "
	"\"numberField\": \"1.23\", "
	"\"stringField\": \"\\\"field value with \\\"double quotes\\\"\\\"\""
	"[{\"name\": \"f1\", \"type\": \"long\", "
	"\"arrayField\": [1], "
	"\"booleanField\": true, "
	"\"mapField\": {\"key1\":\"value1\", \"key2\":\"value2\"}, "
	"\"nullField\": null, "
	"\"numberField\": 1.23, "
	"\"stringField\": \"field value with \\\"double quotes\\\"\""

i.e. CustomAttributes.printJson should assume that the std::string values are already in JSON format, and write them out without adding any quotation marks around them or backslashes within them. Likewise, callers of CustomAttributes::addAttribute (especially in Compiler.cc) should provide a JSON-format std::string.

[KalleOlaviNiemitalo]

If CustomAttributes worked that way, then it would be able to use just std::string rather than boost::optional<std::string>, because an empty std::string could mean that the attribute is not present, while an std::string containing two quotation marks "" would mean that the value is an empty JSON string literal.

[KalleOlaviNiemitalo]

Would the JSON representation of custom attributes be compatible with Avro IDL? The IDL Language spec is not clear on whether the thing between parentheses in an annotation is always a JSON value.

[martin-g]

At the moment the content is preserved as whatever the user provided. It could be JSON, XML, base64, ...
It is up-to the user app to encode/decode the values.
You might be right about the non-optional representation ("") but IMO this way it is more clear. Other opinions are also welcome!

[KalleOlaviNiemitalo]

In an Avro schema file, must all custom attributes of fields have string values? I.e. is this invalid:

{
    "type": "record",
    "name": "Demo",
    "fields": [
        {
            "name": "field",
            "type": "string",
            "custom_flag": true
        }
    ]
}

If this schema is not invalid, then is the Avro C++ library able to load it from a file and then write it to another file, preserving the custom attribute?

[martin-g]

The Avro spec does not say anything about the possible value types of the custom attributes/metadata.

Until AVRO-3547 the C++ SDK didn't support it at all. (The Rust SDK still does not support this too. I expect a user to open a ticket/PR this week).
With AVRO-3601 we found out that using JsonDom.hh for the custom attributes is not recommended, thus the string-based approach.

I guess 1.11.2/1.12.0 will be released in several months, so whoever is interested in better handling of the custom attributes should step up and do it. Here I just tried to fix the broken installation of C++ SDK 1.11.1.

[KalleOlaviNiemitalo]

I think, minimally, the library should be able to read a schema that contains custom attributes with arbitrary value types, but not necessarily able to preserve the values in memory and write them out again. That would help compatibility with future versions of Avro, e.g. new standard logical types.

If CustomAttributes::attributes returns a reference to a map that contains the string values, then that makes it harder for a future version of the library to add support for other types without a breaking change.

[martin-g]

I guess 1.11.2/1.12.0 will be released in several months, so whoever is interested in better handling of the custom attributes should step up and do it. Here I just tried to fix the broken installation of C++ SDK 1.11.1.

Let me re-phrase the above: PRs are very welcome!