diff --git a/alembic.ini b/alembic.ini index 1663bc40..6ebab39b 100644 --- a/alembic.ini +++ b/alembic.ini @@ -38,19 +38,15 @@ script_location = alembic # are written from script.py.mako # output_encoding = utf-8 -sqlalchemy.url = sqlite:///./runestone_dev.db - - [post_write_hooks] # post_write_hooks defines scripts or Python functions that are run # on newly generated revision scripts. See the documentation for further # detail and examples # format using "black" - use the console_scripts runner, against the "black" entrypoint -# hooks=black -# black.type=console_scripts -# black.entrypoint=black -# black.options=-l 79 +hooks=black +black.type=console_scripts +black.entrypoint=black # Logging configuration [loggers] diff --git a/alembic/env.py b/alembic/env.py index 09b66afa..fa3f844f 100644 --- a/alembic/env.py +++ b/alembic/env.py @@ -1,7 +1,7 @@ # ********************************* # |docname| - Alembic configuration # ********************************* -# :index:`docs to write`: Better description... +# Set up Alembic for migrating only tables managed by this server. It does not migrate tables managed by the web2py admin interface. # # Imports # ======= @@ -10,17 +10,17 @@ # Standard library # ---------------- from logging.config import fileConfig +from textwrap import dedent # Third-party imports # ------------------- -from sqlalchemy import engine_from_config -from sqlalchemy import pool - from alembic import context +from sqlalchemy import create_engine # Local application imports # ------------------------- -from app import models +from bookserver import models +from bookserver.config import settings # Configuration @@ -35,17 +35,141 @@ # add your model's MetaData object here # for 'autogenerate' support -# from myapp import mymodel -# target_metadata = mymodel.Base.metadata - -target_metadata = models.metadata +target_metadata = models.Base.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. +# Use the non-async flavor of the provided database URL. +dburl = settings.database_url.replace("+asyncpg", "").replace("+aiosqlite", "") +print(f"Using DBURL of {dburl}.\n") +# Compute tables not to migrate +# ----------------------------- +# We want to include all tables not managed by web2py. To get this list, compute (all tables in a working Runestone Server instance) - (all tables in a working Bookserver instance). To gather these lists of tables, the query is: +## SELECT input_table_name AS truncate_query FROM(SELECT table_name AS input_table_name FROM information_schema.tables WHERE table_schema NOT IN ('pg_catalog', 'information_schema') AND table_schema NOT LIKE 'pg_toast%') AS information order by input_table_name; +# +# The data below was copied and pasted from psql output of these queries. +web2py_tables = dedent( + """ + acerror_log + alembic_version + assignment_questions + assignments + auth_cas + auth_event + auth_group + auth_membership + auth_permission + auth_user + chapters + clickablearea_answers + code + codelens_answers + competency + course_attributes + course_instructor + course_lti_map + course_practice + courses + dragndrop_answers + editor_basecourse + fitb_answers + grades + invoice_request + lp_answers + lti_keys + mchoice_answers + parsons_answers + payments + practice_grades + question_grades + question_tags + questions + scheduler_run + scheduler_task + scheduler_task_deps + scheduler_worker + selected_questions + shortanswer_answers + source_code + sub_chapter_taught + sub_chapters + tags + timed_exam + unittest_answers + useinfo + user_biography + user_chapter_progress + user_courses + user_experiment + user_state + user_sub_chapter_progress + user_topic_practice + user_topic_practice_completion + user_topic_practice_feedback + user_topic_practice_log + user_topic_practice_survey + web2py_session_exam_runestone + web2py_session_runestone +""" +).split() + + +bookserver_tables = dedent( + """ + assignment_questions + assignments + auth_user + chapters + clickablearea_answers + code + codelens_answers + competency + course_attributes + course_instructor + course_lti_map + courses + dragndrop_answers + fitb_answers + grades + lp_answers + lti_keys + mchoice_answers + parsons_answers + payments + question_grades + questions + selected_questions + shortanswer_answers + source_code + sub_chapters + timed_exam + unittest_answers + useinfo + user_chapter_progress + user_courses + user_experiment + user_sub_chapter_progress +""" +).split() + + +web2py_only_tables = set(web2py_tables) - set(bookserver_tables) + + +# Ignore tables used only by the admin interface. +def include_name(name, type_, parent_names): + if type_ == "table": + return name not in web2py_only_tables + else: + return True + + +# Define and run migrations +# ========================= def run_migrations_offline(): """Run migrations in 'offline' mode. @@ -58,12 +182,13 @@ def run_migrations_offline(): script output. """ - url = config.get_main_option("sqlalchemy.url") + url = dburl context.configure( url=url, target_metadata=target_metadata, literal_binds=True, dialect_opts={"paramstyle": "named"}, + include_name=include_name, ) with context.begin_transaction(): @@ -77,14 +202,13 @@ def run_migrations_online(): and associate a connection with the context. """ - connectable = engine_from_config( - config.get_section(config.config_ini_section), - prefix="sqlalchemy.", - poolclass=pool.NullPool, - ) - + connectable = create_engine(dburl) with connectable.connect() as connection: - context.configure(connection=connection, target_metadata=target_metadata) + context.configure( + connection=connection, + target_metadata=target_metadata, + include_name=include_name, + ) with context.begin_transaction(): context.run_migrations() diff --git a/alembic/rename_indices.sql b/alembic/rename_indices.sql new file mode 100644 index 00000000..74a8d865 --- /dev/null +++ b/alembic/rename_indices.sql @@ -0,0 +1,81 @@ +-- ********* +-- |docname| +-- ********* +-- This should be run before generating the first web2py to BookServer migration. +-- +-- TODO: +-- +-- - Should we allow xxx_answers.correct to be NULL? What about xxx_answers.answer? (For example, mchoice_answers.correct or mchoice_answers.answer)? + + + +-- For now, just test these; roll them back at the end. +begin; + +-- Rename indices from web2py / rsmanage to sqlalchemy's convention. +alter index course_id_index rename to ix_useinfo_course_id; +alter index div_id_index rename to ix_useinfo_div_id; +alter index event_index rename to ix_useinfo_event; +alter index sid_index rename to ix_useinfo_sid; +alter index timestamp_idx rename to ix_useinfo_timestamp; +alter index assign_course_idx rename to ix_assignments_course; +alter index chapters_course_id_idx rename to ix_chapters_course_id; +alter index code_acid_idx rename to ix_code_acid; +alter index code_sid_idx rename to ix_code_sid; +alter index code_timestamp_idx rename to ix_code_timestamp; +alter index mchoice_answers_div_id_idx rename to ix_mchoice_answers_div_id; +alter index mchoice_answers_sid_idx rename to ix_mchoice_answers_sid; +alter index parsons_answers_div_id_idx rename to ix_parsons_answers_div_id; +alter index parsons_answers_sid_idx rename to ix_parsons_answers_sid; +alter index q_bc_idx rename to ix_questions_base_course; +alter index questions_chapter_idx rename to ix_questions_chapter; +alter index questions_name_idx rename to ix_questions_name; +alter index source_code_acid_idx rename to ix_source_code_acid; +alter index source_code_course_id_idx rename to ix_source_code_course_id; +alter index sub_chapters_chapter_id_idx rename to ix_sub_chapters_chapter_id; +alter index subchap_idx rename to ix_questions_subchapter; +alter index unittest_answers_div_id_idx rename to ix_unittest_answers_div_id; +alter index unittest_answers_sid_idx rename to ix_unittest_answers_sid; +alter index user_sub_chapter_progress_chapter_id_idx rename to ix_user_sub_chapter_progress_chapter_id; +alter index user_sub_chapter_progress_user_id_idx rename to ix_user_sub_chapter_progress_sub_chapter_id; +alter index chap_label_idx rename to ix_sub_chapters_sub_chapter_label; +alter index code_course_id_idx rename to ix_code_course_id; +alter index mchoice_answers_course_name_idx rename to ix_mchoice_answers_course_name; +alter index parsons_answers_course_name_idx rename to ix_parsons_answers_course_name; +alter index unittest_answers_course_name_idx rename to ix_unittest_answers_course_name; +alter index user_sub_chapter_progress_course_name_idx rename to ix_user_sub_chapter_progress_course_name; +alter index unique_user rename to ix_auth_user_username; +alter index courses_course_name_idx rename to courses_course_name_key; + + +-- Provide reasonable default values where those were lacking. +update assignment_questions set activities_required=0 where activities_required is null; +update assignments set released='F' where released is null; +update assignments set visible='F' where visible is null; +update assignments set from_source='F' where from_source is null; +update auth_user set donated='F' where donated is null; +update auth_user set accept_tcp='F' where accept_tcp is null; +update chapters set chapter_num=999 where chapter_num is null; +update code set language='python' where language is null; +update courses set institution='' where institution is null; +update courses set downloads_enabled='F' where downloads_enabled is null; +update courses set allow_pairs='F' where allow_pairs is null; +update course_instructor set verified='F' where verified is null; +update course_instructor set paid='F' where paid is null; +update questions set from_source='F' where from_source is null; +update grades set manual_total='F' where manual_total is null; +update sub_chapters set sub_chapter_num=999 where sub_chapter_num is null; + +-- Fix old JS producing NULL, probably when clicking on "check me" before providing an answer. +update mchoice_answers set correct='F' where correct is null; +update mchoice_answers set answer='' where answer is null; +update fitb_answers set correct='F' where correct is null; +update fitb_answers set answer='' where answer is null; + + + +-- Delete junk from the database. +delete from useinfo where div_id is null and course_id is null; +delete from courses where base_course is null; + +rollback; diff --git a/alembic/toctree.rst b/alembic/toctree.rst index ccb1a8f9..291763ee 100644 --- a/alembic/toctree.rst +++ b/alembic/toctree.rst @@ -5,6 +5,8 @@ Database migration using Alembic .. toctree:: :maxdepth: 1 + :glob: ../alembic.ini env.py + versions/*.py diff --git a/alembic/versions/.gitignore b/alembic/versions/.gitignore deleted file mode 100644 index 92746295..00000000 --- a/alembic/versions/.gitignore +++ /dev/null @@ -1,7 +0,0 @@ -# ************************************************** -# |docname| - Git ignore for Alembic-generated files -# ************************************************** -# Ignore everything in this directory, since it's generated by Alembic and can (I think) be regenerated from data in the database. -* -# Don't ignore this file. -!.gitignore diff --git a/bookserver/db.py b/bookserver/db.py index f4188f5a..b09971bc 100644 --- a/bookserver/db.py +++ b/bookserver/db.py @@ -17,6 +17,7 @@ from sqlalchemy.ext.asyncio import AsyncSession from sqlalchemy.ext.asyncio import create_async_engine from sqlalchemy.orm import declarative_base, sessionmaker +from sqlalchemy.sql import select # Local application imports # ------------------------- @@ -52,6 +53,35 @@ async def init_models(): await conn.run_sync(Base.metadata.drop_all) await conn.run_sync(Base.metadata.create_all) + await check_not_null() + + +# Look for any records that violate non-null constraints. +async def check_not_null(): + print("Searching for NOT NULL constraint violations..."), + not_null_count = 0 + async with async_session() as session: + for table_name, table in Base.metadata.tables.items(): + for column in table.columns: + if not column.nullable: + # SQLAlchemy requires ``==`` to correctly create the query; it can't overload the ``is`` operator. + query = select(table).where(column == None) # noqa: E711. + res = (await session.execute(query)).fetchall() + if res: + not_null_count += 1 + print( + f"Column {table_name}.{column.key} has {len(res)} NULL records, such as:" + ) + for row in res[0:9]: + + def shorten(s): + s = str(s) + return s if len(s) < 20 else s[0:20] + "..." + + # The result isn't an ORM object, so use this to display it. + s = ", ".join(f"{k}={shorten(row[k])}" for k in row.keys()) + print(f" {s}") + print(f"Done; found {not_null_count} columns with constraint violations.") # If the engine isn't disposed of, then a PostgreSQL database will remain in a pseudo-locked state, refusing to drop of truncate tables (see `bookserver_session`). diff --git a/bookserver/models.py b/bookserver/models.py index 89e4ca3f..f9330fb1 100644 --- a/bookserver/models.py +++ b/bookserver/models.py @@ -41,7 +41,6 @@ String, Date, DateTime, - MetaData, Text, types, Float, @@ -62,6 +61,8 @@ class Web2PyBoolean(types.TypeDecorator): impl = types.CHAR(1) python_type = bool + # From the `docs `_: "The requirements for cacheable elements is that they are hashable and also that they indicate the same SQL rendered for expressions using this type every time for a given cache value." + cache_ok = True def process_bind_param(self, value, dialect): if value: @@ -89,10 +90,6 @@ def copy(self, **kw): # Schema Definition # ================= -# this object is a container for the table objects and can be used by alembic to autogenerate -# the migration information. -metadata = MetaData() - # Provide a container to store information about each type of Runestone Component. While a namedtuple would be better, this can't be used since the fields aren't modifiable after creation; see the comment on `init_graders `. class RunestoneComponentDict: @@ -136,6 +133,8 @@ class IdMixin: # User info logged by the `log_book_event endpoint`. See there for more info. class Useinfo(Base, IdMixin): __tablename__ = "useinfo" + __table_args__ = (Index("sid_divid_idx", "sid", "div_id"),) + # _`timestamp`: when this entry was recorded by this webapp. timestamp = Column(DateTime, index=True, nullable=False) # _`sid`: TODO: The student id? (user) which produced this row. @@ -160,10 +159,7 @@ class Useinfo(Base, IdMixin): # Answers to specific question types # ---------------------------------- -# Many of the tables containing answers are always accessed by sid, div_id and course_name. Provide this as a default query. class AnswerMixin(IdMixin): - # TODO: these entries duplicate Useinfo.timestamp. Why not just have a timestamp_id field? - # # See timestamp_. timestamp = Column(DateTime, nullable=False) # See div_id_. @@ -174,7 +170,9 @@ class AnswerMixin(IdMixin): # See course_name_. Mixins with foreign keys need `special treatment `_. @declared_attr def course_name(cls): - return Column(String(512), ForeignKey("courses.course_name"), nullable=False) + return Column( + String(512), ForeignKey("courses.course_name"), index=True, nullable=False + ) def to_dict(self): return {c.key: getattr(self, c.key) for c in inspect(self).mapper.column_attrs} @@ -209,7 +207,14 @@ class MchoiceAnswers(Base, CorrectAnswerMixin): __tablename__ = "mchoice_answers" # _`answer`: The answer to this question. TODO: what is the format? answer = Column(String(50), nullable=False) - __table_args__ = (Index("idx_div_sid_course_mc", "sid", "div_id", "course_name"),) + __table_args__ = ( + Index( + "mult_scd_idx", + "div_id", + "course_name", + "sid", + ), + ) # An answer to a fill-in-the-blank question. @@ -227,6 +232,7 @@ class DragndropAnswers(Base, CorrectAnswerMixin): __tablename__ = "dragndrop_answers" # See answer_. TODO: what is the format? answer = Column(String(512), nullable=False) + min_height = Column(String(512), nullable=False) __table_args__ = (Index("idx_div_sid_course_dd", "sid", "div_id", "course_name"),) @@ -247,7 +253,7 @@ class ParsonsAnswers(Base, CorrectAnswerMixin): answer = Column(String(512), nullable=False) # _`source`: The source code provided by a student? TODO. source = Column(String(512), nullable=False) - __table_args__ = (Index("idx_div_sid_course_pp", "sid", "div_id", "course_name"),) + __table_args__ = (Index("parsons_scd_idx", "div_id", "course_name", "sid"),) # An answer to a Code Lens problem. @@ -306,11 +312,11 @@ class Code(Base, IdMixin): index=True, nullable=False, ) # unique identifier for a component - course_id = Column(Integer, index=False, nullable=False) + course_id = Column(Integer, index=True, nullable=False) code = Column(Text, index=False, nullable=False) - language = Column(Text, index=False, nullable=False) - emessage = Column(Text, index=False) - comment = Column(Text, index=False) + language = Column(Text, nullable=False) + emessage = Column(Text, nullable=False) + comment = Column(Text) # Used for datafiles and storing questions and their suffix separately. @@ -341,13 +347,14 @@ class Courses(Base, IdMixin): # _`course_name`: The name of this course. course_name = Column(String(512), unique=True, nullable=False) term_start_date = Column(Date, nullable=False) + institution = Column(String(512), nullable=False) # TODO: Why not use base_course_id instead? _`base_course`: the course from which this course was derived. TODO: If this is a base course, this field should be identical to the course_name_? base_course = Column(String(512), ForeignKey("courses.course_name"), nullable=False) - # TODO: This should go in a different table. Not all courses have a Python/Skuplt component. login_required = Column(Web2PyBoolean, nullable=False) allow_pairs = Column(Web2PyBoolean, nullable=False) student_price = Column(Integer) downloads_enabled = Column(Web2PyBoolean, nullable=False) + # Earlier courses didn't have this specified, so allow these old records to remain that way. New records should always specify this value. courselevel = Column(String, nullable=False) institution = Column(String) @@ -359,7 +366,7 @@ class Courses(Base, IdMixin): # ------------------------------ class AuthUser(Base, IdMixin): __tablename__ = "auth_user" - username = Column(String(512), nullable=False, unique=True) + username = Column(String(512), index=True, nullable=False, unique=True) first_name = Column(String(512), nullable=False) last_name = Column(String(512), nullable=False) email = Column(String(512), unique=True, nullable=False) @@ -397,6 +404,8 @@ def username_clear_of_css_characters(cls, v): class CourseInstructor(Base, IdMixin): __tablename__ = "course_instructor" + __table_args__ = (Index("c_i_idx", "course", "instructor"),) + course = Column(Integer, ForeignKey("courses.id"), nullable=False) instructor = Column(Integer, ForeignKey("auth_user.id"), nullable=False) verified = Column(Web2PyBoolean, nullable=False) @@ -472,7 +481,7 @@ class Assignment(Base, IdMixin): threshold_pct = Column(Float(53)) allow_self_autograde = Column(Web2PyBoolean) is_timed = Column(Web2PyBoolean) - time_limit = Column(Integer, nullable=False) + time_limit = Column(Integer) from_source = Column(Web2PyBoolean, nullable=False) nofeedback = Column(Web2PyBoolean) nopause = Column(Web2PyBoolean) @@ -511,37 +520,42 @@ class QuestionGrade(Base, IdMixin): __tablename__ = "question_grades" __table_args__ = ( Index( - "question_grades_sid_course_name_div_id_idx", - "sid", - "course_name", + "question_grades_key", "div_id", - unique=True, + "course_name", + "sid", ), ) sid = Column(String(512), nullable=False) course_name = Column(String(512), nullable=False) div_id = Column(String(512), nullable=False) - score = Column(Float(53), nullable=False) + # Manually-graded questions may be unscored (a NULL score). + score = Column(Float(53)) comment = Column(Text, nullable=False) - deadline = Column(DateTime, nullable=False) - answer_id = Column(Integer, nullable=False) + deadline = Column(DateTime) + # Grades before the improved autograded and manually-scored grades lack this. Since it can refer to an ID from many different tables, don't make it a constraint. + answer_id = Column(Integer) # The Grade table holds the grade for an entire assignment class Grade(Base, IdMixin): __tablename__ = "grades" - __table_args__ = (UniqueConstraint("auth_user", "assignment"),) + __table_args__ = ( + UniqueConstraint("auth_user", "assignment"), + Index("user_assign_unique_idx", "auth_user", "assignment"), + ) auth_user = Column(ForeignKey("auth_user.id", ondelete="CASCADE"), nullable=False) assignment = Column( ForeignKey("assignments.id", ondelete="CASCADE"), nullable=False ) - score = Column(Float(53), nullable=False) + # If all questions in the assignment don't have a score, this won't either. + score = Column(Float(53)) manual_total = Column(Web2PyBoolean, nullable=False) - projected = Column(Float(53), nullable=False) - lis_result_sourcedid = Column(String(1024), nullable=False) - lis_outcome_url = Column(String(1024), nullable=False) + # Not all grades will be reportable via LTI. + lis_result_sourcedid = Column(String(1024)) + lis_outcome_url = Column(String(1024)) # Book Structure Tables @@ -562,7 +576,7 @@ class SubChapter(Base, IdMixin): chapter_id = Column( ForeignKey("chapters.id", ondelete="CASCADE"), index=True, nullable=False ) - sub_chapter_label = Column(String(512), nullable=False) + sub_chapter_label = Column(String(512), index=True, nullable=False) skipreading = Column(Web2PyBoolean, nullable=False) sub_chapter_num = Column(Integer, nullable=False) @@ -575,10 +589,12 @@ class UserSubChapterProgress(Base, IdMixin): user_id = Column(ForeignKey("auth_user.id", ondelete="CASCADE"), index=True) chapter_id = Column(String(512), index=True, nullable=False) sub_chapter_id = Column(String(512), index=True, nullable=False) + # Initial values for this don't have dates. start_date = Column(DateTime, nullable=False) end_date = Column(DateTime) status = Column(Integer, nullable=False) - course_name = Column(String(512), nullable=False) + # Older courses lack this; all newer courses should have one. + course_name = Column(String(512), index=True) UserSubChapterProgressValidator = sqlalchemy_to_pydantic(UserSubChapterProgress) @@ -589,6 +605,7 @@ class UserChapterProgress(Base, IdMixin): user_id = Column(String(512), nullable=False) chapter_id = Column(String(512), nullable=False) + # Initial values for this don't have dates. start_date = Column(DateTime, nullable=False) end_date = Column(DateTime) status = Column(Integer, nullable=False) @@ -629,6 +646,7 @@ class UserExperiment(Base, IdMixin): class SelectedQuestion(Base, IdMixin): __tablename__ = "selected_questions" + __table_args__ = (Index("selector_sid_unique", "selector_id", "sid"),) selector_id = Column(String(512), nullable=False) sid = Column(String(512), nullable=False) @@ -642,6 +660,7 @@ class SelectedQuestion(Base, IdMixin): class Competency(Base, IdMixin): __tablename__ = "competency" + __table_args__ = (Index("q_comp_unique", "question", "competency"),) question = Column(ForeignKey("questions.id", ondelete="CASCADE"), nullable=False) competency = Column(String(512), nullable=False) diff --git a/bookserver/schemas.py b/bookserver/schemas.py index deea8e05..ef033e0a 100644 --- a/bookserver/schemas.py +++ b/bookserver/schemas.py @@ -112,6 +112,8 @@ class LogItemIncoming(BaseModelNone): subchapter: Optional[str] # used by parsons source: Optional[str] + # used by dnd + min_height: Optional[int] # used by unittest passed: Optional[int] failed: Optional[int] diff --git a/conf.py b/conf.py index 9c57161b..157e363f 100644 --- a/conf.py +++ b/conf.py @@ -194,8 +194,6 @@ "sphinx-enki-info.txt", # TODO: Notes here. I assume this is produced by the coverage run in the test suite? A link to that point in the code would be nice. "test/htmlcov", - # `Alembic `_ stores auto-generated migration scripts `here `_. - "alembic/versions", ] # `default_role `_: The diff --git a/docs/requirements-rtd.txt b/docs/requirements-rtd.txt index 46f904c7..bd570b4b 100644 --- a/docs/requirements-rtd.txt +++ b/docs/requirements-rtd.txt @@ -4,4 +4,5 @@ # This is a ``pip`` requirements file used to install prerequisites for ReadTheDocs builds. CodeChat -Sphinx \ No newline at end of file +Sphinx + diff --git a/test/conftest.py b/test/conftest.py index 1f55be17..4493d350 100644 --- a/test/conftest.py +++ b/test/conftest.py @@ -413,6 +413,7 @@ async def test_course_1(create_test_course): return await create_test_course( course_name="test_child_course_1", term_start_date=datetime.datetime(2000, 1, 1), + institution="Test U", login_required=True, base_course="test_course_1", allow_pairs=True,