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
6 changes: 5 additions & 1 deletion app/__init__.py
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

It looks like the updates to connect to the database didn't make it in here. Maybe it was misplaced with the GH confusion? In addition to registering the routes, things that we'd want to see in this file include:

  1. Initialize the database and migration systems
  2. Load connection string
  3. Load models (though if we load a route file that itself loads the models, we don't need to do this here)

Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
from flask import Flask



def create_app(test_config=None):
app = Flask(__name__)

return app
from .routes import bp
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

app.register_blueprint(bp)

return app
Empty file added app/models/__init__.py
Empty file.
24 changes: 24 additions & 0 deletions app/models/planet.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
from app import db

class Planet(db.Model):
id = db.Column(db.Integer, primary_key=True, autoincrement=True)
name = db.Column(db.String, nullable=False)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 Nice decision to mark these columns as not nullable.

description = db.Column(db.String, nullable=False)
rating = db.Column(db.Integer, nullable=False)

@classmethod
def from_dict(cls, data_dict):
return cls(
id=data_dict["id"],
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👀 For at least the scenario we're using from_dict now (POST create route), we would not set the id explicitly. We want the database to set this when we commit the model record.

name=data_dict["name"],
description=data_dict["description"],
rating=data_dict["rating"]
)

def to_dictionary(self):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👀 As modelled in Learn (and how your code is calling it) this should be named to_dict.

return dict(
id=self.id,
name=self.name,
description=self.description,
rating=self.rating
)
2 changes: 0 additions & 2 deletions app/routes.py

This file was deleted.

Empty file added app/routes/__init__.py
Empty file.
69 changes: 69 additions & 0 deletions app/routes/planet_routes.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,69 @@
from flask import Blueprint, jsonify, abort, make_response, request
from app import db
from app.models.planet import Planet
from .routes_helpers import validate_model


bp = Blueprint("planets", __name__, url_prefix="/planets")
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍


#GET ALL ENDPOINT
@bp.route("", methods=["GET"])
def get_all_planets():

rating_param = request.args.get("rating")

planet_query = Planet.query

if rating_param:
planet_query = planet_query.filter_by(rating=rating_param)
Comment on lines +15 to +18
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 Great job making use of this extension pattern!


planets_list = [planet.to_dict() for planet in planet_query]

return jsonify(planets_list)

# CREATE ENDPOINT
@bp.route("", methods=["POST"])
def create_planet():
request_body=request.get_json()

new_planet = Planet.from_dict(request_body)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Note that our from_dict could raise KeyErrors if the body was missing any required value. Consider making this call in a try/except to handle that without crashing (we could return a 400 Bad Request).


db.session.add(new_planet)
db.session.commit()

return make_response(f"Planet {new_planet.name} successfully created!", 201)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👀 Note that this is still returning a bare HTML result here. At the least, we should consistently return JSON from all our endpoints (use jsonify, or pack the message into aq dict like we do in the validation helper).

But here, we might also decide to return the created planet record, as this would give the caller access to the id that was created, as well as any default values they may not have included in their request.


# GET ONE ENDPOINT
@bp.route("/<id>", methods=["GET"])
def handle_planet(id):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

planet = validate_model(Planet, id)

return jsonify(planet.to_dict()), 200

# UPDATE ONE ENDPOINT
@bp.route("/<id>", methods=["PUT"])
def update_planet(id):
planet = validate_model(Planet, id)
request_body = request.get_json()

planet.id = request_body["id"]
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👀 We should not attempt to change the id. Once the id has been assigned by the database during the create, we should leave it unchanged as long as the data remains in the DB.

planet.name = request_body["name"]
planet.description = request_body["description"]
planet.rating = request_body["rating"]
Comment on lines +50 to +52
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

As with my suggestion for the POST endpoint, we may want to wrap this with a try/expect to handle any KeyError that occurs.


db.session.commit()

return make_response(f"Planet {planet.name} successfully updated", 200)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👀 At the least, this should jsonify the result so that all our endpoints return JSON. However, as with the POST endpoint, we might want to return the updated record.


# DELETE ONE ENDPOINT
@bp.route("/<id>", methods=["DELETE"])
def delete_planet(id):
planet = validate_model(Planet, id)

db.session.delete(planet)
db.session.commit()

return make_response(f"Planet {planet.name} successfully deleted", 200)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👀 At the least, this should jsonify the result so that all our endpoints return JSON. However, we might consider having no response body at all, since the 200 status itself should be enough to tell the caller that the delete occurred successfully.




13 changes: 13 additions & 0 deletions app/routes/routes_helpers.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
from flask import abort, make_response

#HELPER FUNCTIONS
def validate_model(cls, id):
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👀 This helper should return the model if it was able to be looked up.

try:
id = int(id)
except:
abort(make_response({"message": f"{id} was invalid"}, 400))

model = cls.query.get(id)

if not model:
abort(make_response({"message": f"{cls.__name__} with id {id} was not found"}, 404))
1 change: 1 addition & 0 deletions migrations/README
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Generic single-database configuration.
45 changes: 45 additions & 0 deletions migrations/alembic.ini
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
# 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

[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

[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
97 changes: 97 additions & 0 deletions migrations/env.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
from __future__ import with_statement

import logging
from logging.config import fileConfig

from sqlalchemy import engine_from_config
from sqlalchemy import pool
from flask import current_app

from alembic import context

# this is the Alembic Config object, which provides
# access to the values within the .ini file in use.
config = context.config

# Interpret the config file for Python logging.
# This line sets up loggers basically.
fileConfig(config.config_file_name)
logger = logging.getLogger('alembic.env')

# add your model's MetaData object here
# for 'autogenerate' support
# from myapp import mymodel
# target_metadata = mymodel.Base.metadata
config.set_main_option(
'sqlalchemy.url',
str(current_app.extensions['migrate'].db.engine.url).replace('%', '%%'))
target_metadata = current_app.extensions['migrate'].db.metadata

# other values from the config, defined by the needs of env.py,
# can be acquired:
# my_important_option = config.get_main_option("my_important_option")
# ... etc.


def run_migrations_offline():
"""Run migrations in 'offline' mode.

This configures the context with just a URL
and not an Engine, though an Engine is acceptable
here as well. By skipping the Engine creation
we don't even need a DBAPI to be available.

Calls to context.execute() here emit the given string to the
script output.

"""
url = config.get_main_option("sqlalchemy.url")
context.configure(
url=url, target_metadata=target_metadata, literal_binds=True
)

with context.begin_transaction():
context.run_migrations()


def run_migrations_online():
"""Run migrations in 'online' mode.

In this scenario we need to create an Engine
and associate a connection with the context.

"""

# this callback is used to prevent an auto-migration from being generated
# when there are no changes to the schema
# reference: http://alembic.zzzcomputing.com/en/latest/cookbook.html
def process_revision_directives(context, revision, directives):
if getattr(config.cmd_opts, 'autogenerate', False):
script = directives[0]
if script.upgrade_ops.is_empty():
directives[:] = []
logger.info('No changes in schema detected.')

connectable = engine_from_config(
config.get_section(config.config_ini_section),
prefix='sqlalchemy.',
poolclass=pool.NullPool,
)

with connectable.connect() as connection:
context.configure(
connection=connection,
target_metadata=target_metadata,
process_revision_directives=process_revision_directives,
**current_app.extensions['migrate'].configure_args
)

with context.begin_transaction():
context.run_migrations()


if context.is_offline_mode():
run_migrations_offline()
else:
run_migrations_online()

24 changes: 24 additions & 0 deletions migrations/script.py.mako
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
"""${message}

Revision ID: ${up_revision}
Revises: ${down_revision | comma,n}
Create Date: ${create_date}

"""
from alembic import op
import sqlalchemy as sa
${imports if imports else ""}

# revision identifiers, used by Alembic.
revision = ${repr(up_revision)}
down_revision = ${repr(down_revision)}
branch_labels = ${repr(branch_labels)}
depends_on = ${repr(depends_on)}


def upgrade():
${upgrades if upgrades else "pass"}


def downgrade():
${downgrades if downgrades else "pass"}
34 changes: 34 additions & 0 deletions migrations/versions/5857683fef96_.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
"""empty message
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👀 We should use -m "message" to add a message when creating a migration to help us remember what the migration file does (this is more important when we have multiple migrations).


Revision ID: 5857683fef96
Revises:
Create Date: 2023-04-28 11:08:47.978712

"""
from alembic import op
import sqlalchemy as sa


# revision identifiers, used by Alembic.
revision = '5857683fef96'
down_revision = None
branch_labels = None
depends_on = None


def upgrade():
# ### commands auto generated by Alembic - please adjust! ###
op.create_table('cat',
sa.Column('id', sa.Integer(), autoincrement=True, nullable=False),
sa.Column('name', sa.String(), nullable=True),
sa.Column('color', sa.String(), nullable=True),
sa.Column('personality', sa.String(), nullable=True),
sa.PrimaryKeyConstraint('id')
)
# ### end Alembic commands ###


def downgrade():
# ### commands auto generated by Alembic - please adjust! ###
op.drop_table('cat')
# ### end Alembic commands ###
11 changes: 11 additions & 0 deletions seed.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
from app import create_app, db
from app.models.planet import Planet

my_app = create_app()
with my_app.app_context():
db.session.add(Planet(id="1", name="Apple Planet", description="A planet that only grows apples.", rating="5"))
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 Nice use of a "seed" file to give yourself a quick way to reload development data if necessary.

👀 But you should leave off the id value, and leave it to the database to assign. The way we've defined the model primary key isn't as strict as what we did in the SQL mini-unti, so this technically works, but generally we should leave it to the database to assign the primary key.

db.session.add(Planet(id="2", name="Orange Planet", description="A planet that only grows oranges.", rating="3"))
db.session.add(Planet(id="3", name="Pear Planet", description="A planet that only grows pears.", rating="1"))
db.session.commit()


Empty file added tests/__init__.py
Empty file.
38 changes: 38 additions & 0 deletions tests/conftest.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
import pytest
from app import create_app
from app import db
from flask.signals import request_finished
from app.models.planet import Planet


@pytest.fixture
def app():
app = create_app({"TESTING": True})

@request_finished.connect_via(app)
def expire_session(sender, response, **extra):
db.session.remove()

with app.app_context():
db.create_all()
yield app

with app.app_context():
db.drop_all()


@pytest.fixture
def client(app):
return app.test_client()

@pytest.fixture
def one_planet(app):
planet = Planet(
id="id",
name="name",
description="description",
rating="rating"
)
Comment on lines +30 to +35
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Leave off the id here (notice also, that the type doesn't mathc the column), and make sure to provide a valid value for rating (the column is declared to be an integer, so we should use a number here).

    planet = Planet(
        name="name",
        description="description",
        rating=5,
    )

db.session.add(planet)
db.session.commit()
return planet
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍 Returning the record from the fixture will make it available to access for any tests using the fixture.

Loading