Validate change/variable targets against the model they reference - #168
Open
jcschaff wants to merge 2 commits into
Open
Validate change/variable targets against the model they reference#168jcschaff wants to merge 2 commits into
jcschaff wants to merge 2 commits into
Conversation
`validate_doc` builds `model_etrees` in `for model in doc.models:`, then three
later `validate_target(...)` calls read `model_etrees.get(model, None)` - the
leaked loop variable, which by then is always the last model in the document -
where they mean the model belonging to the change or variable being validated.
Every other argument at those call sites already comes from `change.model` /
`variable.model`; only the etree was taken from the stale name, so the error
names a model that was never searched:
XPath `...parameter[@id='pA']/@value` does not match any elements of
model `modelA`.
reported for a target that does resolve against modelA's source.
The two other readers of `model_etrees.get(model, ...)`, at L190 and L1408,
use genuine loop variables and are left alone.
Fixes biosimulators#167
Validates one document twice, swapping only the order of two models in `doc.models`. The setValue references the same model and carries the same target both times, so both orderings must be valid; before the previous commit the ordering alone decided it. The existing repeated-task fixtures all have a single model, where the leaked variable happens to equal the right model - which is why this went unnoticed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #167.
validate_docreports validrepeatedTasksetValuetargets as invalid, because the XPath is evaluated against the last<model>in the document rather than the model the change references.Cause
model_etreesis built infor model in doc.models:(L131, rebound at L229). Three latervalidate_target(...)calls then readmodel_etrees.get(model, None)— the leaked loop variable, which by then is always the last model — where they mean the model belonging to the change or variable being validated:model_etrees.get(variable.model, None)setValuechange targetmodel_etrees.get(change.model, None)setValuevariablemodel_etrees.get(variable.model, None)Every other argument at those call sites already comes from
change.model/variable.model— only the etree was taken from the stale name. That is why the message names a model that was never searched, and the element it claims is missing is plainly present in the named model's source.L190 and L1408 also read
model_etrees.get(model, ...)but use genuine loop variables; they are untouched.Test
test_validate_repeated_task_xpaths_use_the_referenced_modelvalidates one document twice, swapping only the order of two models indoc.models. The change references the same model and carries the same target both times, so both orderings must be valid.On
devthe new test fails in one ordering with exactly the reported message:and passes in the other — the ordering alone decides it. With the fix both orderings pass.
The existing repeated-task fixtures (
BIOMD0000000297_repeat_*.sedml) all have a single model, where the leaked variable happens to equal the correct one, which is why this went unnoticed.Verification
tests/sedml/run against this branch versus the same clone with only thevalidation.pychange reverted:The 3 pre-existing failures (
test_validate_model_with_language,test_get_parameters_variables_for_simulation,test_build_combine_archive_for_model_xpp_with_plot) are missing optional dependencies in my environment —ModuleNotFoundError: No module named 'libcellml'and XPP — and fail identically before and after. Nothing else changes.Deliberately not included
Two adjacent points raised in #167 are left out, since they are behaviour calls rather than clear bugs and I did not want to bundle them:
model_change=change.model and change.model.has_structural_changes()where its sibling at L359 usesvariable.model. Since the target validated there isvariable.target,variable.modellooks right, but it can turn warnings into errors for some documents.Model.has_structural_changes()returnsTrueforModelAttributeChange, contradicting its docstring (add/replace/remove only). This is what demotes genuine XPath misses to warnings for any model carrying changes, so it masked half of this bug — but tightening it would surface errors in documents that validate today.Happy to fold either in, or open them separately, whichever you prefer.
🤖 Generated with Claude Code
https://claude.ai/code/session_01SvqmME7MkRUNEYje5HpiLt