Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,21 @@
# Use an official Python runtime as the base image
FROM python:3.11-slim

# Set the working directory in the container
WORKDIR /app

# Copy the current directory contents into the container at /app
COPY . /app
Comment on lines +7 to +8

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider optimizing file copying.

While copying the entire current directory is simple, it might include unnecessary files (e.g., .git, __pycache__, temporary files) in the container. This can increase the image size and potentially expose sensitive information.

Consider using a .dockerignore file to exclude unnecessary files and directories. Also, you could copy only the essential files explicitly. For example:

-COPY . /app
+COPY requirements.txt /app/
+COPY app.py /app/
+# Add other necessary files/directories

This approach keeps the container lean and reduces potential security risks.

Committable suggestion was skipped due to low confidence.


# Install any needed packages specified in requirements.txt
RUN pip install --no-cache-dir -r requirements.txt

# Expose port 5000 for Flask to run on
EXPOSE 5000

# Define the environment variable for Flask
ENV FLASK_APP=app.py
ENV FLASK_RUN_HOST=0.0.0.0

# Run the Flask application
CMD ["flask", "run"]
Comment on lines +20 to +21

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using ENTRYPOINT with CMD for more flexibility.

While the current CMD instruction works, using ENTRYPOINT with CMD can provide more flexibility and clarity.

Consider updating the run command as follows:

-CMD ["flask", "run"]
+ENTRYPOINT ["flask"]
+CMD ["run", "--host=0.0.0.0"]

This change offers several benefits:

  1. It clearly separates the main command (ENTRYPOINT) from its default arguments (CMD).
  2. It allows easier overriding of arguments when running the container.
  3. The --host=0.0.0.0 argument in CMD ensures consistency with the FLASK_RUN_HOST environment variable.

This approach maintains the current functionality while improving flexibility for different runtime scenarios.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
# Run the Flask application
CMD ["flask", "run"]
# Run the Flask application
ENTRYPOINT ["flask"]
CMD ["run", "--host=0.0.0.0"]

68 changes: 67 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,76 @@ Your task is to implement simple CRUD API using a simple in-memory data structur
- _title_ - book's title (string, **required**)
- _author_ - book's auther name (string, **required**)
- _price_ - book's price (int, **required**)
- _category_ - book's category (_array_ of _strings_, **required**)
- _category_ - book's category (_array_ of _strings_, **required**)
- _publication_year_ - date and year of the book's publicaiton (date, **required**)


- Requests to non-existing endpoints (e.g. _/some-non/existing/resource_) should be handled.
- Internal server errors should be handled and processed correctly.
- Make sure the api is accesible by frontend apps hosted on a different domain (cross-site resource sharing)


---
---

# Simple Doc for Flask App [Version 1]

A lightweight Flask application that runs entirely in-memory, no database required! This app provides basic book management functionality with RESTful API routes.

## Features:
- Add, update, delete, and fetch books.
- Runs in-memory (no database required).
- Dockerized for easy deployment.

## Getting Started:

