dlmarion commented on code in PR #5293:
URL: https://github.com/apache/accumulo/pull/5293#discussion_r1935539626
##########
core/src/main/java/org/apache/accumulo/core/security/ColumnVisibility.java:
##########
@@ -502,7 +520,36 @@ public ColumnVisibility(Text expression) {
* @see #ColumnVisibility(String)
*/
public ColumnVisibility(byte[] expression) {
- validate(expression);
+ this(expression, true);
+ }
+
+ /**
+ * Creates a column visibility for a mutation from a string already encoded
in UTF-8 bytes. Allows
+ * validation to be skipped for use with accumulo-access AccessExpressions
+ *
+ * @param expression visibility expression, encoded as UTF-8 bytes
+ * @param validate enables or disables validation and parse tree generation
+ */
+ private ColumnVisibility(byte[] expression, boolean validate) {
+ if (validate) {
+ validate(expression);
+ } else {
+ this.expression = Arrays.copyOf(expression, expression.length);
+ this.node = EMPTY_NODE;
+ }
+ }
+
+ /**
+ * Creates a new column visibility by performing a deep copy of an existing
column visibility
+ * object
+ *
+ * @param visibility ColumnVisibility object
+ * @since 2.1.4
+ */
+ public ColumnVisibility(ColumnVisibility visibility) {
+ byte[] incomingExpression = visibility.expression;
+ this.expression = Arrays.copyOf(incomingExpression,
incomingExpression.length);
+ this.node = new Node(visibility.node);
Review Comment:
I think there is an issue here with the parse tree being mutable. Given that
ColumnVisibility's can be cached, and that the parse tree is mutable, it's
possible that the parse tree could be mutating in another thread while this
copy constructor is called. I'm not sure how to resolve this issue. Using the
Cloneable interface and adding a `clone` method doesn't fully solve this issue
either. I think this issue exists in #5217 as well.
##########
core/src/main/java/org/apache/accumulo/core/security/ColumnVisibility.java:
##########
@@ -502,7 +520,36 @@ public ColumnVisibility(Text expression) {
* @see #ColumnVisibility(String)
*/
public ColumnVisibility(byte[] expression) {
- validate(expression);
+ this(expression, true);
+ }
+
+ /**
+ * Creates a column visibility for a mutation from a string already encoded
in UTF-8 bytes. Allows
+ * validation to be skipped for use with accumulo-access AccessExpressions
+ *
+ * @param expression visibility expression, encoded as UTF-8 bytes
+ * @param validate enables or disables validation and parse tree generation
+ */
+ private ColumnVisibility(byte[] expression, boolean validate) {
+ if (validate) {
+ validate(expression);
+ } else {
+ this.expression = Arrays.copyOf(expression, expression.length);
+ this.node = EMPTY_NODE;
Review Comment:
I think the `expression` needs to be parsed to populate `node` otherwise we
run into issues in other areas of the code. For example, here in
[VisibilityEvaluator](https://github.com/apache/accumulo/blob/2.1/core/src/main/java/org/apache/accumulo/core/security/VisibilityEvaluator.java#L208)
an Exception will be thrown. Here in
[NodeComparator](https://github.com/apache/accumulo/blob/2.1/core/src/main/java/org/apache/accumulo/core/security/ColumnVisibility.java#L193)
all ColumnVisibilities that are created this way will be "equal".
--
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]
