[24.0] Fix History Dataset Association creation so that hid is always set #18036
Conversation
I think that's the missing piece to find all the instances where we might still be committing HDAs without HIDs, and I think we might have a similar bug for DatasetCollectionElements.
Otherwise we may end up with a history item that either temporarily or permanently lacks a hid. This could currently be encountered via implicit conversion that set `visible=False` in the when calling `datatype.convert_dataset`.
mvdbeek
force-pushed
the
implicit_conversion_no_hid_fix
branch
from
5d6e057 to
badbd3a
Compare
mvdbeek
changed the title
mvdbeek
force-pushed
the
implicit_conversion_no_hid_fix
branch
from
3196da4 to
cedc441
Compare
| log.debug("Using strict flush checks") | ||
|
|
||
| def before_flush(session, flush_context, instances): | ||
| for obj in session.new: |
There was a problem hiding this comment.
Isn't that what versioned_objects_strict() does? If so, why not do something like this:
for obj in versioned_objects_strict(session.new):
passThere was a problem hiding this comment.
It creates a version, which doesn't make sense for new objects.
There was a problem hiding this comment.
Ah, yes, I could do that, though it's not related to versioning objects, which is what threw my off in your comment.
There was a problem hiding this comment.
And then, if I'm not mistaken, you wouldn't need this if/then that checks os.environ and contains 2 similar function definitions, and just use the existing logic, I think? i.e., setting versioned_objects = versioned_objects_strict would still work?
There was a problem hiding this comment.
I don't think one is better than the other with the exception this one is slightly better since we don't run the os.environ check in the module space ?
mvdbeek
force-pushed
the
implicit_conversion_no_hid_fix
branch
from
cedc441 to
121f27e
Compare
We already had test coverage for this, but we were not running the strict checks on new items. b1333e0 should be the missing puzzle piece to prevent this entirely in testing, however we might discover more bugs in the process.
xref #18014:
Significantly extends test coverage by also checking new items for strict rules, and also fixes serialization of Dataset Collection Elements for collection elements that aren't populated yet.
How to test the changes?
(Select all options that apply)
License