Outline use of type_annotation_map to fix mypy issues

[jmchilton]

Builds on #17897

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.

[jdavcs]

Suggested change

	Optional[STR_TO_STR_DICT]: JSONType,
	Optional[TRANSFORM_ACTIONS]: MutableJSONType,
	STR_TO_STR_DICT: JSONType,
	TRANSFORM_ACTIONS: MutableJSONType,

We use Optional in the model definition

[jmchilton]

Well my thinking was the thing in map needed to match the column definition completely. If it doesn't have to match the column definition exactly... I think we don't actually need this map right? I am thinking we just drop this if it isn't actually needed. We would have test failures if this was needed and wrong.

[jdavcs]

@jmchilton it works as intended; I've reread the docs and tested it to make sure I'm not misreading stuff. Here's how I read it:

STR_TO_STR_DICT = Dict[str, str]  # we define a type alias
...
STR_TO_STR_DICT: JSONType  
# in the statement above (type annotation map), we say that the Python type STR_TO_STR_DICT 
# should be mapped to SQLAlchemy's JSONType, which is a SQLAlchemy type that knows how to 
# generate its DDL. 
# If we substituted JSONType for Integer, the Python code would still expect Dict[str, str],
# but in the db that field would be defined as an integer type.
...
object_store_id_overrides: Mapped[Optional[STR_TO_STR_DICT]]  # that's all we need here!
# the field will be typed as Optional[Dict[str, str]], represented as bytea in the database. 

The Optional type specifier works fine: here's an example that causes a mypy error; removing Optional fixes it - as expected:

def test(job: Job):
    job.object_store_id_overrides['key']
test(Job())

# lib/galaxy/model/__init__.py:11430: error: Value of type "Optional[dict[str, str]]" is not indexable  [index]

Does this make sense?

(There's a conflict to resolve - I'm happy to take care of this.)

[jmchilton]

Maybe this makes sense - I think given that we don't have a typed way to distinguish between what should be mutable or not currently we should drop the dictionary and keep being explicit but I will defer to you until I understand it better.

Update: I mean like we probably have some string to string dictionaries that are mutable and some that are not right? And regardless - we might want to add some or either variety even if we only have one type currently?

[jdavcs]

Yes, i think so (re. your update). Also, that type annotation map may be useful for other type mappings (SA's docs have some examples here), even if the only benefit in those cases would be more succinct code.

[jdavcs]

Since we've decided to keep HasTable, the second commit (82fd47b) needs to be dropped. I can do that and push to your branch and merge if that's OK.

Also, to make DeclarativeBase work, we need this commit: dcf2d84, but that is part of #17922, which I'll merge today.

[jdavcs]

Thanks @jmchilton!

(all: please don't merge before 82fd47b commit is dropped)

[jdavcs]

Suggested change

	object_store_id_overrides: Mapped[Optional[STR_TO_STR_DICT]] = mapped_column(JSONType)
	object_store_id_overrides: Mapped[Optional[STR_TO_STR_DICT]]

I think we don't need this: it's derived from the type_annotation_map.
Same for transform below.