Skip to content

fix: preserve async TLS verification default - #7642

Open
mikemikimike wants to merge 4 commits into
chroma-core:mainfrom
mikemikimike:fix/async-http-tls-verification
Open

fix: preserve async TLS verification default#7642
mikemikimike wants to merge 4 commits into
chroma-core:mainfrom
mikemikimike:fix/async-http-tls-verification

Conversation

@mikemikimike

Copy link
Copy Markdown

Summary

AsyncFastAPI previously converted the default None value of chroma_server_ssl_verify to verify=False, disabling TLS certificate verification. This makes the async client match the synchronous client's behavior while preserving explicit boolean and CA-path settings.

Testing

  • pytest -q chromadb/test/test_client.py -k async_fastapi_passes_ssl_verify --tb=short
  • python -m black --check chromadb/api/async_fastapi.py chromadb/test/test_client.py
  • git diff --check

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@github-actions

Copy link
Copy Markdown

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

@Mahnoor-Zaffar

Copy link
Copy Markdown

Reviewed: chromadb/api/async_fastapi.py, chromadb/test/test_client.py. CI: mergeability_check success.

  • The change in async_fastapi.py correctly preserves the behavior of the chroma_server_ssl_verify setting. The conditional logic for setting the verify parameter is clear and maintains expected functionality.
  • The new test test_async_fastapi_passes_ssl_verify_only_when_configured in test_client.py effectively covers various scenarios for the ssl_verify parameter, ensuring that the behavior aligns with the intended changes.
  • Consider adding a test case for an invalid CA path to further validate the robustness of the SSL verification handling, as this could lead to runtime errors if not properly managed.

Overall, the implementation looks solid and the tests appear comprehensive. Nice work on aligning the async client behavior with the synchronous one!

@mikemikimike

Copy link
Copy Markdown
Author

Addressed the review suggestion in 008dd29. Added a regression test that uses a missing CA path, verifies httpx raises FileNotFoundError, and confirms no partially initialized AsyncClient remains in the shared cache. Validation: pytest -q chromadb/test/test_client.py (20 passed), Black passed, Flake8 passed, and git diff --check passed.

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.

2 participants