diff --git a/CHANGELOG.md b/CHANGELOG.md index 56bde4bc..410f376c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,13 @@ `RepositoryConfig`, a constructed app holds it as `repository`, `Phabfive.arcrc_hosts()` lists the instances in `~/.arcrc` without their tokens, and `Maniphest.tag_hints()` looks the hints up +* **A spec can switch interpolation off with `interpolate: false`.** Every string in a + spec is a Jinja2 template, so a task description or paste that only quotes one - + `{{ .Values.tag }}` in a Helm example - was refused as an undefined variable, and no + escape survived both the check and the render. With `interpolate: false` in the + envelope every string is sent exactly as written, in a create and a search spec + alike. Such a spec takes no variables: a `variables:` section or a `--set` is the new + problem code `interpolation-off` ## Bug Fixes diff --git a/docs/phorge-spec.md b/docs/phorge-spec.md index 6c514a6c..a313a57d 100644 --- a/docs/phorge-spec.md +++ b/docs/phorge-spec.md @@ -128,12 +128,13 @@ priority = "high" ## The envelope -Three keys describe the document itself rather than the objects in it. +Four keys describe the document itself rather than the objects in it. | Key | Required | Meaning | |---|---|---| +| `interpolate` | optional | `false` sends every string exactly as written: `{{ name }}` is text, not a variable. Absent means `true`. | | `kind` | optional | `create` or `search`. Absent means inferred from the body keys. | | `metadata` | optional | What the *file* is, for a person reading it. Nothing acts on it. | | `spec` | optional | The revision of the format, `phorge/v1alpha1`. Absent means the reader's own revision is assumed. | @@ -702,6 +703,35 @@ under `variables:` is what makes `{{ sprint }}` optional. It is written once, wh the name is introduced, so every use of the name is covered by it and a reader can settle the whole question by reading `variables:`. +### Switching interpolation off + +A document whose text only *quotes* a template - a task description showing a Helm +value, a paste holding a Jinja2 file - has no use for any of the above, and under the +undefined-name rule it is an error. The envelope key says so once, for the whole +document: + +```yaml +spec: phorge/v1alpha1 +kind: create +interpolate: false +tasks: + - title: Document the image tag + description: "Set image.tag to {{ .Values.tag }}" +``` + +With `interpolate: false` every string is sent **exactly as written**: no name is +looked up and nothing is rendered. It applies to every kind, and to every document of +a multi-document file - two documents that disagree about it are refused, like any +other envelope key. + +Such a document takes no variables. A `variables:` section, or a value supplied from +outside, is the error `interpolation-off` rather than something quietly ignored. The +value is `true` or `false`; anything else is `wrong-type` and leaves interpolation on. + +There is deliberately no per-string escape. `{% raw %}` is outside the portable +subset, and whether it is consumed depends on whether a reader renders a document +that declares no variables. + --- ## Grammars @@ -867,6 +897,7 @@ record to, and a clean report for a check that never ran is worse than an error. | `undefined-variable` | offline | error | A `{{ name }}` nothing declares, overrides or defaults. | | `missing-variable` | offline | error | A declared variable with no value, no default and no override. | | `circular-variable` | offline | error | Variables that define each other. | +| `interpolation-off` | offline | error | A variable declared for, or supplied to, a document that says `interpolate: false`. | | `duplicate-local-id` | offline | error | Two items declare the same `id:`. | | `unknown-local-id` | offline | error | A `$ref` naming a local id the document never declares, or one of the wrong object type for the key. | | `local-id-cycle` | offline | error | Local references that depend on each other, so no order creates them. | diff --git a/phabfive/SKILL.md b/phabfive/SKILL.md index e9315fa0..d65a9065 100644 --- a/phabfive/SKILL.md +++ b/phabfive/SKILL.md @@ -975,7 +975,9 @@ thing written two ways. Every string is rendered with Jinja2. A `{{ name }}` the file does not declare under `variables:` and no `--set NAME=VALUE` supplies is an error before anything runs, not an -empty string. `--set` is repeatable and beats the file's own default. +empty string. `--set` is repeatable and beats the file's own default. A file whose text +quotes `{{ ... }}` itself says `interpolate: false` at the top level: every string is +then sent as written, and the file takes no variables. Check a file without running it: diff --git a/phabfive/spec/envelope.py b/phabfive/spec/envelope.py index 2e174e94..5a3b34dc 100644 --- a/phabfive/spec/envelope.py +++ b/phabfive/spec/envelope.py @@ -70,7 +70,7 @@ SUPPORTED_SPEC_VERSIONS = frozenset({SPEC_VERSION}) #: The keys that belong to the envelope rather than to the body. -ENVELOPE_KEYS = frozenset({"spec", "kind", "metadata"}) +ENVELOPE_KEYS = frozenset({"spec", "kind", "metadata", "interpolate"}) #: The `metadata:` keys with a meaning. Anything else is kept in #: `Metadata.extra`, where the offline validation pass reports it as a @@ -226,6 +226,14 @@ class Envelope: metadata: Metadata = dataclasses.field(default_factory=lambda: Metadata()) """What the file says about itself; empty when it says nothing.""" + interpolate: Any = True + """`interpolate:` exactly as written, True when the file said nothing. + + Only `False` switches interpolation off - see `Spec.interpolates`. It is + kept as written rather than coerced so the offline pass can report an + `interpolate: "no"` instead of quietly reading it as one or the other. + """ + def _kind_names() -> str: """The kinds a message offers, in a stable order.""" @@ -521,6 +529,7 @@ def normalize_documents( spec_version, _ = _declared(parsed, "spec", source=source) declared_kind, kind_declared = _declared(parsed, "kind", source=source) metadata, _ = _declared(parsed, "metadata", source=source) + interpolate, interpolate_declared = _declared(parsed, "interpolate", source=source) if spec_version is not None and spec_version not in SUPPORTED_SPEC_VERSIONS: supported = ", ".join(sorted(SUPPORTED_SPEC_VERSIONS)) @@ -556,6 +565,7 @@ def normalize_documents( kind=resolved, kind_declared=kind_declared, metadata=Metadata.from_data(metadata, source=source), + interpolate=interpolate if interpolate_declared else True, ) body = _merged( @@ -629,6 +639,16 @@ def metadata(self) -> Metadata: """Shorthand for `spec.envelope.metadata`.""" return self.envelope.metadata + @property + def interpolates(self) -> bool: + """Whether `{{ name }}` in this spec is a variable rather than text. + + False only for a document that says `interpolate: false`: every + string in it is then sent exactly as written, which is what a + description quoting a template of its own needs. + """ + return self.envelope.interpolate is not False + @classmethod def from_data( cls, @@ -744,10 +764,25 @@ def render(self, variables: Mapping[str, Any] | None = None) -> Spec: value nor a default, or a cycle between them. This raises rather than reporting because a half-rendered spec is not a spec; `validate_offline` is what reports the same mistakes as problems - a person can read all at once. + a person can read all at once. Also when values are given to a + spec that says `interpolate: false`, which has nothing to put + them in. + + Notes + ----- + A spec that says `interpolate: false` comes back with its body + untouched and `rendered` True: there is nothing left to render. """ from phabfive.spec.variables import render_tree, resolve_variables + if not self.interpolates: + if self.variables or variables: + raise PhabfiveDataException( + f"{self.source} says interpolate: false, so it takes no variables" + ) + + return dataclasses.replace(self, rendered=True) + values = resolve_variables(self.variables, variables) return dataclasses.replace( @@ -825,6 +860,9 @@ def to_data(self) -> dict[str, Any]: if metadata: data["metadata"] = metadata + if self.envelope.interpolate is not True: + data["interpolate"] = self.envelope.interpolate + if self.variables: data[VARIABLES_KEY] = copy.deepcopy(dict(self.variables)) diff --git a/phabfive/spec/problems.py b/phabfive/spec/problems.py index 397f07c6..bc264ddc 100644 --- a/phabfive/spec/problems.py +++ b/phabfive/spec/problems.py @@ -48,6 +48,7 @@ "undefined-variable", "missing-variable", "circular-variable", + "interpolation-off", # The offline layer: local ids "duplicate-local-id", "unknown-local-id", diff --git a/phabfive/spec/schema.py b/phabfive/spec/schema.py index d57f9366..cf015f85 100644 --- a/phabfive/spec/schema.py +++ b/phabfive/spec/schema.py @@ -508,6 +508,13 @@ def _envelope_properties(kind: Kind) -> dict[str, Any]: "description": "The verb: what this spec asks for.", }, "metadata": _metadata_schema(), + "interpolate": { + "type": "boolean", + "description": ( + "false sends every string as written, so {{ name }} is text " + "rather than a variable." + ), + }, VARIABLES_KEY: { "type": "object", "description": ( diff --git a/phabfive/spec/validate.py b/phabfive/spec/validate.py index 41662c94..4b8056dd 100644 --- a/phabfive/spec/validate.py +++ b/phabfive/spec/validate.py @@ -218,11 +218,69 @@ def _check_envelope(spec: "Spec") -> list[Problem]: ) ) + if not isinstance(envelope.interpolate, bool): + problems.append( + problem( + DOCUMENT, + field="interpolate", + value=envelope.interpolate, + reason=( + f"interpolate is true or false, not a " + f"{_type_name(envelope.interpolate)}." + ), + code="wrong-type", + ) + ) + problems += _check_metadata(spec) return problems +def _check_literal(spec: "Spec", overrides: Mapping[str, Any]) -> list[Problem]: + """A document that says `interpolate: false`, given variables anyway. + + Its strings are sent as written, so a value declared for it or supplied + to it has nowhere to go. Reported rather than ignored: a `--set` that + silently does nothing is how a title ends up reading `{{ sprint }}`. + """ + problems: list[Problem] = [] + + for name, value in spec.variables.items(): + problems.append( + problem( + "variables", + field=name, + value=value, + reason=( + f"Variable {name!r} is declared, but the document says " + f"interpolate: false, so nothing reads it. Remove one of " + f"the two." + ), + code="interpolation-off", + ) + ) + + for name, value in overrides.items(): + if name in spec.variables: + continue + + problems.append( + problem( + "variables", + field=name, + value=value, + reason=( + f"Variable {name!r} was supplied, but the document says " + f"interpolate: false, so nothing reads it." + ), + code="interpolation-off", + ) + ) + + return problems + + def _check_metadata(spec: "Spec") -> list[Problem]: """What a file says about itself. @@ -1736,6 +1794,15 @@ def validate_offline( problems = _check_envelope(spec) + if not spec.interpolates: + # Every string is text: no name to look up, nothing to render, and + # no value check to skip because it still holds a `{{` + problems += _check_literal(spec, overrides) + problems += _check_schema(spec, frozenset()) + problems += _check_semantics(spec) + + return problems + variable_problems, effective = _check_variables(spec, overrides) problems += variable_problems diff --git a/scripts/gen_spec_docs.py b/scripts/gen_spec_docs.py index f1485062..e05b8220 100644 --- a/scripts/gen_spec_docs.py +++ b/scripts/gen_spec_docs.py @@ -151,6 +151,12 @@ "A declared variable with no value, no default and no override.", ), "circular-variable": ("offline", "error", "Variables that define each other."), + "interpolation-off": ( + "offline", + "error", + "A variable declared for, or supplied to, a document that says " + "`interpolate: false`.", + ), "duplicate-local-id": ( "offline", "error", @@ -430,6 +436,10 @@ def _envelope_block() -> list[str]: + "`. Absent means inferred from the body keys." ), "metadata": "What the *file* is, for a person reading it. Nothing acts on it.", + "interpolate": ( + "`false` sends every string exactly as written: `{{ name }}` is " + "text, not a variable. Absent means `true`." + ), } rows = [(f"`{key}`", "optional", described[key]) for key in sorted(ENVELOPE_KEYS)] diff --git a/tests/test_spec_interpolate.py b/tests/test_spec_interpolate.py new file mode 100644 index 00000000..c28b39c3 --- /dev/null +++ b/tests/test_spec_interpolate.py @@ -0,0 +1,202 @@ +# -*- coding: utf-8 -*- + +"""`interpolate: false`: a document whose strings are text, not templates. + +Every string in a spec is a Jinja2 template, and a name it mentions that +nothing declares is an error. That is the right rule for a spec somebody +wrote and the wrong one for text that only *quotes* a template - a task +description showing a Helm value, say - which has no spelling that survives +both the check and the render. The envelope key is the way out, and it is +what a document written from an existing object carries. +""" + +import pytest + +from phabfive.exceptions import PhabfiveDataException +from phabfive.spec import Spec, build_schema +from phabfive.spec.envelope import ENVELOPE_KEYS + +QUOTED = "Set image.tag to {{ .Values.tag }} and {{ release }}" + + +def create_spec(**envelope): + return Spec.from_data( + { + "kind": "create", + **envelope, + "tasks": [{"title": "Document {{ release }}", "description": QUOTED}], + } + ) + + +def codes(problems): + return [problem.code for problem in problems] + + +class TestWithoutIt: + """What the key exists to get around, pinned so it stays true.""" + + def test_a_quoted_template_is_an_undefined_variable(self): + problems = create_spec().validate_offline() + + assert set(codes(problems)) == {"undefined-variable"} + + def test_the_raw_block_escape_passes_but_is_never_rendered_away(self): + """Why `{% raw %}` is not the answer. + + It names no variable, so it validates - and a spec with no + variables is not rendered by the command, so the escape itself + would be what reaches the server. + """ + spec = Spec.from_data( + { + "kind": "create", + "tasks": [{"title": "{% raw %}{{ release }}{% endraw %}"}], + } + ) + + assert spec.validate_offline() == [] + assert spec.items("task")[0]["title"].startswith("{% raw %}") + + +class TestWithIt: + def test_it_is_an_envelope_key_and_not_part_of_the_body(self): + spec = create_spec(interpolate=False) + + assert "interpolate" in ENVELOPE_KEYS + assert "interpolate" not in spec.body + assert spec.envelope.interpolate is False + assert spec.interpolates is False + + def test_a_document_that_says_nothing_interpolates(self): + spec = create_spec() + + assert spec.envelope.interpolate is True + assert spec.interpolates is True + + def test_a_quoted_template_is_clean(self): + assert create_spec(interpolate=False).validate_offline() == [] + + def test_rendering_leaves_every_string_as_written(self): + rendered = create_spec(interpolate=False).render() + + assert rendered.rendered is True + assert rendered.items("task")[0] == { + "title": "Document {{ release }}", + "description": QUOTED, + } + + def test_the_other_checks_still_run(self): + spec = Spec.from_data( + { + "kind": "create", + "interpolate": False, + "tasks": [{"title": "{{ release }}", "colour": "red"}], + } + ) + + assert codes(spec.validate_offline()) == ["unknown-key"] + + def test_a_search_spec_takes_it_too(self): + spec = Spec.from_data( + { + "kind": "search", + "interpolate": False, + "searches": [{"search": {"text_query": "{{ release }}"}}], + } + ) + + assert spec.validate_offline() == [] + + def test_explicit_true_is_the_default_spelled_out(self): + assert set(codes(create_spec(interpolate=True).validate_offline())) == { + "undefined-variable" + } + + +class TestVariablesHaveNowhereToGo: + def test_a_declared_variable_is_refused(self): + spec = Spec.from_data( + { + "kind": "create", + "interpolate": False, + "variables": {"release": "1.0"}, + "tasks": [{"title": "Document {{ release }}"}], + } + ) + + problems = spec.validate_offline() + + assert codes(problems) == ["interpolation-off"] + assert problems[0].field == "release" + + def test_a_supplied_variable_is_refused(self): + problems = create_spec(interpolate=False).validate_offline( + variables={"release": "1.0"} + ) + + assert codes(problems) == ["interpolation-off"] + + def test_one_name_declared_and_supplied_is_reported_once(self): + spec = Spec.from_data( + { + "kind": "create", + "interpolate": False, + "variables": {"release": "1.0"}, + "tasks": [{"title": "t"}], + } + ) + + assert codes(spec.validate_offline(variables={"release": "2.0"})) == [ + "interpolation-off" + ] + + def test_rendering_with_values_raises_rather_than_dropping_them(self): + with pytest.raises(PhabfiveDataException, match="interpolate: false"): + create_spec(interpolate=False).render({"release": "1.0"}) + + +class TestTheValueIsABoolean: + @pytest.mark.parametrize("value", ["false", "no", 0, None]) + def test_anything_else_is_reported_and_does_not_switch_it_off(self, value): + spec = create_spec(interpolate=value) + + assert spec.interpolates is True + assert "wrong-type" in codes(spec.validate_offline()) + + +class TestItSurvivesTheRoundTrip: + def test_to_data_writes_it_back(self): + spec = create_spec(interpolate=False) + data = spec.to_data() + + assert data["interpolate"] is False + assert Spec.from_data(data) == spec + + def test_to_data_does_not_grow_one(self): + assert "interpolate" not in create_spec().to_data() + + def test_documents_that_disagree_are_refused(self): + with pytest.raises(PhabfiveDataException, match="disagree about interpolate"): + Spec.from_data( + [ + {"kind": "create", "interpolate": False, "tasks": [{"title": "a"}]}, + {"interpolate": True, "tasks": [{"title": "b"}]}, + ] + ) + + def test_one_document_saying_it_covers_the_file(self): + spec = Spec.from_data( + [ + {"kind": "create", "interpolate": False, "tasks": [{"title": "a"}]}, + {"tasks": [{"title": "{{ b }}"}]}, + ] + ) + + assert spec.validate_offline() == [] + + +class TestTheSchemaKnowsIt: + @pytest.mark.parametrize("kind", ["create", "search"]) + def test_it_is_a_boolean_property(self, kind): + assert build_schema(kind)["properties"]["interpolate"]["type"] == "boolean"