Skip to content

Remove duplicate method calls in AccessEvaluator methods #99

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
dlmarion merged 3 commits into from
Jan 15, 2026
Merged

Remove duplicate method calls in AccessEvaluator methods #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ee251bd
Remove duplicate method calls in AccessEvaluator methods
dlmarion Jan 13, 2026
9962936
Merge branch 'main' into remove-duplicate-method-calls
dlmarion Jan 15, 2026
11494bb
Remove duplicate calls to method in for loop
dlmarion Jan 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion core/src/main/java/org/apache/accumulo/access/AuthorizationValidator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,8 @@ enum AuthorizationCharacters {
}

// Unsure what characters are present, so must validate them all.
for (int i = 0; i < auth.length(); i++) {
final int len = auth.length();
for (int i = 0; i < len; i++) {
var c = auth.charAt(i);
if (!Character.isDefined(c) || Character.isISOControl(c) || c == '\uFFFD') {
return false;
Expand Down
18 changes: 10 additions & 8 deletions core/src/main/java/org/apache/accumulo/access/impl/AccessEvaluatorImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@

import static org.apache.accumulo.access.impl.CharUtils.BACKSLASH;
import static org.apache.accumulo.access.impl.CharUtils.QUOTE;
import static org.apache.accumulo.access.impl.CharUtils.isBackslashSymbol;
import static org.apache.accumulo.access.impl.CharUtils.isQuoteOrSlash;
import static org.apache.accumulo.access.impl.CharUtils.isQuoteSymbol;

Expand Down Expand Up @@ -74,7 +75,8 @@ public AccessEvaluatorImpl(Authorizations authorizations,

public static CharSequence unescape(CharSequence auth) {
int escapeCharCount = 0;
for (int i = 0; i < auth.length(); i++) {
final int authLength = auth.length();
Copy link
Contributor

@keith-turner keith-turner Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could also make this change to avoid duplicate length() calls in AuthorizationValidator

Copy link
Contributor Author

@dlmarion dlmarion Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not seeing duplicate calls in that class, can you point me to it?

Copy link
Contributor

@keith-turner keith-turner Jan 13, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It only calls it once in the code, noticed the function call is repeatedly called at runtime for each loop iteration. That was what I was thinking of in terms of duplication, but not sure if its worthwhile to avoid that or not.

Copy link
Contributor Author

@dlmarion dlmarion Jan 15, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm seeing that where it is being called from an authorization is being passed in, presumably the auth token is unique. I'm not seeing a loop, can you point me to it?

Copy link
Contributor

@keith-turner keith-turner Jan 15, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following code is what I was thinking could benefit from the same changes being made elsewhere in this PR.

accumulo-access/core/src/main/java/org/apache/accumulo/access/AuthorizationValidator.java

Line 107 in c48e028

for (int i = 0; i < auth.length(); i++) {

for (int i = 0; i < authLength; i++) {
char c = auth.charAt(i);
if (isQuoteOrSlash(c)) {
escapeCharCount++;
Expand All @@ -86,11 +88,11 @@ public static CharSequence unescape(CharSequence auth) {
throw new IllegalArgumentException("Illegal escape sequence in auth : " + auth);
}

char[] unescapedCopy = new char[auth.length() - escapeCharCount / 2];
char[] unescapedCopy = new char[authLength - escapeCharCount / 2];
int pos = 0;
for (int i = 0; i < auth.length(); i++) {
for (int i = 0; i < authLength; i++) {
char c = auth.charAt(i);
if (c == BACKSLASH) {
if (isBackslashSymbol(c)) {
i++;
c = auth.charAt(i);
if (!isQuoteOrSlash(c)) {
Expand Down Expand Up @@ -120,17 +122,17 @@ public static CharSequence unescape(CharSequence auth) {
*/
public static CharSequence escape(CharSequence auth, boolean shouldQuote) {
int escapeCount = 0;

for (int i = 0; i < auth.length(); i++) {
final int authLength = auth.length();
for (int i = 0; i < authLength; i++) {
if (isQuoteOrSlash(auth.charAt(i))) {
escapeCount++;
}
}

if (escapeCount > 0 || shouldQuote) {
char[] escapedAuth = new char[auth.length() + escapeCount + (shouldQuote ? 2 : 0)];
char[] escapedAuth = new char[authLength + escapeCount + (shouldQuote ? 2 : 0)];
int index = shouldQuote ? 1 : 0;
for (int i = 0; i < auth.length(); i++) {
for (int i = 0; i < authLength; i++) {
char c = auth.charAt(i);
if (isQuoteOrSlash(c)) {
escapedAuth[index++] = BACKSLASH;
Expand Down
4 changes: 2 additions & 2 deletions core/src/main/java/org/apache/accumulo/access/impl/AccessExpressionImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@ public static CharSequence quote(CharSequence term) {
}

boolean needsQuote = false;

for (int i = 0; i < term.length(); i++) {
final int len = term.length();
for (int i = 0; i < len; i++) {
if (!Tokenizer.isValidAuthChar(term.charAt(i))) {
needsQuote = true;
break;
Expand Down