[24.0] Don't commit without having set a hid

[mvdbeek]

hid-less items in a history are bad, and this is potentially one way to produce them.

We've got a bunch of them in the history and this has been showing up in as https://sentry.galaxyproject.org/share/issue/3cc69eedc9f841b6ae8ac02e3f88e8fe/:

ValidationError: 210 validation errors for HistoryContentsWithStatsResult
contents.0.HDACustom.type
  Input should be 'file' [type=literal_error, input_value='collection', input_type=str]
    For further information visit https://errors.pydantic.dev/2.5/v/literal_error
contents.0.HDACustom.history_content_type
  Input should be 'dataset' [type=literal_error, input_value='dataset_collection', input_type=str]
    For further information visit https://errors.pydantic.dev/2.5/v/literal_error
contents.0.HDADetailed.hid
  Input should be a valid integer [type=int_type, input_value=None, input_type=NoneType]
    For further information visit https://errors.pydantic.dev/2.5/v/int_type
contents.0.HDADetailed.type
  Input should be 'file' [type=literal_error, input_value='collection', input_type=str]
    For further information visit https://errors.pydantic.dev/2.5/v/literal_error
contents.0.HDADetailed.history_content_type
  Input should be 'dataset' [type=literal_error, input_value='dataset_collection', input_type=str]
    For fur...
  File "starlette/applications.py", line 123, in __call__
    await self.middleware_stack(scope, receive, send)
  File "starlette/middleware/errors.py", line 186, in __call__
    raise exc
  File "starlette/middleware/errors.py", line 164, in __call__
    await self.app(scope, receive, _send)
  File "starlette_context/middleware/raw_middleware.py", line 92, in __call__
    await self.app(scope, receive, send_wrapper)
  File "starlette/middleware/base.py", line 189, in __call__
    with collapse_excgroups():
  File "contextlib.py", line 155, in __exit__
    self.gen.throw(typ, value, traceback)
  File "starlette/_utils.py", line 93, in collapse_excgroups
    raise exc
  File "starlette/middleware/base.py", line 191, in __call__
    response = await self.dispatch_func(request, call_next)
  File "galaxy/webapps/galaxy/fast_app.py", line 108, in add_x_frame_options
    response = await call_next(request)
  File "starlette/middleware/base.py", line 165, in call_next
    raise app_exc
  File "starlette/middleware/base.py", line 151, in coro
    await self.app(scope, receive_or_disconnect, send_no_error)
  File "starlette/middleware/exceptions.py", line 62, in __call__
    await wrap_app_handling_exceptions(self.app, conn)(scope, receive, send)
  File "starlette/_exception_handler.py", line 64, in wrapped_app
    raise exc
  File "starlette/_exception_handler.py", line 53, in wrapped_app
    await app(scope, receive, sender)
  File "starlette/routing.py", line 758, in __call__
    await self.middleware_stack(scope, receive, send)
  File "starlette/routing.py", line 778, in app
    await route.handle(scope, receive, send)
  File "starlette/routing.py", line 299, in handle
    await self.app(scope, receive, send)
  File "starlette/routing.py", line 79, in app
    await wrap_app_handling_exceptions(app, request)(scope, receive, send)
  File "starlette/_exception_handler.py", line 64, in wrapped_app
    raise exc
  File "starlette/_exception_handler.py", line 53, in wrapped_app
    await app(scope, receive, sender)
  File "starlette/routing.py", line 74, in app
    response = await func(request)
  File "fastapi/routing.py", line 278, in app
    raw_response = await run_endpoint_function(
  File "fastapi/routing.py", line 193, in run_endpoint_function
    return await run_in_threadpool(dependant.call, **values)
  File "starlette/concurrency.py", line 42, in run_in_threadpool
    return await anyio.to_thread.run_sync(func, *args)
  File "anyio/to_thread.py", line 56, in run_sync
    return await get_async_backend().run_sync_in_worker_thread(
  File "anyio/_backends/_asyncio.py", line 2144, in run_sync_in_worker_thread
    return await future
  File "anyio/_backends/_asyncio.py", line 851, in run
    result = context.run(func, *args)
  File "galaxy/webapps/galaxy/api/history_contents.py", line 465, in index
    items = self.service.index(
  File "galaxy/webapps/galaxy/services/history_contents.py", line 312, in index
    return self.__index_v2(trans, history_id, params, serialization_params, filter_query_params, accept)
  File "galaxy/webapps/galaxy/services/history_contents.py", line 1006, in __index_v2
    return HistoryContentsWithStatsResult(contents=items, stats={"total_matches": total_matches})

I'm hesitatant to relax the validation here, these are really pretty bad bugs. We do appear to have test coverage here via https://app.codecov.io/gh/galaxyproject/galaxy/commit/4a4ead2b5ee913b61a97b84b60e77f28415d5ad9/blob/lib/galaxy/tools/actions/upload_common.py#L144

so I'm surprised we're not hitting

galaxy/lib/galaxy/model/__init__.py

Line 5140 in 67436a6

raise Exception(f"HistoryDatasetAssociation {self} without hid, this is not valid")

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]

I think this is a good example of a commit we don't really need. It used to be a flush, and when we migrated from autocommit as a pre-2.0 step (and a move towards sanity) we changed ALL flushes to commits. However, not all flushes were needed in the first place. This particular flush was added 15 years ago as a transition from hda.flush(). We don't need it: the subsequent commit is sufficient. Also, the next 3 lines do belong in the same transaction.
I suspect there may be many other similar cases where we can get rid of unnecessary commits.