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

Sphinx-Weixi_He, Phoenix- Madina Dzhetegenova #67

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 8 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
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -56,4 +56,4 @@ For Waves 04, 05, and 06, the new driver may need to setup or update their datab
1. Activate the virtual environment
1. Create the database `solar_system_development`
1. Run `flask db upgrade`
1. Run `flask run` to confirm that the API is running as expected
1. Run `flask run` to confirm that the API is running as expected
13 changes: 12 additions & 1 deletion app/__init__.py
Original file line number Diff line number Diff line change
@@ -1,9 +1,20 @@
from flask import Flask
from .db import db, migrate
from .models import planet
Copy link
Contributor

Choose a reason for hiding this comment

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

Note that once we have the Planet model imported in the routes, and the routes are imported here, we don't technically need to import the planet module here. If you find the fact that VS Code shows this as not being used, it would now be safe to remove.

from .routes.routes import solar_system_bp
import os

def create_app():
def create_app(config=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 We need this param to accept the additional options for testing, but it needs to be defaulted since nothing will be passed in when starting with flask run

app = Flask(__name__)

app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False
app.config['SQLALCHEMY_DATABASE_URI'] = os.environ.get('SQLALCHEMY_DATABASE_URI')
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Loads the connection string from the environment (provided with our .env during development).


if config:
app.config.update(config)
db.init_app(app)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Leave a blank line between the end of the conditional block and this logic.

migrate.init_app(app, db)

app.register_blueprint(solar_system_bp)

return app
6 changes: 6 additions & 0 deletions app/db.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
from flask_sqlalchemy import SQLAlchemy
from flask_migrate import Migrate
from .models.base import Base

db = SQLAlchemy(model_class=Base)
migrate = Migrate()
4 changes: 4 additions & 0 deletions app/models/base.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
from sqlalchemy.orm import DeclarativeBase

class Base(DeclarativeBase):
pass
39 changes: 23 additions & 16 deletions app/models/planet.py
Original file line number Diff line number Diff line change
@@ -1,19 +1,26 @@
from sqlalchemy.orm import Mapped, mapped_column
from ..db import db

class Planet:
def __init__(self, id, name, description, flag= bool):
self.id = id
self.name = name
self.description = description
self.flag = flag
class Planet(db.Model):
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice modifications to associate this model type with our database.

id: Mapped[int] = mapped_column(primary_key=True, autoincrement=True)
name: Mapped[str]
description: Mapped[str]
flag:Mapped[bool]
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Leave a space after the : in type expressions.

    flag: Mapped[bool]


# class Planet:
Copy link
Contributor

Choose a reason for hiding this comment

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

It can be tempting to keep old code around for reference, but keep in mind that with git, we can always get back to previous versions if necessary. So in general, prefer to remove old code, or at least put very clear comments around why this code is here and commented out.

Even with git, it can be inconvenient to find a previous version that had our original approach. To help with that, we can create a side branch on a commit that still has the old code, so that we can find it more easily, then remove the code from the main branch.

# def __init__(self, id, name, description, flag= False):
# self.id = id
# self.name = name
# self.description = description
# self.flag = flag

planets = [
Planet(1, "Mercury", "The smallest planet in our solar system.", False),
Planet(2, "Venus", "The hottest planet in the solar system.", False),
Planet(3, "Earth", "The only planet known to support life.", True ),
Planet(4, "Mars", "Known as the Red Planet.", True ),
Planet(5, "Jupiter", "The largest planet in our solar system.", False),
Planet(6, "Saturn", "Famous for its ring system.", False),
Planet(7, "Uranus", "An ice giant with a unique tilt.", False),
Planet(8, "Neptune", "The farthest planet from the Sun.", False)
]
# planets = [
# Planet(1, "Mercury", "The smallest planet in our solar system.", False),
# Planet(2, "Venus", "The hottest planet in the solar system.", False),
# Planet(3, "Earth", "The only planet known to support life.", True ),
# Planet(4, "Mars", "Known as the Red Planet.", True ),
# Planet(5, "Jupiter", "The largest planet in our solar system.", False),
# Planet(6, "Saturn", "Famous for its ring system.", False),
# Planet(7, "Uranus", "An ice giant with a unique tilt.", False),
# Planet(8, "Neptune", "The farthest planet from the Sun.", False)
# ]
152 changes: 129 additions & 23 deletions app/routes/routes.py
Copy link
Contributor

Choose a reason for hiding this comment

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

When moving the routes file into a folder (theoretically, to group multiple routes files together as we add additional resources), prefer to include the resource name as part of the file name. So here, something like planets_routes.py or planet_routes.py.

Original file line number Diff line number Diff line change
@@ -1,42 +1,148 @@
from flask import Blueprint, abort, make_response
from ..models.planet import planets
from flask import Blueprint, abort, make_response, request, Response
from ..models.planet import Planet
from ..db import db

solar_system_bp = Blueprint("solar_system_bp", __name__, url_prefix="/planets")
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer to use the resource name (rather than the project name) as part of the blueprint name. So here, planets_bp, since this blueprint is related to the planets resources.


@solar_system_bp.post("")
def create_planet():
request_body = request.get_json()
name = request_body["name"]
description = request_body["description"]
flag = request_body["flag"]
Comment on lines +10 to +12
Copy link
Contributor

Choose a reason for hiding this comment

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

We didn't explicitly look at invalid/missing data scenarios for creating a record, but think about what would happen here, and what response we might want to reply with.


new_planet = Planet(name=name, description=description, flag=flag)
db.session.add(new_planet)
db.session.commit()

response = {
"id": new_planet.id,
"name": new_planet.name,
"description": new_planet.description,
"flag": new_planet.flag
}
return response, 201

@solar_system_bp.get("")
def get_all_planets():
results_list = []
for planet in planets:
results_list.append(dict(
id=planet.id,
name=planet.name,
description=planet.description,
flag=planet.flag
))
query = db.select(Planet)

description_param = request.args.get("description")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice filter parameter behavior.

if description_param:
query = query.where(Planet.description.ilike(f"%{description_param}%"))


flag_param = request.args.get("flag")
if flag_param is not None:
# Convert flag_param to a boolean
flag_value = flag_param.lower() == "true"
query = query.where(Planet.flag == flag_value)
Comment on lines +36 to +39
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice custom logic for the boolean flag value. Note that we could still have done the regular truthy/falsy style test (even the literal string "false" would still be considered truthy). Then to convert to an actual boolean, we did need to compare against a string rather than casting using the bool() function.


return results_list
sort_param = request.args.get("sort")
if sort_param == "name":
Copy link
Contributor

Choose a reason for hiding this comment

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

A conditional can be used here, but since the logic here is figuring out how to map from a string (the specific column name) to an actual model column, we could either store this relationship in a dictionary for easy lookup, or we could look into using the python getattr function to retrieve the model column by name directly.

For the dictionary approach, consider

NAME_TO_COLUMN = {
    "name": Planet.name,
    "description": Planet.description,
    ...  # and so forth
}

query = query.order_by(NAME_TO_COLUMN.get(sort_param, Planet.id))

getattr is part of Python's "introspection" features. For most any code of the form a.b, we can rewrite that as getattr(a, "b"), in other words, we can access an attribute by string name. Feel free to dig into that further to think about how that could be useful here.

query = query.order_by(Planet.name)
elif sort_param == "description":
query = query.order_by(Planet.description)
elif sort_param == "flag":
query = query.order_by(Planet.flag)
else:
# Default sorting by id
query = query.order_by(Planet.id)

planets = db.session.scalars(query.order_by(Planet.id))
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need the order_by clause here any more, since that's all been attached in the previous sort logic.


planet_response = []
for planet in planets:
planet_response.append(
{
"id": planet.id,
"name": planet.name,
"description": planet.description,
"flag": planet.flag
}
)
return planet_response

@solar_system_bp.get("/<planet_id>")
def get_one_planet(planet_id):
planet = validate_planet(planet_id)

return{
"id":planet.id,
"name":planet.name,
"description":planet.description,
"flag":planet.flag
return {
"id": planet.id,
"name": planet.name,
"description": planet.description,
"flag": planet.flag
}

def validate_planet(planet_id):
try:
planet_id = int(planet_id)
except:
response = {"message": f"planet {planet_id} invalid"}
abort(make_response(response, 400))
abort(make_response(response , 400))
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Watch out for extra spaces being iontroduced into existing code.

        abort(make_response(response, 400))


for planet in planets:
if planet.id == planet_id:
return planet
query = db.select(Planet).where(Planet.id == planet_id)
planet = db.session.scalar(query)

if not planet:
response = {"message": f"planet {planet_id} not found"}
abort(make_response(response, 404))
return planet

@solar_system_bp.put("/<planet_id>")
def update_planet(planet_id):
planet = validate_planet(planet_id)
request_body = request.get_json()

planet.name= request_body["name"]
planet.description = request_body["description"]
planet.flag = request_body["flag"]
Comment on lines +97 to +99
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar problems as for creating a record could happen here as well. What might we want to check for, and how might we respond?

db.session.commit()

return Response(status=204, mimetype="application/json")
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 This is how we say there's no content in the response, but even still, we should keep our type consistently as json for all our endpoints.


@solar_system_bp.delete("/<planet_id>")
def delete_planet(planet_id):
planet = validate_planet(planet_id)
db.session.delete(planet)
db.session.commit()

return Response(status=204, mimetype="application/json")

# @solar_system_bp.get("")
# def get_all_planets():
# results_list = []
# for planet in planets:
# results_list.append(dict(
# id=planet.id,
# name=planet.name,
# description=planet.description,
# flag=planet.flag
# ))

# return results_list

# @solar_system_bp.get("/<planet_id>")
# def get_one_planet(planet_id):
# planet = validate_planet(planet_id)

# return{
# "id":planet.id,
# "name":planet.name,
# "description":planet.description,
# "flag":planet.flag
# }

# def validate_planet(planet_id):
# try:
# planet_id = int(planet_id)
# except:
# response = {"message": f"planet {planet_id} invalid"}
# abort(make_response(response, 400))

# for planet in planets:
# if planet.id == planet_id:
# return planet

response = {"message": f"planet {planet_id} not found"}
abort(make_response(response, 404))
# response = {"message": f"planet {planet_id} not found"}
# abort(make_response(response, 404))
1 change: 1 addition & 0 deletions migrations/README
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Single-database configuration for Flask.
50 changes: 50 additions & 0 deletions migrations/alembic.ini
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
# A generic, single database configuration.

[alembic]
# template used to generate migration files
# file_template = %%(rev)s_%%(slug)s

# set to 'true' to run the environment during
# the 'revision' command, regardless of autogenerate
# revision_environment = false


# Logging configuration
[loggers]
keys = root,sqlalchemy,alembic,flask_migrate

[handlers]
keys = console

[formatters]
keys = generic

[logger_root]
level = WARN
handlers = console
qualname =

[logger_sqlalchemy]
level = WARN
handlers =
qualname = sqlalchemy.engine

[logger_alembic]
level = INFO
handlers =
qualname = alembic

[logger_flask_migrate]
level = INFO
handlers =
qualname = flask_migrate

[handler_console]
class = StreamHandler
args = (sys.stderr,)
level = NOTSET
formatter = generic

[formatter_generic]
format = %(levelname)-5.5s [%(name)s] %(message)s
datefmt = %H:%M:%S
Loading