jhump commented on code in PR #3266:
URL: https://github.com/apache/avro/pull/3266#discussion_r1931179865
##########
lang/c++/impl/CustomAttributes.cc:
##########
@@ -16,26 +16,63 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
-#include "CustomAttributes.hh"
-#include "Exception.hh"
+
+#if defined(__clang__)
+// Even though CustomAttributes::ValueMode::STRING is deprecated, we still
have to
+// handle/implement it.
+#pragma clang diagnostic ignored "-Wdeprecated-declarations"
+#endif
+
#include <map>
#include <memory>
+#include "CustomAttributes.hh"
+#include "Exception.hh"
+
+#include "json/JsonDom.hh"
+
namespace avro {
+CustomAttributes::CustomAttributes(ValueMode valueMode) {
+ switch (valueMode) {
+ case ValueMode::STRING:
+ case ValueMode::JSON:
+ valueMode_ = valueMode;
+ break;
+ default:
+ throw Exception("invalid ValueMode: " +
std::to_string(static_cast<int>(valueMode)));
+ }
+}
+
std::optional<std::string> CustomAttributes::getAttribute(const std::string
&name) const {
- std::optional<std::string> result;
- std::map<std::string, std::string>::const_iterator iter =
- attributes_.find(name);
+ auto iter = attributes_.find(name);
if (iter == attributes_.end()) {
- return result;
+ return {};
}
- result = iter->second;
- return result;
+ return iter->second;
}
void CustomAttributes::addAttribute(const std::string &name,
const std::string &value) {
+ // Validate the incoming value.
+ //
+ // NOTE: This is a bit annoying that we accept the data as a string
instead of
+ // as an Entity. That means the compiler must convert the value to a
string only
+ // for this method to convert it back. But we can't directly refer to the
+ // json::Entity type in the signatures for this class (and thus cannot
accept
+ // that type directly as a parameter) because then it would need to be
included
+ // from a header file: CustomAttributes.hh. But the json header files are
not
+ // part of the Avro distribution (intentionally), so CustomAttributes.hh
cannot
+ // #include any of the json header files.
+ const std::string &jsonVal = (valueMode_ == ValueMode::STRING)
+ ? std::move("\"" + value + "\"")
+ : value;
+ try {
Review Comment:
If we don't validate it, then we could end up generating invalid Avro files.
I suppose we could also make a private method that skips the validation and
declare the Compiler as a friend class that can invoke it. From the compiler,
the values will have already been validated when the JSON schema was originally
parsed, so it would be fine to skip in those cases.
The existing "string only" usage is error-prone enough, since it still
requires that interior quotes and newlines be escaped (since all it does is add
a leading and trailing `"` to turn it into a JSON value). But I figured it
would be even more error-prone with allowing any arbitrary JSON element
(including complicated values like arrays and objects) and also more
error-prone now that it has a modality that is defined at construction time.
Failing fast seemed like a much better developer experience than ignoring such
issues and letting the library generate an invalid file.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
