Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
package org.apache.polaris.service.types;

import java.util.Arrays;
import java.util.Locale;
import java.util.Map;
import java.util.Optional;
import java.util.stream.Collectors;
Expand Down Expand Up @@ -84,9 +83,9 @@ public static Optional<NotificationType> lookupByName(String name) {
return Optional.empty();
}

for (NotificationType NotificationType : NotificationType.values()) {
if (name.toUpperCase(Locale.ROOT).equals(NotificationType.name())) {
return Optional.of(NotificationType);
for (NotificationType notificationType : NotificationType.values()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the lookupByName method seems completely unused, so we could also just remove it instead afaict.

same applies to the displayName field (whose value is always the same as name())

if (name.equalsIgnoreCase(notificationType.name())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this flagged by ErrorProne or collateral?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

collateral as i thought it didnt make sense to always call uppercase inside the loop.
since the method is unused we can still remove all of it.

return Optional.of(notificationType);
}
}
return Optional.empty();
Expand Down
12 changes: 12 additions & 0 deletions codestyle/errorprone-rules.properties
Original file line number Diff line number Diff line change
Expand Up @@ -227,6 +227,12 @@ UseCorrectAssertInTests=ERROR
ConstantPatternCompile=ERROR
# Variables initialized with Pattern#compile calls on constants can be constants

ObjectsHashCodePrimitive=ERROR
# Objects.hashCode(Object o) should not be passed a primitive value

OptionalMapToOptional=ERROR
# Mapping to another Optional will yield a nested Optional. Did you mean flatMap?
Comment on lines +233 to +234
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the only one I'm not so sure about. The bug you found looks real to me, but this is a legit pattern and I'm a bit wary of disallowing it

Copy link
Contributor

@dimas-b dimas-b May 23, 2025

Choose a reason for hiding this comment

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

The need for nested Optional is quite hypothetical, but the error pattern is common in my experience. I think the check is worth enforcing for now. We can remove it if/when we have to.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with @dimas-b; and in the unlikely case where there is a compelling reason to have Optional<Optional<...>>, we can also add a @SuppressWarnings annotation.


PrimitiveArrayPassedToVarargsMethod=ERROR
# Passing a primitive array to a varargs method is usually wrong

Expand All @@ -239,6 +245,9 @@ RedundantThrows=ERROR
StringCaseLocaleUsage=ERROR
# Specify a `Locale` when calling `String#to{Lower,Upper}Case`. (Note: there are multiple suggested fixes; the third may be most appropriate if you're dealing with ASCII Strings.)

StringCharset=ERROR
# Prefer StandardCharsets over using string names for charsets

StronglyTypeByteString=WARN
# This primitive byte array is only used to construct ByteStrings. It would be clearer to strongly type the field instead.

Expand All @@ -254,6 +263,9 @@ TransientMisuse=ERROR
UrlInSee=ERROR
# URLs should not be used in @see tags; they are designed for Java elements which could be used with @link.

VariableNameSameAsType=ERROR
# variableName and type with the same name would refer to the static field instead of the class

####################################################################################################
# Experimental : SUGGESTION
# See https://errorprone.info/bugpatterns
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
package org.apache.polaris.core.secrets;

import jakarta.annotation.Nonnull;
import java.io.UnsupportedEncodingException;
import java.nio.charset.StandardCharsets;
import java.security.SecureRandom;
import java.util.Base64;
import java.util.HashMap;
Expand Down Expand Up @@ -54,12 +54,7 @@ public UserSecretReference writeSecret(
// secret as well as the secretReferencePayload to recover the original secret, we'll use
// basic XOR encryption and store the randomly generated key in the reference payload.
// A production implementation will typically use a standard crypto library if applicable.
byte[] secretBytes;
try {
secretBytes = secret.getBytes("UTF-8");
} catch (UnsupportedEncodingException e) {
throw new RuntimeException(e);
}
byte[] secretBytes = secret.getBytes(StandardCharsets.UTF_8);
byte[] oneTimeKey = new byte[secretBytes.length];
byte[] cipherTextBytes = new byte[secretBytes.length];

Expand Down Expand Up @@ -143,11 +138,7 @@ public String readSecret(@Nonnull UserSecretReference secretReference) {
secretBytes[i] = (byte) (cipherTextBytes[i] ^ oneTimeKey[i]);
}

try {
return new String(secretBytes, "UTF-8");
} catch (UnsupportedEncodingException e) {
throw new RuntimeException(e);
}
return new String(secretBytes, StandardCharsets.UTF_8);
}

/** {@inheritDoc} */
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import jakarta.annotation.Nonnull;
import java.util.EnumMap;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import org.apache.polaris.core.PolarisDiagnostics;

Expand Down Expand Up @@ -129,7 +128,7 @@ public boolean equals(Object o) {

@Override
public int hashCode() {
return Objects.hashCode(success);
return Boolean.hashCode(success);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

just wondering: the class seems to intentionally exclude the message field from the equals and hashCode contract?

}

@Override
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ public ProductionReadinessCheck checkTokenBrokers(
if (config
.tokenBroker()
.symmetricKey()
.map(SymmetricKeyConfiguration::secret)
.flatMap(SymmetricKeyConfiguration::secret)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

looking at

interface SymmetricKeyConfiguration {
/**
* The secret to use for both signing and verifying signatures. Either this option of {@link
* #file()} must be provided.
*/
Optional<String> secret();
/**
* The file to read the secret from. Either this option of {@link #secret()} must be provided.
*/
Optional<Path> file();

this might actually be a bug fix in that previously any symmetricKey was flagged as an error?
i.e. this line was checking Optional.of(Optional.empty()).isPresent() when the SymmetricKeyConfiguration is file-based.

.isPresent()) {
errors.add(
Error.of(
Expand Down
Loading