ctubbsii commented on code in PR #6402:
URL: https://github.com/apache/accumulo/pull/6402#discussion_r3562718206
##########
core/src/main/java/org/apache/accumulo/core/iterators/user/VisibilityFilter.java:
##########
@@ -132,20 +155,31 @@ public IteratorOptions describeOptions() {
IteratorOptions io = super.describeOptions();
io.setName("visibilityFilter");
io.setDescription("The VisibilityFilter allows you to filter for key/value"
- + " pairs by a set of authorizations or filter invalid labels from
corrupt files.");
+ + " pairs by a set(s) of authorizations or filter invalid labels from
corrupt files.");
io.addNamedOption(FILTER_INVALID_ONLY,
"if 'true', the iterator is instructed to ignore the authorizations
and"
+ " only filter invalid visibility labels (default: false)");
- io.addNamedOption(AUTHS,
- "the serialized set of authorizations to filter against (default:
empty"
- + " string, accepts only entries visible by all)");
+ io.addNamedOption(AUTHS, "The set of Authorizations to use when filtering
(default: empty"
+ + " string, accepts only entries visible by all). The value can"
+ + " either be of the older form (\"auth1,auth2\") which only supports"
+ + " a single Authorizations object, or the newer form which uses " +
Authorizations.HEADER
+ + " concatenated with Base64 encoded comma-separated authorization
tokens."
+ + "The latter case supports one or more Authorizations (\"" +
Authorizations.HEADER
+ + "Base64(auth1,auth2)\") or (\"" + Authorizations.HEADER +
"Base64(auth1,auth2)"
+ + Authorizations.HEADER + "auth2,auth3\")");
Review Comment:
We haven't serialized using the old format since before 1.5.0. I kinda
forgot about that.
It's fine if we continue to support that, but the description should
instruct users on which serialization format they should be using, which should
be `VisibilityFilter.setAuthorizations()` or if the config is manually set,
they should be serialized using `Authorizations.serialize()`.
Previously, this said "the serialized set of authorizations". I think it
should still say that. The wording can be something like "concatenated
serialized sets of authorizations to filter against". I don't think it should
go into detail about the serialization format, but should instead defer to the
setAuthorizations method and Authorizations.serialize.
##########
core/src/test/java/org/apache/accumulo/core/iterators/user/VisibilityFilterTest.java:
##########
@@ -256,4 +290,11 @@ public void testDeepCopyAfterInit() throws IOException {
assertTrue(copyFilter.accept(k, new Value()));
}
+ @Test
+ public void testAuthorizationSerialization() {
+ String auth = new Authorizations("abc", "def").serialize();
+ System.out.println(auth);
+
+ }
+
Review Comment:
This test isn't doing any testing. Looks like it was examining the output
during dev. Probably can delete it now.
--
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]
