Surface gojq runtime errors instead of treating them as results#214
Open
damilolaedwards wants to merge 1 commit into
Open
Surface gojq runtime errors instead of treating them as results#214damilolaedwards wants to merge 1 commit into
damilolaedwards wants to merge 1 commit into
Conversation
gojq returns a runtime evaluation error (type error, wrong type, division by zero, and so on) as a value with ok true. ResolveQuery and ConsumeVars read that value without checking its type, so an erroring query was reported as a successful result. In the task if condition path this made the condition a non boolean value and the task was silently skipped rather than failing; in ConsumeVars the error object was written into the task config. Type assert the result and return the error in both places.
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
1 participant
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.
Problem
gojq returns runtime evaluation errors (type errors, wrong-type indexing, division by zero, context timeout) as values of type
error, with the iterator reportingok == true.ResolveQueryandConsumeVarsread that value without checking its type, so an erroring query was treated as a successful result.Consequences:
if:path, the erroring condition became a non-boolean value, so the boolean assertion failed and the task was silently skipped instead of failing. A check that should have run (and might have failed) was quietly dropped and the run stayed green.ConsumeVars, the error object was marshalled into the task config, corrupting the consumed value.Fix
After reading the query result, type assert it against
errorand return the error in bothResolveQueryandConsumeVars. A failed condition or config query now surfaces loudly, the same way a parse error already did, instead of being hidden.Tests
An erroring query (adding a number to a string) now returns an error from both functions and does not leak the error value as a result; valid queries still resolve normally.
go build ./...,go vet, and the vars package tests pass.