absurdfarce commented on code in PR #1964:
URL:
https://github.com/apache/cassandra-java-driver/pull/1964#discussion_r1792608417
##########
query-builder/src/main/java/com/datastax/oss/driver/api/querybuilder/schema/RelationOptions.java:
##########
@@ -336,4 +337,11 @@ default SelfT withReadRepairChance(double
readRepairChance) {
default SelfT withSpeculativeRetry(@NonNull String speculativeRetry) {
return withOption("speculative_retry", speculativeRetry);
}
+
+ /** Attaches custom metadata to CQL table definition. */
+ @NonNull
+ @CheckReturnValue
+ default SelfT withExtensions(@NonNull Map<String, byte[]> extensions) {
+ return withOption("extensions", extensions);
+ }
Review Comment:
That's correct, but that's driven by a quirk regarding the encoding of
Strings. OptionsUtils.extractOptionValue() just blindly assumes that any
string passed in should be understood as a CQL string (and thus should be
quoted). We can work around this by any number of methods. Pretty much any
type other than a String (or String subtype) would get what we want, so for
example this actually works fine:
```java
/** Attaches custom metadata to CQL table definition. */
@NonNull
@CheckReturnValue
default SelfT withExtensions(@NonNull Map<String, byte[]> extensions) {
return withOption("extensions", Maps.transformValues(extensions, (s) ->
CharBuffer.wrap(ByteUtils.toHexString(s))));
}
```
But the above code is still relying on details of the implementation (the
fact that CharBuffer isn't a String) to do what it needs to do... it's not
clear to someone reading the code why we're doing that.
We could modify extractOptionValue() to take an arg governing whether
strings should be quoted or not but then that arg would have to be pushed down
through the call to OptionsUtils.buildOptions() in DefaultCreateTable... which
isn't exactly intuitive either.
A far better approach would be to rely on the type system:
```java
/**
* Wrapper class to indicate that the contained String value should be
understood to represent
* a CQL literal that can be included directly in a CQL statement (i.e.
without escaping).
*/
class CqlLiteral {
private final String val;
public CqlLiteral(String val) {
this.val = val;
}
public String toString() { return this.val; }
}
...
/** Attaches custom metadata to CQL table definition. */
@NonNull
@CheckReturnValue
default SelfT withExtensions(@NonNull Map<String, byte[]> extensions) {
return withOption("extensions", Maps.transformValues(extensions, (s) ->
new CqlLiteral(ByteUtils.toHexString(s))));
}
```
This also makes the test pass and is _much_ clearer with respect to our
intent.
Finally, I'd note that the question of making these quotes come out okay is
very different from the question of whether we should encapsulate the logic in
RelationOptions.withExtensions() or whether we should force changes to
OptionsUtils to make it work. If we believe that this encapsulation is useful
we should seek out one or more options to the encoding question (as discussed
above) and not scrap the idea in it's entirety.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
