fix: report undefined CLI binding env vars#1034
Open
He-Pin wants to merge 3 commits into
Open
Conversation
He-Pin
force-pushed
the
fix/cli-undefined-env-vars
branch
from
2023dc4 to
23eea1d
Compare
Motivation: --ext-str NAME and --ext-code NAME read NAME from the environment when no = value is provided. If NAME was unset, sjsonnet either crashed with a NullPointerException (--ext-str) or produced a misleading extVar kind error (--ext-code), and --ext-code could even succeed when the binding was unused. Modification: Detect missing environment variables while parsing CLI bindings and return the same clean CLI-level error used by go-jsonnet. Keep the success path allocation-light with an empty-binding fast path, a single mutable map for non-empty bindings, and a stackless CLI exception only on the error path. Add jsonnet-resource + golden CLI regression tests for --ext-str and --ext-code. Result: Missing binding environment variables now fail eagerly with ERROR: environment variable NAME was undefined instead of leaking Java/internal errors or succeeding inconsistently.
He-Pin
force-pushed
the
fix/cli-undefined-env-vars
branch
from
23eea1d to
eb137b6
Compare
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.
Motivation
--ext-str NAMEcrashed with a JavaNullPointerExceptionwhen the environment variableNAMEwas undefined.--ext-code NAMEproduced an unhelpful"Unsupported external variable kind: code"error. Reference implementations (C++ jsonnet, go-jsonnet) report a clean error message.--ext-str UNDEFINED_VARenvironment variable UNDEFINED_VAR was undefinedenvironment variable UNDEFINED_VAR was undefinedNullPointerExceptionstack traceenvironment variable UNDEFINED_VAR was undefinedModification
parseBindingsto use a singlecollection.mutable.HashMapwith while-loop iteration instead of chainedMap() ++ ... ++ ...with intermediate collections.System.getenv(s)fornullbefore invoking the value transformation, throwing aCliErrorwithfillInStackTraceoverridden to avoid JVM overhead.Map.emptyfast path when all binding lists are empty.--ext-str,--ext-code,--tla-str,--tla-code.Result
HashMapinstead of 8 intermediate collections from chained.mapand++calls.Map.emptywithout allocating.CliError.fillInStackTraceoverride avoids expensive JVM stack capture on error path.