AL-751 validate on DNode or LNode assignment to a Node attribute

[perrygreenfield]

Resolves AL-751

Closes #274

This PR addresses the shortcoming in the previous PR ( #253) to deal with this issue, namely that no validation was done on assignment. Now is validated on assignment.

[codecov]

Codecov Report

All modified lines are covered by tests ✅

Files	Coverage Δ
src/roman_datamodels/stnode/_converters.py	98.33% <ø> (ø)
src/roman_datamodels/stnode/_node.py	89.02% <100.00%> (+0.27%)	⬆️
tests/test_models.py	100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

📢 Thoughts on this report? Let us know!.

[perrygreenfield]

This is currently waiting for ASDF 3.0.0 to be released.

[braingram]

Thanks for putting this together. I think I understand what the goal is now.

I'm wondering if the following would achieve the same result (and be compatible with asdf 2.15).

It appears that validation is occurring through a call to _validate in stnode/_node:

roman_datamodels/src/roman_datamodels/stnode/_node.py

Lines 64 to 66 in 72ebfe8

	def _validate(attr, instance, schema, ctx):
	tagged_tree = yamlutil.custom_tree_to_tagged_tree(instance, ctx)
	return _value_change(attr, tagged_tree, schema, False, will_strict_validate(), ctx)

As the goal is to convert DNode and LNode instances to dict and list prior to validation. Would adding the following to _validate work?

def _validate(attr, instance, schema, ctx):
    if isinstance(instance, DNode):
        instance = instance._data
    elif isinstance(instance, LNode):
        instance = instance.data
    tagged_tree = yamlutil.custom_tree_to_tagged_tree(instance, ctx)
    return _value_change(attr, tagged_tree, schema, False, will_strict_validate(), ctx)

If I'm understanding stnode, the DNode and LNode creation happens at access time so there is not really any nested LNodes (for instance) like in the case of exposure.read_pattern. This is instead a single LNode instance that creates new LNode instances when values are acccessed. So converting the top level instance passed to validate should be equivalent (and testing this locally the read_pattern assignment in the test you included works with the above modification and asdf 2.15).

[braingram]

The CI is looking much greener :)

Would you rebase this to pick up the docs fixes in #266 to see if that makes RTD succeed?

Also, to follow up on the note about the subclasses that need to keep the DNode/LNode, which ones are you referring to? Is it straightforward to add a test for this?

[perrygreenfield]

Actually, it may be better to test to see if the instance is a TaggedObjectNode or TaggedListNode, where each inherits from DNode and LNode respectively. I.e., if isinstance(instance, DNode) and not isinstance(instance, TaggedObjectNode): Is this what you were getting at? I'm not sure if there are any cases that use the TaggedListNode offhand. Lots of TaggedObjectNode cases though. Yes, I should rebase.

[braingram]

It looks like the changelog still needs some clean-up. I also added a comment about the type checking. Thanks!

[braingram]

The pre-commit job wasn't picking up the noqa comments I think due to a missing :.

[braingram]

It looks like this needs another rebase. I'm also still seeing unrelated changelog edits. Other than that the code changes look good to me.

[braingram]

LGTM!