You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@polaris.apache.org by "XN137 (via GitHub)" <gi...@apache.org> on 2025/05/23 13:22:10 UTC
[PR] fix and enforce more errorprone checks [polaris]
XN137 opened a new pull request, #1663:
URL: https://github.com/apache/polaris/pull/1663
enforces the following checks:
https://errorprone.info/bugpattern/ObjectsHashCodePrimitive
https://errorprone.info/bugpattern/OptionalMapToOptional
https://errorprone.info/bugpattern/StringCharset
https://errorprone.info/bugpattern/VariableNameSameAsType
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix and enforce more errorprone checks [polaris]
Posted by "XN137 (via GitHub)" <gi...@apache.org>.
XN137 commented on code in PR #1663:
URL: https://github.com/apache/polaris/pull/1663#discussion_r2104829919
##########
api/iceberg-service/src/main/java/org/apache/polaris/service/types/NotificationType.java:
##########
@@ -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()) {
+ if (name.equalsIgnoreCase(notificationType.name())) {
Review Comment:
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.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix and enforce more errorprone checks [polaris]
Posted by "eric-maynard (via GitHub)" <gi...@apache.org>.
eric-maynard commented on code in PR #1663:
URL: https://github.com/apache/polaris/pull/1663#discussion_r2104782845
##########
codestyle/errorprone-rules.properties:
##########
@@ -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?
Review Comment:
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
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix and enforce more errorprone checks [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b merged PR #1663:
URL: https://github.com/apache/polaris/pull/1663
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix and enforce more errorprone checks [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1663:
URL: https://github.com/apache/polaris/pull/1663#discussion_r2104794680
##########
codestyle/errorprone-rules.properties:
##########
@@ -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?
Review Comment:
The need to 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.
##########
codestyle/errorprone-rules.properties:
##########
@@ -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?
Review Comment:
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.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix and enforce more errorprone checks [polaris]
Posted by "dimas-b (via GitHub)" <gi...@apache.org>.
dimas-b commented on code in PR #1663:
URL: https://github.com/apache/polaris/pull/1663#discussion_r2104800845
##########
api/iceberg-service/src/main/java/org/apache/polaris/service/types/NotificationType.java:
##########
@@ -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()) {
+ if (name.equalsIgnoreCase(notificationType.name())) {
Review Comment:
Is this flagged by ErrorProne or collateral?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix and enforce more errorprone checks [polaris]
Posted by "XN137 (via GitHub)" <gi...@apache.org>.
XN137 commented on code in PR #1663:
URL: https://github.com/apache/polaris/pull/1663#discussion_r2104607933
##########
api/iceberg-service/src/main/java/org/apache/polaris/service/types/NotificationType.java:
##########
@@ -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()) {
Review Comment:
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()`)
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix and enforce more errorprone checks [polaris]
Posted by "XN137 (via GitHub)" <gi...@apache.org>.
XN137 commented on code in PR #1663:
URL: https://github.com/apache/polaris/pull/1663#discussion_r2104606805
##########
polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java:
##########
@@ -129,7 +128,7 @@ public boolean equals(Object o) {
@Override
public int hashCode() {
- return Objects.hashCode(success);
+ return Boolean.hashCode(success);
Review Comment:
just wondering: the class seems to intentionally exclude the `message` field from the `equals` and `hashCode` contract?
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix and enforce more errorprone checks [polaris]
Posted by "XN137 (via GitHub)" <gi...@apache.org>.
XN137 commented on code in PR #1663:
URL: https://github.com/apache/polaris/pull/1663#discussion_r2104605453
##########
quarkus/service/src/main/java/org/apache/polaris/service/quarkus/config/ProductionReadinessChecks.java:
##########
@@ -151,7 +151,7 @@ public ProductionReadinessCheck checkTokenBrokers(
if (config
.tokenBroker()
.symmetricKey()
- .map(SymmetricKeyConfiguration::secret)
+ .flatMap(SymmetricKeyConfiguration::secret)
Review Comment:
looking at https://github.com/apache/polaris/blob/e1c0a1ccdad4e4e432c026cdc3ae30dcb8112da1/service/common/src/main/java/org/apache/polaris/service/auth/AuthenticationRealmConfiguration.java#L73-L84
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.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix and enforce more errorprone checks [polaris]
Posted by "adutra (via GitHub)" <gi...@apache.org>.
adutra commented on code in PR #1663:
URL: https://github.com/apache/polaris/pull/1663#discussion_r2104830387
##########
codestyle/errorprone-rules.properties:
##########
@@ -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?
Review Comment:
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.
--
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: issues-unsubscribe@polaris.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org