wgtmac commented on code in PR #3266:
URL: https://github.com/apache/avro/pull/3266#discussion_r1930670212
##########
lang/c++/include/avro/CustomAttributes.hh:
##########
@@ -19,27 +19,62 @@
#ifndef avro_CustomAttributes_hh__
#define avro_CustomAttributes_hh__
-#include "Config.hh"
#include <iostream>
#include <map>
#include <optional>
#include <string>
+#include "Config.hh"
+
namespace avro {
// CustomAttributes class stores avro custom attributes.
// Each attribute is represented by a unique name and value.
// User is supposed to create CustomAttributes object and then add it to
Schema.
class AVRO_DECL CustomAttributes {
+
public:
- // Retrieves the custom attribute json entity for that attributeName,
returns an
- // null if the attribute doesn't exist.
+ enum class ValueMode : uint8_t {
+ // When a CustomAttributes is created using this mode, all values are
expected
+ // to be strings. The value should not be quoted, but any interior
quotes and
+ // special characters (such as newlines) must be escaped.
+ STRING
+ [[deprecated("The JSON ValueMode is less error-prone and less
limited.")]],
Review Comment:
Perhaps add (in the comment) that it has been deprecated since 1.3.0 and
will be removed in XXX version?
##########
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"
Review Comment:
Is it better to add `#pragma clang diagnostic push|pop` to wrap only
`#include "CustomAttributes.hh"`?
##########
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:
Is it expensive to validate the input? If we don't do this, what will happen
for malformed input?
##########
lang/c++/impl/json/JsonIO.hh:
##########
@@ -116,6 +116,8 @@ public:
return curToken;
}
+ bool hasMore();
Review Comment:
Is it possible not to add `hasMore()` by reusing `peek()` above?
##########
lang/c++/impl/json/JsonIO.cc:
##########
@@ -49,6 +49,28 @@ char JsonParser::next() {
return ch;
}
+bool JsonParser::hasMore() {
+ if (peeked || hasNext && !isspace(nextChar)) {
Review Comment:
```suggestion
if (peeked || (hasNext && !isspace(nextChar))) {
```
--
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]
