Model edits and bug fixes

[jdavcs]

Builds on #17897

To do:

• The added type hints in ee06700 exposed a bug that needs to be fixed by adding an implementation for the _serialize method to the WorkflowInvocationMessage class definition (ref) UPADATE: type-ignore for now.

Misc. post-SA20 model edits and bug fixes.
Remove pre-SA20 code (autocommit arg in session, future arg in session and engine)

A note on nullability in the database schema versus optionality in the python app:

The nullability of the field in the database schema is derived from the type hint. So:
foo: Mapped[int] = mapped_column() will add NOT NULL to the table field definition, whereas
foo: Optional[Mapped[int]] = mapped_column() will not. Furthermore, the nullable argument to mapped_column() takes precedence over this derivation. Thus, it is possible to have a mapped attribute that can contain None values, but will require a value when saved to the database, and vice versa. Here's an example (comments indicate nullability in the db definition):

In SQLAlchemy 1.4:

class Foo(Base):
    __tablename__ = "foo"

    id = Column(Integer, primary_key=True)  # NOT NULL (pkey)
    data1 = Column(String)                  # null (because nullable defaults to True)
    data2 = Column(String, nullable=True)   # null
    data3 = Column(String, nullable=False)  # NOT NULL

In SQLAlchemy 2.0:

class Foo(Base):
    __tablename__ = "foo"

    id: Mapped[int] = mapped_column(primary_key=True)               # NOT NULL (pkey)
    data1: Mapped[str]                                              # NOT NULL (derived from type hint)
    data2: Mapped[Optional[str]]                                   # null (derived from type hint)
    data3: Mapped[str] = mapped_column(nullable=True)               # null
    data4: Mapped[Optional[str]] = mapped_column(nullable=True)    # null
    data5: Mapped[str] = mapped_column(nullable=False)              # NOT NULL
    data6: Mapped[Optional[str]] = mapped_column(nullable=False)   # NOT NULL

https://docs.sqlalchemy.org/en/20/orm/declarative_tables.html#mapped-column-derives-the-datatype-and-nullability-from-the-mapped-annotation

How to test the changes?

(Select all options that apply)

• I've included appropriate automated tests.
• This is a refactoring of components with existing test coverage.
• Instructions for manual testing are as follows:
  1. [add testing steps and prerequisites here if you didn't write automated tests covering all your changes]

License

• I agree to license these and all my past contributions to the core galaxy codebase under the MIT license.

[jmchilton]

This is hard to manually inspect - any chance you generated a schema for our model before and after and checked the diff was empty or made sense where there are differences?

[jmchilton]

We have to fix these column definitions. This is just a wild line of code.

[jmchilton]

Please merge #17902 and follow up with a proper type definitions for that column. The truth is it can be any JSON serializable type - it is sort of up to workflow module to process it - so maybe Any or maybe start with bool and create a comment somewhere that it will need to be unioned with other types if we add more workflow module types. I'm not sure if conditionals use this or not - probably worth looking into but maybe the typing alone would tell us.

[jdavcs]

Will do.

[jdavcs]

This is hard to manually inspect - any chance you generated a schema for our model before and after and checked the diff was empty or made sense where there are differences?

It is a pain to review indeed! (and thanks!) I intentionally reviewed these model edits multiple times. I didn't run a script, but I can do that; it won't be a full model comparison (I tried doing that when moving to declarative - it's next to impossible to cover all model details), but I can compare field types and nullability - that's what's most relevant here. I'll do that before merging, to be on the safe side.

[jdavcs]

I've compared the model definitions on dev and this branch programmatically, comparing column name, type and nullability. There is no difference.

from galaxy.model import mapping
for t in mapping.metadata.tables.values():
    for c in t.columns:
        print(f"{c.name} {c.type} {c.nullable}")
# run the above on both branches, then diff the output.