fix and enforce more errorprone checks #1663

XN137 · 2025-05-23T13:22:07Z

enforces the following checks:
https://errorprone.info/bugpattern/ObjectsHashCodePrimitive
https://errorprone.info/bugpattern/OptionalMapToOptional
https://errorprone.info/bugpattern/StringCharset
https://errorprone.info/bugpattern/VariableNameSameAsType

enforces the following checks: https://errorprone.info/bugpattern/ObjectsHashCodePrimitive https://errorprone.info/bugpattern/OptionalMapToOptional https://errorprone.info/bugpattern/StringCharset https://errorprone.info/bugpattern/VariableNameSameAsType

snazy

I'm fine with the additional checks and the code fixes.
The flatMap fix looks nit-y enough for this PR.

XN137 · 2025-05-23T13:31:51Z

...rvice/src/main/java/org/apache/polaris/service/quarkus/config/ProductionReadinessChecks.java

                      .tokenBroker()
                      .symmetricKey()
-                      .map(SymmetricKeyConfiguration::secret)
+                      .flatMap(SymmetricKeyConfiguration::secret)


looking at

polaris/service/common/src/main/java/org/apache/polaris/service/auth/AuthenticationRealmConfiguration.java

Lines 73 to 84 in e1c0a1c

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.

XN137 · 2025-05-23T13:32:31Z

