Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Use 'api_key' parameter if passed explicitly #278

Merged
merged 2 commits into from
May 14, 2024
Merged

Conversation

ashraq1455
Copy link
Collaborator

@ashraq1455 ashraq1455 commented May 14, 2024

PR Type

Enhancement, Bug fix


Description

  • Introduced an instance attribute api_key in PineconeIndex to handle API keys more effectively.
  • Constructor now sets api_key with a direct assignment, allowing explicit passing of the key, and falls back to the environment variable if not provided.
  • Added error handling to raise a ValueError if api_key is not set, ensuring the API key's presence is validated during object initialization.
  • Refactored the _get_all method to utilize the instance's api_key, improving security by avoiding direct environment variable access.

Changes walkthrough 📝

Relevant files
Enhancement
pinecone.py
Enhance API Key Management in PineconeIndex Class               

semantic_router/index/pinecone.py

  • Added an api_key attribute to the PineconeIndex class.
  • Modified the constructor to prioritize the passed api_key over the
    environment variable.
  • Added a check to raise an exception if api_key is not provided.
  • Updated the _get_all method to use the instance's api_key instead of
    directly accessing the environment variable.
  • +8/-5     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    @github-actions github-actions bot added enhancement Enhancement to existing features Bug fix labels May 14, 2024
    Copy link

    PR Description updated to latest commit (261e68a)

    Copy link

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are straightforward and localized to a single class, but require careful consideration of the new logic for API key handling and error management.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Bug: The constructor now uses api_key or os.getenv("PINECONE_API_KEY") which might not correctly handle cases where api_key is an empty string but not None.

    🔒 Security concerns

    No

    Code feedback:
    relevant filesemantic_router/index/pinecone.py
    suggestion      

    Consider explicitly checking for both None and empty strings when assigning api_key in the constructor. This ensures that an empty string passed as an api_key does not fallback to the environment variable, which might be unexpected behavior. [important]

    relevant lineself.api_key = api_key or os.getenv("PINECONE_API_KEY")

    relevant filesemantic_router/index/pinecone.py
    suggestion      

    To enhance security and avoid potential misuse, consider logging a warning if the api_key falls back to the environment variable. This can help in tracing and debugging API key source issues. [medium]

    relevant lineself.api_key = api_key or os.getenv("PINECONE_API_KEY")

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Ensure consistent use of the API key by using the class attribute

    Instead of directly accessing the environment variable in the headers dictionary, use the
    api_key attribute of the class which already handles the fallback to the environment
    variable. This ensures consistency in how the API key is retrieved throughout the class.

    semantic_router/index/pinecone.py [196]

    +headers = {"Api-Key": self.api_key}
     
    -
    Suggestion importance[1-10]: 9

    Why: This suggestion correctly identifies an improvement in the consistency of API key usage across the class by utilizing the already established class attribute, which handles the fallback to the environment variable. This ensures that the API key is managed uniformly throughout the class.

    9
    Use a more specific exception type for missing API key errors

    Consider using a more specific exception type instead of the generic ValueError. Using
    specific exceptions like KeyError or creating a custom exception like MissingAPIKeyError
    can make error handling more precise and informative.

    semantic_router/index/pinecone.py [76]

     if self.api_key is None:
    -    raise ValueError("Pinecone API key is required.")
    +    raise KeyError("Pinecone API key is required.")
     
    Suggestion importance[1-10]: 7

    Why: Changing from a generic ValueError to a more specific KeyError or a custom exception like MissingAPIKeyError can indeed make error handling more precise and informative.

    7
    Security
    Add logging to warn when falling back to environment variable for API key

    To ensure that the api_key parameter is always handled securely, consider logging a
    warning or error if the environment variable fallback is used, encouraging the explicit
    passing of the api_key.

    semantic_router/index/pinecone.py [73]

    -self.api_key = api_key or os.getenv("PINECONE_API_KEY")
    +self.api_key = api_key
    +if self.api_key is None:
    +    self.api_key = os.getenv("PINECONE_API_KEY")
    +    logging.warning("Falling back to environment variable for API key.")
     
    Suggestion importance[1-10]: 8

    Why: Adding a warning log when falling back to an environment variable for the API key is a good security practice, ensuring explicit awareness of how sensitive information is being handled.

    8
    Maintainability
    Simplify the dictionary construction for pinecone_args

    Refactor the initialization of pinecone_args to include the namespace directly in the
    dictionary definition if it exists, simplifying the code and improving readability.

    semantic_router/index/pinecone.py [91-93]

    -pinecone_args = {"api_key": api_key, "source_tag": "semantic-router"}
    -if self.namespace:
    -    pinecone_args["namespace"] = self.namespace
    +pinecone_args = {
    +    "api_key": api_key,
    +    "source_tag": "semantic-router",
    +    "namespace": self.namespace if self.namespace else None
    +}
     
    Suggestion importance[1-10]: 6

    Why: The suggestion to refactor the initialization of pinecone_args to include the namespace directly in the dictionary definition simplifies the code and improves readability.

    6

    @ashraq1455 ashraq1455 changed the title Fix: Use 'api_key' parameter if passed explicitly fix: Use 'api_key' parameter if passed explicitly May 14, 2024
    Copy link

    codecov bot commented May 14, 2024

    Codecov Report

    Attention: Patch coverage is 66.66667% with 2 lines in your changes are missing coverage. Please review.

    Project coverage is 80.99%. Comparing base (423e1a3) to head (1810b24).

    Files Patch % Lines
    semantic_router/index/pinecone.py 66.66% 2 Missing ⚠️
    Additional details and impacted files
    @@           Coverage Diff           @@
    ##             main     #278   +/-   ##
    =======================================
      Coverage   80.98%   80.99%           
    =======================================
      Files          45       45           
      Lines        2672     2673    +1     
    =======================================
    + Hits         2164     2165    +1     
      Misses        508      508           

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @jamescalam jamescalam merged commit 62aca47 into main May 14, 2024
    7 of 9 checks passed
    @jamescalam jamescalam deleted the pc-index-api-key branch May 14, 2024 13:18
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Bug fix enhancement Enhancement to existing features Review effort [1-5]: 2
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    2 participants