Skip to content

Fix several edge-case bugs and strengthen tests - #227

Draft
sophiecarreras wants to merge 1 commit into
masterfrom
review/java-fixes-and-tests
Draft

sophiecarreras wants to merge 1 commit into
masterfrom
review/java-fixes-and-tests

Conversation

@sophiecarreras

Copy link
Copy Markdown
Contributor

Fixes seven edge-case bugs found in a review of master, and adds or strengthens tests. Each fix has a test.

Fixes

  • B2AccountAuthorizationCache: no longer caches an authorization for a different account after the account-id mismatch check fails.
  • B2WebApiHttpClientImpl.getContent: honors the timeouts configured on the HttpClientFactory. Adds a default HttpClientFactory.getRequestConfig(), overridden by HttpClientFactoryImpl.
  • B2LargeFileStorer: part index and start byte come from the sorted part list, so progress events are correct for out-of-order parts.
  • B2CancellableContentSource: the wrapped stream is closed even when close() reports the cancellation.
  • B2EventNotificationRule: isSuspended() and compareTo() no longer throw NullPointerException when suspension fields are not provided.
  • B2InputStreamExcerpt.skip(): no longer advances the position twice.
  • JSON handlers for LocalDate, ConcurrentMap and EnumSet: throw B2JsonException for invalid dates and null elements instead of unchecked exceptions.

Tests

  • New tests for each fix.
  • Stronger tests for the finish-large-file request payload, content writer sha1 handling, ranged content sources, unverified sha1 headers, JSON field handling and BigInteger, and the resume path.
  • Error-path download tests now fail if getContent does not throw.

Notes for reviewers

  • HttpClientFactory gains a default method; custom factories keep the default timeouts unless they override it.
  • Verified with ./gradlew test (core 703, httpclient 4, 0 failures) and check_code. The fail-before proofs were run on the individual changes earlier, not re-run on this combined branch.
  • The findings came from an automated review and have not been confirmed by a maintainer yet; please treat the fixes as proposals.

🤖 Generated with Claude Code

Fixes:
- B2AccountAuthorizationCache: do not cache an authorization for a different
  account after the account-id mismatch check fails.
- B2WebApiHttpClientImpl.getContent: honor the timeouts configured on the
  HttpClientFactory (adds default HttpClientFactory.getRequestConfig()).
- B2LargeFileStorer: compute part index and start byte from the sorted part
  list so progress events are right for out-of-order parts.
- B2CancellableContentSource: close the wrapped stream even when close()
  reports the cancellation.
- B2EventNotificationRule: isSuspended() and compareTo() no longer throw
  NullPointerException when suspension fields are not provided.
- B2InputStreamExcerpt.skip(): do not advance the position twice.
- B2Json LocalDate/ConcurrentMap/EnumSet handlers: throw B2JsonException for
  invalid dates and null elements instead of unchecked exceptions.

Tests: new tests for each fix, plus stronger tests for the finish-large-file
request payload, content writer sha1 handling, ranged content sources,
unverified sha1 headers, JSON field handling and BigInteger, resume, and
error-path download tests that could never fail.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@sophiecarreras

Copy link
Copy Markdown
Contributor Author

Review request from Sofia (drafted with Claude). Nothing in this review has been checked by a human yet; the numbers come from an agent review of whole main. Please verify, in this order:

  1. For each new or changed test, revert the production line it covers (or the named fix) and confirm the test fails. A test that still passes is a finding.
  2. No production behavior changed beyond what the description says.
  3. The decision or check listed for this PR below.
    Reply on the linked issue if a finding is a real bug, not a bug, or already known.

This PR: nothing open besides the review. CI is green. Ready for sign-off.

@sophiecarreras

Copy link
Copy Markdown
Contributor Author

@jeronimodeleon: you are the sign-off for this PR once Felipe, Gonzalo and Eduardo have reviewed it. GitHub would not let me add you as a reviewer (you have read access on this repo), so this is your request. What to confirm is in the comment above.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant