jhump commented on code in PR #3266:
URL: https://github.com/apache/avro/pull/3266#discussion_r1932305894
##########
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:
> 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.
Welp, this won't work. The compiler isn't a class but a set of functions.
But the real issue is that the `CustomAttributes.addAttribute` method is called
from a static function in `Compiler.cc`, so we can't possibly make it a friend
of `CustomAttributes`. If you have any other recommendation for how we might
omit the validation just from the compiler, I will happily implement it.
I suppose another option is to just make it another constructor argument,
such as a new `ValidationMode`, and then document that it's unwise to use the
"no validate" mode unless the caller is doing something else to independently
validate the values beforehand, since it could otherwise result in
invalid/unreadable Avro files.
What do you think?
--
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: issues-unsubscr...@avro.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