polaris-core/src/main/java/org/apache/polaris/core/storage/PolarisStorageIntegration.java

    @Override
    public int hashCode() {
-      return Objects.hashCode(success);
+      return Boolean.hashCode(success);


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

XN137 · 2025-05-23T13:33:08Z

api/iceberg-service/src/main/java/org/apache/polaris/service/types/NotificationType.java

-    for (NotificationType NotificationType : NotificationType.values()) {
-      if (name.toUpperCase(Locale.ROOT).equals(NotificationType.name())) {
-        return Optional.of(NotificationType);
+    for (NotificationType notificationType : NotificationType.values()) {


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())

eric-maynard · 2025-05-23T14:59:27Z

codestyle/errorprone-rules.properties

+OptionalMapToOptional=ERROR
+# Mapping to another Optional will yield a nested Optional. Did you mean flatMap?


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

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.

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.

dimas-b

LGTM 👍 Thanks for looking into this @XN137 !

dimas-b · 2025-05-23T15:09:31Z

api/iceberg-service/src/main/java/org/apache/polaris/service/types/NotificationType.java

-      if (name.toUpperCase(Locale.ROOT).equals(NotificationType.name())) {
-        return Optional.of(NotificationType);
+    for (NotificationType notificationType : NotificationType.values()) {
+      if (name.equalsIgnoreCase(notificationType.name())) {


Is this flagged by ErrorProne or collateral?

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.

* fix(nightly-CI): Do not publish snapshots from forks (apache#1635) Adopt the `Nightly Build` workflow to not (try to) publish every night from forks. * main: Update dependency io.smallrye.config:smallrye-config-core to v3.13.0 (apache#1637) * Use echo to print script errors (apache#1648) * [HOTFIX] QUICKSTART (apache#1646) The change adds the following to fix Quick start experience : [1] ENV variables required by common assets after apache#1522 [2] New configs required to enable FILE based sources apache#1649 Co-authored-by: singhpk234 <singhpk234@users.noreply.github.com> Co-authored-by: pjanuario <pjanuario@users.noreply.github.com> * main: Update dependency gradle to v8.14.1 (apache#1652) * main: Update dependency gradle to v8.14.1 * Re-adopt PR to the project's needs --------- Co-authored-by: Robert Stupp <snazy@snazy.de> * [Policy Store] Add policyTypeCode to Slice/Index for Future Filtering Support and Update Policy Persistence Method (apache#1628) This PR adds policyTypeCode to the in-memory tree map store's slice and the SQL index on policy_mapping_records (already done in JDBC in apache#1468). This prepares for future features that need to filter efficiently by policy type, like listing all entities with a data compaction policy. It also updates the loadAllTargetsOnPolicy method to accept policyTypeCode, enabling it to use the new index for better performance. * fix(test): Do not let some more tests spam `/tmp` (apache#1651) * fix(test): Do not let some more tests not spam `/tmp` * `PolarisRestCatalogViewFileIntegrationTest` * `FileIOExceptionsTest` * `PolarisRestCatalogViewFileIntegrationTest` Changes the tests to leverage JUnit's `@TempDir`. Simplifies `PolarisEclipseLinkMetaStoreManagerTest` * review: rename the (now) abstract class * fix(testing): Do not let PolarisOverlappingTableTest spam `/tmp` (apache#1641) Changes the test to leverage JUnit's `@TempDir`. * Add CATALOG_MANAGE_METADATA to super privilege set of policy attachment privileges (apache#1643) * Fix quickstart doc with docker compose (apache#1610) * main: Update dependency boto3 to v1.38.22 (apache#1657) * Refactor IcebergCatalog to isolate internal state (apache#1659) Following up on apache#1694 * Restore `private` scope on internal fields in `IcebergCatalog` * Use a test-only setter instead of sub-classing to manage injecting test FileIO implementations * Refactor: Use per-request STS credentials (apache#1629) * Refactor: Use per-request STS credentials No functional changes. This is mostly to allow more storage integration flexibility in downstream build. This might also be useful for non-AWS storage. * fix and enforce more errorprone checks (apache#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 * Create a wrapper script to generate python client; regenerate the python client (apache#1347) As noted in apache#755 and elsewhere, the generated types in client/python are currently out of date. This introduces a script to regenerate them and a gradle task to run that script. I've also run the script, which necessitated several things to get tests passing: 1. There were small nonfunctional spec changes needed in order to keep the Python client working 2. The CLI and its tests required a few fixes to work with the updated Python client 3. Many of the regtests required fixes to work with the updated Python client * [Python Client] CI for Python client (Continue PR#1096) (apache#1639) Adds CI for python client. It does not include caching poetry step for now since we do not have poetry.lock (it is in .gitignore), see relevant discussion in: apache#1102 (comment), apache#1096 (comment), we can add that later * main: Update actions/setup-python action to v5 (apache#1671) * main: Update actions/checkout action to v4 (apache#1670) * main: Update python Docker tag to v3.13 (apache#1669) * main: Update dependency pytest to ~=7.4.4 (apache#1668) * main: Update dependency software.amazon.awssdk:bom to v2.31.50 (apache#1677) * main: Update dependency boto3 to v1.38.23 (apache#1667) * feat(build): make archive builds reproducible (apache#1664) See https://docs.gradle.org/current/userguide/working_with_files.html#sec:reproducible_archives * main: Update dependency io.prometheus:prometheus-metrics-exporter-servlet-jakarta to v1.3.8 (apache#1679) * NoSQL: adapt to change on oss/main * INFO: Last merged commit: 6ef8b3e --------- Co-authored-by: Mend Renovate <bot@renovateapp.com> Co-authored-by: ModEtchFill <50123102+ModEtchFill@users.noreply.github.com> Co-authored-by: Prashant Singh <35593236+singhpk234@users.noreply.github.com> Co-authored-by: singhpk234 <singhpk234@users.noreply.github.com> Co-authored-by: pjanuario <pjanuario@users.noreply.github.com> Co-authored-by: Honah (Jonas) J. <honahx@apache.org> Co-authored-by: MonkeyCanCode <yongzheng0809@gmail.com> Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@dremio.com> Co-authored-by: Dmitri Bourlatchkov <dmitri.bourlatchkov@gmail.com> Co-authored-by: Christopher Lambert <xn137@gmx.de> Co-authored-by: Eric Maynard <eric.maynard+oss@snowflake.com>

github-project-automation bot added this to Basic Kanban Board May 23, 2025

github-project-automation bot moved this to PRs In Progress in Basic Kanban Board May 23, 2025

snazy approved these changes May 23, 2025

View reviewed changes

github-project-automation bot moved this from PRs In Progress to Ready to merge in Basic Kanban Board May 23, 2025

XN137 commented May 23, 2025

View reviewed changes

eric-maynard reviewed May 23, 2025

View reviewed changes

dimas-b approved these changes May 23, 2025

View reviewed changes

XN137 marked this pull request as ready for review May 23, 2025 15:26

XN137 requested review from HonahX, MonkeyCanCode, RussellSpitzer, ajantha-bhat, ashvina, collado-mike, dennishuo, ebyhr, flyrain, jackye1995, jbonofre, pingtimeout, singhpk234, takidau and vvcephei as code owners May 23, 2025 15:26

adutra approved these changes May 23, 2025

View reviewed changes

eric-maynard approved these changes May 23, 2025

View reviewed changes

dimas-b merged commit 6e6da48 into apache:main May 23, 2025
6 checks passed

github-project-automation bot moved this from Ready to merge to Done in Basic Kanban Board May 23, 2025

XN137 deleted the enforce-more-errorprone-checks branch May 23, 2025 16:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix and enforce more errorprone checks #1663

fix and enforce more errorprone checks #1663

Uh oh!

XN137 commented May 23, 2025

Uh oh!

snazy left a comment

Uh oh!

XN137 May 23, 2025

Uh oh!

XN137 May 23, 2025

Uh oh!

XN137 May 23, 2025

Uh oh!

eric-maynard May 23, 2025

Uh oh!

dimas-b May 23, 2025 •

edited

Loading

Uh oh!

adutra May 23, 2025

Uh oh!

dimas-b left a comment

Uh oh!

dimas-b May 23, 2025

Uh oh!

XN137 May 23, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	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();

		OptionalMapToOptional=ERROR
		# Mapping to another Optional will yield a nested Optional. Did you mean flatMap?

fix and enforce more errorprone checks #1663

fix and enforce more errorprone checks #1663

Uh oh!

Conversation

XN137 commented May 23, 2025

Uh oh!

snazy left a comment

Choose a reason for hiding this comment

Uh oh!

XN137 May 23, 2025

Choose a reason for hiding this comment

Uh oh!

XN137 May 23, 2025

Choose a reason for hiding this comment

Uh oh!

XN137 May 23, 2025

Choose a reason for hiding this comment

Uh oh!

eric-maynard May 23, 2025

Choose a reason for hiding this comment

Uh oh!

dimas-b May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adutra May 23, 2025

Choose a reason for hiding this comment

Uh oh!

dimas-b left a comment

Choose a reason for hiding this comment

Uh oh!

dimas-b May 23, 2025

Choose a reason for hiding this comment

Uh oh!

XN137 May 23, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dimas-b May 23, 2025 •

edited

Loading