search

parkpow / deep-license-plate-recognition

Automatic License Plate Recognition (ALPR) or Automatic Number Plate Recognition (ANPR) software that works with any camera.
https://platerecognizer.com/
MIT License
523 stars 122 forks source link

Forward Verkada LPR Webhooks to ParkPow Visits #197

Closed danleyb2 closed 6 months ago

danleyb2 commented 6 months ago

https://trello.com/c/7FsGpiYS/1624-integration-verkada

github-actions[bot] commented 6 months ago

Risk Level 3 - /home/runner/work/deep-license-plate-recognition/deep-license-plate-recognition/parkpow/webhook-verkada/main.py

The code changes introduce several new features and classes, which increases the complexity of the system. There are a few areas that could be improved for better error handling, code readability, and maintainability:

  1. Exception Handling: The except blocks catch general exceptions and log them, but it might be more useful to handle different types of exceptions differently. For example, network-related exceptions could trigger a retry, while others might not.

  2. Magic Numbers: The code uses the number 5 as a magic number in the while loop for retries. It would be better to define this as a constant at the top of the file to make it clear what it represents and to make it easier to change if needed.

  3. Hardcoded URLs: The URLs for the API endpoints are hardcoded. It would be better to define these as constants or configuration settings to make the code more flexible and easier to maintain.

  4. Error Handling in log_vehicle: The method log_vehicle raises a generic Exception with the message \"Error logging vehicle\". It would be more informative to include the actual error message from the response in the exception.

  5. Base64 Encoding: The method download_image returns the base64 encoded image, which is not immediately clear from the method name. Consider renaming the method to reflect this or changing the behavior to return the raw image data.

  6. Thread Safety: The WebhookQueue class uses a Queue, which is thread-safe, but it's not clear if other parts of the class are thread-safe. Ensure that all shared resources are properly synchronized.

  7. Logging Level: The logging level is set based on an environment variable, which is good for configurability. However, ensure that the default level is appropriate for production use if the environment variable is not set.

  8. Content-Type Validation: The do_POST method in RequestHandler checks the content type but writes b\"OK\" and continues processing even if the content type is not application/json. This could lead to unexpected behavior. It should return an error response instead.

Here's an example of how to define a constant for retries:

MAX_RETRIES = 5

# ... later in the code ...

while tries < MAX_RETRIES:
    # ... retry logic ...

And an example of improved exception handling in log_vehicle:

except requests.RequestException as e:
    error_message = f\"Error logging vehicle: {response.text}\"
    lgr.error(error_message, exc_info=e)
    raise Exception(error_message)

🔍🛠️🚦

Powered by Code Review GPT

marcbelmont commented 6 months ago

@danleyb2 FYI, I've converted the PR to "Draft pull request".