-
Notifications
You must be signed in to change notification settings - Fork 61
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
Sea Turtles - Georgia A & Lili P #9
base: main
Are you sure you want to change the base?
Changes from all commits
b7ceb35
bb3c58a
c0d5705
29710d3
691f864
982ece0
cef35b2
189cfea
ce5c4ef
224dcac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,29 @@ | ||
from flask import Flask | ||
from flask_sqlalchemy import SQLAlchemy | ||
from flask_migrate import Migrate | ||
from dotenv import load_dotenv | ||
import os | ||
|
||
db = SQLAlchemy() | ||
migrate = Migrate() | ||
load_dotenv() | ||
|
||
def create_app(test_config=None): | ||
app = Flask(__name__) | ||
|
||
app.config['SQLALCHEMY_TRACK_MODIFICATIONS'] = False | ||
|
||
if not test_config: | ||
app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get("SQLALCHEMY_DATABASE_URI") | ||
else: | ||
app.config["TESTING"] = True | ||
app.config["SQLALCHEMY_DATABASE_URI"] = os.environ.get("SQLALCHEMY_TEST_DATABASE_URI") | ||
|
||
db.init_app(app) | ||
migrate.init_app(app, db) | ||
|
||
from app.routes.routes import planets_bp | ||
app.register_blueprint(planets_bp) | ||
Comment on lines
+25
to
+26
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you renamed things according to my later suggestions, these lines could be re-written as: from app.routes import planet_routes
app.register_blueprint(planet_routes.bp) |
||
|
||
return app | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
from app import db | ||
|
||
class Planet(db.Model): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
id = db.Column(db.Integer, primary_key=True, autoincrement=True) | ||
name = db.Column(db.String, nullable=False) | ||
description = db.Column(db.String) | ||
has_life = db.Column(db.Boolean, nullable=False) | ||
|
||
def to_dict(self): | ||
return dict( | ||
id=self.id, | ||
name=self.name, | ||
description=self.description, | ||
has_life=self.has_life | ||
) |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
from unicodedata import name | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like a stray import from VS Code got in here. Thanks, VS Code! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, since you moved this file into a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't forget to add a |
||
from app import db | ||
from app.models.planet import Planet | ||
from flask import Blueprint, jsonify, make_response, request | ||
from .routes_helper import validate_planet, error_message | ||
|
||
|
||
planets_bp = Blueprint("planets", __name__, url_prefix="/planets") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When a route file has only a single blueprint in it (pretty typical), consider naming the variable simply |
||
|
||
@planets_bp.route("", methods=["POST"]) | ||
def create_planet(): | ||
request_body = request.get_json() | ||
|
||
planet = Planet( | ||
name=request_body["name"], | ||
description=request_body["description"], | ||
has_life=request_body["has_life"]) | ||
Comment on lines
+14
to
+17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This would be great to move into a |
||
|
||
db.session.add(planet) | ||
db.session.commit() | ||
|
||
return jsonify(planet.to_dict()), 201 | ||
|
||
|
||
@planets_bp.route("", methods=["GET"]) | ||
def get_planets(): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
|
||
name_param = request.args.get("name") | ||
|
||
if name_param: | ||
planets = Planet.query.filter_by(name=name_param) | ||
else: | ||
planets = Planet.query.all() | ||
|
||
planets_response = [planet.to_dict() for planet in planets] | ||
|
||
return jsonify(planets_response), 200 | ||
|
||
|
||
@planets_bp.route("/<planet_id>", methods=["GET"]) | ||
def get_planet_by_id(planet_id): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
|
||
planet = validate_planet(planet_id) | ||
|
||
return jsonify(planet.to_dict()), 200 | ||
|
||
@planets_bp.route("/<planet_id>", methods=["PUT"]) | ||
def replace_planet(planet_id): | ||
planet = validate_planet(planet_id) | ||
request_body = request.get_json() | ||
|
||
try: | ||
planet.name = request_body["name"] | ||
planet.description = request_body["description"] | ||
planet.has_life = request_body["has_life"] | ||
Comment on lines
+53
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider moving this to an instance method in |
||
except KeyError as err: | ||
error_message(f"Missing key {err}", 400) | ||
|
||
db.session.commit() | ||
return make_response(f"Planet {planet_id} successfully updated!", 200) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Make sure to return jsonify(f"Planet {planet_id} successfully updated!"), 200 |
||
|
||
@planets_bp.route("/<planet_id>", methods=["DELETE"]) | ||
def delete_planet(planet_id): | ||
planet = validate_planet(planet_id) | ||
|
||
db.session.delete(planet) | ||
db.session.commit() | ||
return make_response(f"Planet {planet_id} successfully deleted.") | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Here, too, make sure to jsonify this. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
from flask import jsonify, make_response, abort | ||
from app.models.planet import Planet | ||
|
||
def error_message(message, status_code): | ||
abort(make_response(message, status_code)) | ||
Comment on lines
+4
to
+5
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It looks like the places where you're using this to return a message, there are some places where you're passing in a full dictionary, and others where you're passing in a stand-alone string. From the naming of the parameter If we pass in a dictionary, Flask will assume we are returning JSON data, and automatically set the proper headers. But if we pass in a string, 1) the structure of the response body won't match the cases where a dictionary is being used, which can be confusing for the client, and 2) Flask assumes a string response is HTML, not JSON, so the response headers will be set to tell the client the result is HTML, which can cause some HTTP client libraries to assume there is no response data (like python's We could address point 2 by always calling def error_message(message, status_code):
abort(make_response(jsonify(message), status_code)) And we could address point 1 by having the function accept a string, and building the dictionary wrapper internally. def error_message(message, status_code):
abort(make_response(jsonify({"message": message}), status_code)) |
||
|
||
def validate_planet(id): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Deciding where to put stuff is an extension of deciding how to name things, which is really hard! |
||
try: | ||
id = int(id) | ||
except: | ||
error_message({"message":f"planet {id} invalid"}, 400) | ||
|
||
planet = Planet.query.get(id) | ||
|
||
if not planet: | ||
error_message({"message":f"planet {id} not found"}, 404) | ||
else: | ||
return planet | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
Generic single-database configuration. |
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 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,96 @@ | ||
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() |
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"} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
"""adds Planet model | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 Nice migration message |
||
|
||
Revision ID: e29e7cf3fb9b | ||
Revises: | ||
Create Date: 2022-04-29 14:14:00.240448 | ||
|
||
""" | ||
from alembic import op | ||
import sqlalchemy as sa | ||
|
||
|
||
# revision identifiers, used by Alembic. | ||
revision = 'e29e7cf3fb9b' | ||
down_revision = None | ||
branch_labels = None | ||
depends_on = None | ||
|
||
|
||
def upgrade(): | ||
# ### commands auto generated by Alembic - please adjust! ### | ||
op.create_table('planet', | ||
sa.Column('id', sa.Integer(), autoincrement=True, nullable=False), | ||
sa.Column('name', sa.String(), nullable=False), | ||
sa.Column('description', sa.String(), nullable=True), | ||
sa.Column('has_life', sa.Boolean(), nullable=False), | ||
sa.PrimaryKeyConstraint('id') | ||
) | ||
# ### end Alembic commands ### | ||
|
||
|
||
def downgrade(): | ||
# ### commands auto generated by Alembic - please adjust! ### | ||
op.drop_table('planet') | ||
# ### end Alembic commands ### |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,34 @@ | ||
from flask.signals import request_finished | ||
from app import db | ||
import pytest | ||
from app.models.planet import Planet | ||
|
||
from app import create_app | ||
|
||
@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 two_planets(app): | ||
earth = Planet(name="earth", description="green and blue", has_life=True) | ||
mercury = Planet(name="mercury", description="extremely hot", has_life=False) | ||
|
||
db.session.add(earth) | ||
db.session.add(mercury) | ||
db.session.commit() | ||
Comment on lines
+27
to
+34
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 The migration logic sets up the id behavior a little differently from how we did it when we worked with SQL manually, and would also allow us to set the ids explicitly. One nice thing about setting the id explicitly in a fixture is that we know what id to use in subsequent tests. If we don't, then we should still be able to assume that the ids will start from 1 and increment, but we probably shouldn't depend on it. So if we leave off the ids, then we'd probably want to return the two records in a list so that the test using these records could get their ids safely, regardless of what they were set to. This would look like: @pytest.fixture
def two_planets(app):
earth = Planet(name="earth", description="green and blue", has_life=True)
mercury = Planet(name="mercury", description="extremely hot", has_life=False)
db.session.add(earth)
db.session.add(mercury)
db.session.commit()
return [earth, mercury] And then in a test using the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