### Prerequisites:
- [Python 3.8+](https://www.python.org/downloads/)
- [Docker](https://www.docker.com/)

### Run Locally:

1. **Clone the repository**
```bash
git clone https://github.com/Yohannes90/flask-crud-challenge.git
cd flask-crud-challenge
```

2. **Install dependencies**
```bash
pip install -r requirements.txt
```

3. **Run the app**
```bash
python -m flask --app app run
```

4. **Access the API**
- Open your browser or Postman and visit: `http://127.0.0.1:5000/books`

### Run with Docker:

1. **Build the Docker image**
```bash
docker build -t flask-app .
```

2. **Run the Docker container**
```bash
docker run -p 5000:5000 flask-app
```

3. **Access the API**
- Open your browser or Postman and visit: `http://localhost:5000/books`

---

## API Endpoints:
- **GET** `/books` - Retrieve all books.
- **GET** `/books/:id` - Retrieve a specific book by ID.
- **POST** `/books` - Add a new book.
- **PUT** `/books/:id` - Update an existing book by ID.
- **DELETE** `/books/:id` - Delete a book by ID.

---
17 changes: 17 additions & 0 deletions app.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
from flask import Flask
from flask_cors import CORS
from routes import book_routes
from config import DevelopmentConfig


app = Flask(__name__)
CORS(app)

# Register the blueprints
app.register_blueprint(book_routes)

app.config.from_object(DevelopmentConfig)


if __name__ == "__main__":
app.run()
3 changes: 3 additions & 0 deletions config.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
class DevelopmentConfig:
DEBUG = True
CORS_HEADERS = 'Content-Type'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codebase verification

Incomplete CORS Configuration Detected

The current CORS setup only specifies the Content-Type header and lacks additional configurations needed for full CORS support.

  • config.py
    • CORS_HEADERS = 'Content-Type'
  • app.py
    • from flask_cors import CORS
    • CORS(app)

Recommendation: Consider adding CORS_ORIGINS, CORS_METHODS, and other relevant settings to ensure comprehensive CORS support as outlined in the PR objectives.

🔗 Analysis chain

Review CORS configuration for completeness.

The current CORS configuration only specifies the Content-Type header. Depending on your application's needs, you might want to consider additional CORS settings such as CORS_ORIGINS, CORS_METHODS, etc., to ensure full CORS support as mentioned in the PR objectives.

To check if there are any CORS-related configurations elsewhere in the project, you can run the following script:


Update on CORS Configuration Verification

The previous search encountered an issue with recognizing Python files. I've adjusted the search to target .py files specifically to ensure all CORS-related configurations are identified.


Retrying CORS Configuration Search with Correct rg Syntax

Previously, the search for CORS-related configurations failed due to incorrect usage of the rg command. The updated script below correctly utilizes the --glob flag to target .py files.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Search for CORS-related configurations in Python files

# Test: Look for CORS-related imports and configurations
rg --type python -i '(from flask_cors import CORS|CORS\(|cors_origins|cors_headers|cors_methods)'

Length of output: 130


Script:

#!/bin/bash
# Description: Search for CORS-related configurations in Python files by targeting .py extensions

rg -i '(from flask_cors import CORS|CORS\(|cors_origins|cors_headers|cors_methods)' -- '*.py'

Length of output: 140


Script:

#!/bin/bash
# Description: Search for CORS-related configurations in Python files using correct glob pattern

rg -i '(from flask_cors import CORS|CORS\(|cors_origins|cors_headers|cors_methods)' --glob '*.py'

Length of output: 195

11 changes: 11 additions & 0 deletions models.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
from uuid import uuid4


class Book:
def __init__(self, title, author, price, category, publication_year):
self.id = str(uuid4())
self.title = title
self.author = author
self.price = price
self.category = category
self.publication_year = publication_year
Comment on lines +4 to +11

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider adding type hints, docstring, and using Decimal for price.

The Book class implementation is correct and follows good practices. However, consider the following improvements:

  1. Add type hints to improve code readability and catch potential type-related errors:
from decimal import Decimal
from uuid import uuid4

class Book:
    def __init__(self, title: str, author: str, price: Decimal, category: str, publication_year: int):
        self.id: str = str(uuid4())
        self.title: str = title
        self.author: str = author
        self.price: Decimal = price
        self.category: str = category
        self.publication_year: int = publication_year
  1. Add a docstring to provide more information about the class and its attributes:
class Book:
    """
    Represents a book in the inventory.

    Attributes:
        id (str): Unique identifier for the book.
        title (str): The title of the book.
        author (str): The author of the book.
        price (Decimal): The price of the book.
        category (str): The category or genre of the book.
        publication_year (int): The year the book was published.
    """
  1. Consider using Decimal for the price attribute to ensure precise financial calculations:
from decimal import Decimal

# In the __init__ method:
self.price: Decimal = Decimal(price)

These changes will improve code readability, maintainability, and potentially prevent issues related to financial calculations.

3 changes: 3 additions & 0 deletions requirements.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
Flask==3.0.3
Flask-Cors==5.0.0
Werkzeug==3.0.4
Comment on lines +1 to +3

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Consider using version ranges and adding production/development dependencies

The current requirements.txt file correctly includes the necessary packages for implementing a Flask-based CRUD API with CORS support. However, consider the following suggestions to improve maintainability and completeness:

  1. Use version ranges instead of pinning exact versions. This allows for automatic security updates while maintaining compatibility. For example:

    Flask>=3.0.3,<4.0
    Flask-Cors>=5.0.0,<6.0
    Werkzeug>=3.0.4,<4.0
    
  2. Add production dependencies that might be needed for deployment, such as:

    gunicorn  # For WSGI HTTP server
    
  3. Consider separating development dependencies into a requirements-dev.txt file, including packages for testing, linting, etc. For example:

    pytest
    flake8
    black
    

Would you like me to provide a complete example of separated production and development requirement files?

97 changes: 97 additions & 0 deletions routes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
from flask import Blueprint, jsonify, request
from uuid import uuid4
from validators import validate_book


book_routes = Blueprint('book_routes', __name__)

# In-memory data structure
books = []


@book_routes.route('/books', methods=['GET'])
def get_all_books():
return jsonify(books), 200


@book_routes.route('/books/<string:book_id>', methods=['GET'])
def get_book_by_id(book_id):
for book in books:
if book['id'] == book_id:
return jsonify(book), 200

return jsonify({"error": "Book not found"}), 404
Comment on lines +19 to +23

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor repetitive book lookup code into a helper function

The logic for finding a book by ID is repeated in multiple endpoints (get_book_by_id, update_book_by_id, delete_book_by_id). Refactoring this logic into a helper function will improve code maintainability and reduce duplication.

Create a helper function find_book_by_id(book_id):

def find_book_by_id(book_id):
    for book in books:
        if book['id'] == book_id:
            return book
    return None

Update your endpoints to use the helper function:

In get_book_by_id:

 def get_book_by_id(book_id):
-    for book in books:
-        if book['id'] == book_id:
-            return jsonify(book), 200
-
-    return jsonify({"error": "Book not found"}), 404
+    book = find_book_by_id(book_id)
+    if book:
+        return jsonify(book), 200
+    else:
+        return jsonify({"error": "Book not found"}), 404

In update_book_by_id:

 def update_book_by_id(book_id):
     data = request.get_json()
     # ... [validation code] ...

-    for book in books:
-        if book['id'] == book_id:
-            data.pop('id', None)
-            book.update(data)
-            return jsonify(book), 200
-
-    return jsonify({"error": "Book not found"}), 404
+    book = find_book_by_id(book_id)
+    if book:
+        data.pop('id', None)
+        book.update(data)
+        return jsonify(book), 200
+    else:
+        return jsonify({"error": "Book not found"}), 404

In delete_book_by_id:

 def delete_book_by_id(book_id):
-    for book in books:
-        if book['id'] == book_id:
-            books.remove(book)
-            return jsonify({"message": "Book deleted"}), 200
-
-    return jsonify({"error": "Book not found"}), 404
+    book = find_book_by_id(book_id)
+    if book:
+        books.remove(book)
+        return jsonify({"message": "Book deleted"}), 200
+    else:
+        return jsonify({"error": "Book not found"}), 404

Also applies to: 61-67, 71-77



@book_routes.route('/books', methods=['POST'])
def add_book():
data = request.get_json()

if not data:
return jsonify({"error": "No data provided."}), 400

validation_error = validate_book(data)
if validation_error:
return jsonify(validation_error), validation_error[1]

new_book = {
'id': str(uuid4()),
'title': data['title'],
'author': data['author'],
'price': data['price'],
'category': data['category'],
'publication_year': data['publication_year']
}
Comment on lines +37 to +44

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Ensure user-provided 'id' is not used when adding a new book

Currently, if the user provides an 'id' field in the request data when adding a new book, it will be ignored because a new UUID is generated. However, it's good practice to explicitly remove or ignore the 'id' field from the user input to prevent any confusion or potential misuse.

Consider removing the 'id' key from data before creating new_book:

 def add_book():
     data = request.get_json()

     if not data:
         return jsonify({"error": "No data provided."}), 400

     validation_error = validate_book(data)
     if validation_error:
         return jsonify(validation_error), validation_error[1]

+    data.pop('id', None)  # Remove 'id' if present in data

     new_book = {
         'id': str(uuid4()),
         'title': data['title'],
         'author': data['author'],
         'price': data['price'],
         'category': data['category'],
         'publication_year': data['publication_year']
     }

Committable suggestion was skipped due to low confidence.

books.append(new_book)

return jsonify(new_book), 201


@book_routes.route('/books/<string:book_id>', methods=['PUT'])
def update_book_by_id(book_id):
data = request.get_json()

if not data:
return jsonify({"error": "No data provided."}), 400

validation_error = validate_book(data)
if validation_error:
return jsonify(validation_error), validation_error[1]

for book in books:
if book['id'] == book_id:
book.update(data)
return jsonify(book), 200
Comment on lines +61 to +64

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ Potential issue

Prevent 'id' field from being modified during update

When updating a book, if the request data contains an 'id' field, it could overwrite the existing book's 'id', leading to inconsistencies and potential errors in the data structure.

To prevent this, remove the 'id' from data before updating the book:

 def update_book_by_id(book_id):
     data = request.get_json()

     if not data:
         return jsonify({"error": "No data provided."}), 400

     validation_error = validate_book(data)
     if validation_error:
         return jsonify(validation_error), validation_error[1]

     for book in books:
         if book['id'] == book_id:
+            data.pop('id', None)  # Remove 'id' if present in data
             book.update(data)
             return jsonify(book), 200
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for book in books:
if book['id'] == book_id:
book.update(data)
return jsonify(book), 200
for book in books:
if book['id'] == book_id:
data.pop('id', None) # Remove 'id' if present in data
book.update(data)
return jsonify(book), 200


return jsonify({"error": "Book not found"}), 404


@book_routes.route('/books/<string:book_id>', methods=['DELETE'])
def delete_book_by_id(book_id):
for book in books:
if book['id'] == book_id:
books.remove(book)
return jsonify({"message": "Book deleted"}), 200

return jsonify({"error": "Book not found"}), 404


@book_routes.errorhandler(404)
def not_found(error):
return jsonify({"error": "Resource not found"}), 404


@book_routes.errorhandler(400)
def bad_request(error):
return jsonify({"error": "Bad Request", "message": str(error)}), 400


@book_routes.errorhandler(500)
def internal_error(error):
return jsonify({"error": "An internal error occurred"}), 500






30 changes: 30 additions & 0 deletions validators.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
from datetime import datetime


# Validate the book data
def validate_book(data):
required_fields = ['title', 'author', 'price', 'category', 'publication_year']
for field in required_fields:
if field not in data:
return {"error": f"'{field}' is a required field."}, 400

if not isinstance(data['title'], str):
return {"error": "'title' must be a string."}, 400

if not isinstance(data['author'], str):
return {"error": "'author' must be a string."}, 400

if not isinstance(data['price'], int):
return {"error": "'price' must be an integer."}, 400

if not isinstance(data['category'], list) or not all(isinstance(cat, str) for cat in data['category']):
return {"error": "'category' must be an array of strings."}, 400

try:
pub_year = int(data['publication_year'])
if pub_year > datetime.now().year:
return {"error": "'publication_year' cannot be in the future."}, 400
except ValueError:
return {"error": "'publication_year' must be a valid year (integer)."}, 400

return None