Skip to content

Enhance SDK with type safety, error handling and resource management improvements #11

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sujalsalekar
Copy link

fix: Enhance SDK with type safety, error handling and resource management improvements

  • Added proper type hints and validation across core SDK files
  • Improved error handling with specific exceptions and cleanup
  • Enhanced file resource management with proper cleanup
  • Added request timeouts and better HTTP error handling
  • Improved documentation with comprehensive docstrings
  • Fixed potential memory leaks in file operations

@nandit123
Copy link
Member

/describe

Copy link

Title

Enhance SDK with type safety, error handling and resource management improvements


User description

fix: Enhance SDK with type safety, error handling and resource management improvements

  • Added proper type hints and validation across core SDK files
  • Improved error handling with specific exceptions and cleanup
  • Enhanced file resource management with proper cleanup
  • Added request timeouts and better HTTP error handling
  • Improved documentation with comprehensive docstrings
  • Fixed potential memory leaks in file operations

PR Type

Enhancement


Description

  • Added comprehensive type hints and validation across SDK

  • Enhanced error handling with specific exceptions and cleanup

  • Improved resource management with proper file cleanup

  • Added request timeouts and better HTTP error handling


Diagram Walkthrough

flowchart LR
  A["Input Validation"] --> B["Type Safety"]
  B --> C["HTTP Requests"]
  C --> D["Error Handling"]
  D --> E["Resource Cleanup"]
  E --> F["Response Processing"]
Loading

File Walkthrough

Relevant files
Enhancement
__init__.py
Enhanced main SDK interface with validation                           

src/lighthouseweb3/init.py

  • Added input validation for all public methods
  • Enhanced error handling with descriptive exception messages
  • Improved docstrings with parameter and exception documentation
  • Fixed method parameter validation logic
+47/-8   
axios.py
Enhanced HTTP client with type safety                                       

src/lighthouseweb3/functions/axios.py

  • Added comprehensive type hints throughout the class
  • Implemented request timeouts (30 seconds) for all HTTP calls
  • Enhanced error handling with specific exception types
  • Improved resource cleanup in file upload methods
+109/-36
upload.py
Enhanced upload functions with validation                               

src/lighthouseweb3/functions/upload.py

  • Added type hints for all function parameters and returns
  • Enhanced input validation for source and token parameters
  • Improved error handling with contextual error messages
  • Removed debug print statements for cleaner error propagation
+84/-50 
utils.py
Enhanced utility functions with type safety                           

src/lighthouseweb3/functions/utils.py

  • Added comprehensive type hints and docstrings
  • Enhanced input validation for all utility functions
  • Improved error handling with proper exception cleanup
  • Added resource management for file operations
+138/-47

@nandit123
Copy link
Member

/review

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Error Handling

The post_files method catches all exceptions and wraps them in RequestException, but doesn't properly handle the case where files is None before attempting to close files in the finally block.

files = None
try:
    self.parse_url_query(kwargs.get("query"))
    files = utils.read_files_for_upload(file)
    r = req.post(self.url, headers=headers, files=files, timeout=30)  # Added timeout
    r.raise_for_status()

    # Always ensure files are closed
    if files:
        utils.close_files_after_upload(files)

    try:
        return r.json()
    except req.exceptions.JSONDecodeError:
        # Handle special case where response contains multiple lines
        temp = r.text.split("\n")
        return json.loads(temp[-2])  # Use -2 index instead of len(temp) - 2
except Exception as e:
    # Ensure files are closed even if an error occurs
    if files:
        utils.close_files_after_upload(files)
    raise RequestException(f"File upload failed: {str(e)}")
Resource Leak

The read_files_for_upload function opens file handles but doesn't close them if an exception occurs during the loop iteration, potentially causing resource leaks.

try:
    for file in files["files"]:
        if not os.path.exists(file):
            raise ValueError(f"File does not exist: {file}")

        name = (extract_file_name_with_source(file, files["path"]) 
               if files["is_dir"] 
               else extract_file_name(file))

        file_list.append(
            (
                "file",
                (
                    name,
                    open(file, "rb"),
                    "application/octet-stream",
                ),
            )
        )
    return file_list
Inconsistent Error Handling

The exception handling in uploadBlob method checks for read/close methods but doesn't verify write method for BufferedWriter in downloadBlob, creating inconsistent validation.

if not isinstance(source, io.BufferedReader):
    raise TypeError("source must be an instance of io.BufferedReader")

if not filename or not isinstance(filename, str):
    raise ValueError("filename must be a non-empty string")

if not (hasattr(source, 'read') and hasattr(source, 'close')):
    raise TypeError("source must have 'read' and 'close' methods")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants