Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix --field not applying pending contracts #1778

Merged
merged 1 commit into from
Jan 24, 2024
Merged

Conversation

yannham
Copy link
Member

@yannham yannham commented Jan 24, 2024

Closes #1774

The --field argument has been added in 1.3 to act only on a specific field of the configuration. However, pending contracts (contracts attached to the field by metadata annotations or propagated through merging) weren't properly applied, leading to illegal values being accepted.

This commit fixes the issue by adding the missing contract application step. A snapshot test is added as a regression test. While it's not ideal (we don't encode in the test what is the expected result), it's the best we can do currently for CLI-only tests.

I plan to publish a minor release with this patch, as silently not applying contract is serious enough.

The `--field` argument has been added in 1.3 to act only on a specific
field of the configuration. However, pending contracts (contracts
attached to the field by metadata annotations or propagated through
merging) weren't properly applied, leading to illegal values being
accepted.

This commit fixes the issue by adding the missing contract application
step. A snapshot test is added as a regression test. While it's not
ideal (we don't encode in the test what is the expected result), it's
the best we can do currently for CLI-only tests.
@yannham yannham requested review from jneem and vkleen January 24, 2024 11:18
@github-actions github-actions bot temporarily deployed to pull request January 24, 2024 11:23 Inactive
Copy link
Member

@vkleen vkleen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like a good quick fix. Long term, I wonder if it wouldn't be better to encapsulate the "extract value out of field" operation so that we'd have a harder time forgetting to do that. Now, there are quite a few places in the code base that need to call RuntimeContract::apply_all and the whole setup feels quite fragile.

@yannham
Copy link
Member Author

yannham commented Jan 24, 2024

This is a good point. A simple helper first, like value_with_ctrs(&self) -> Option<RichTerm> would probably go a long way, at least for deduplication. For correctness, we might end up making the value field private, even if general I don't like accessors proliferation when the language provide a built in way to access struct fields, but I guess encapsulation (for correctness) is a valid motivation.

@yannham yannham added this pull request to the merge queue Jan 24, 2024
Merged via the queue into master with commit 266f609 Jan 24, 2024
5 checks passed
@yannham yannham deleted the fix/contract-eval-field branch January 24, 2024 13:36
yannham added a commit that referenced this pull request Jan 25, 2024
The `--field` argument has been added in 1.3 to act only on a specific
field of the configuration. However, pending contracts (contracts
attached to the field by metadata annotations or propagated through
merging) weren't properly applied, leading to illegal values being
accepted.

This commit fixes the issue by adding the missing contract application
step. A snapshot test is added as a regression test. While it's not
ideal (we don't encode in the test what is the expected result), it's
the best we can do currently for CLI-only tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Contract attached to record key not getting evaluated
2 participants