diff --git a/CHANGELOG.md b/CHANGELOG.md index 3e1207b7..ac3f3909 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,15 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **`fluid validate` and `fluid plan` warn about an id the build phase refuses.** The build + phase puts `contract.id` and each `builds[].id` in directory names and generated code, so they + must start with a letter or underscore, use only letters, digits, `_`, `.` and `-`, and be at + most 128 characters. The contract schema allows more (`1orders`, or a longer id), and + `fluid apply` in a build mode refuses such an id before it applies anything, but validate and + plan said nothing. For a contract with builds, both now warn and name the id: plain + `fluid validate` passes and `--strict` fails, and `fluid plan` still writes the plan, whatever + its `--mode`. A contract without builds is not checked, as in `fluid apply`. + - **IaC plugins load when a contract needs their platform, and cannot take a built-in's name unless the operator says so.** A package that ships an OpenTofu emitter through the `fluid_build.iac_providers` entry-point group used to be imported whenever `fluid_build.iac` @@ -539,6 +548,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 trailing newline from an environment variable or a file; strip the value where it is set. ### Upgrade notes +- New validate warning, so `--strict` fails: a contract with builds whose `contract.id` or a + `builds[].id` the build phase refuses (`1orders`, or more than 128 characters). `fluid apply` + in a build mode already refuses it; rename the id to fit. - A contract with a column typed `array`, `struct`, `map`, `object`, `record` or `row` (or `array(...)`) on an `aws` Glue table or a `gcp` BigQuery table now fails `fluid validate`, `fluid generate iac` and `fluid apply` with `column-type`, where it validated and wrote a diff --git a/fluid_build/build_runners/_ids.py b/fluid_build/build_runners/_ids.py index 5fdb0f24..c68e89c8 100644 --- a/fluid_build/build_runners/_ids.py +++ b/fluid_build/build_runners/_ids.py @@ -47,6 +47,12 @@ # can never traverse out of the directory it is joined into. _IDENT_RE = re.compile(r"^[A-Za-z_][A-Za-z0-9_.\-]{0,127}\Z") +# :data:`_IDENT_RE` in words, for a message that tells an author what to change. +IDENT_RULE = ( + "start with a letter or underscore, use only letters, digits, '_', '.' and '-', " + "and be at most 128 characters" +) + class IdentifierViolation(ValueError): """Raised when a contract identifier fails validation. @@ -103,3 +109,27 @@ def validate_build_ids(contract: Mapping[str, Any]) -> None: for build in builds: if isinstance(build, Mapping) and build.get("id"): validate_identifier(build["id"], kind="build.id") + + +def build_id_warning(contract: Mapping[str, Any]) -> Optional[str]: + """The warning ``fluid validate`` and ``fluid plan`` give for an id the build phase refuses. + + The schema's ``$defs.identifier`` admits more than :data:`_IDENT_RE` (a + leading digit, any length), and the two stay different by decision. So + validate and plan accept such an id and warn, and ``fluid apply`` in a + build mode refuses it. The check is :func:`validate_build_ids`, so the + warning names the id apply would name. Returns ``None`` when it passes. + + The text does not say the schema accepted the id: validate also runs this + on a contract the schema refused, such as one whose ``id`` is ``123``. + """ + try: + validate_build_ids(contract) + except IdentifierViolation as exc: + return ( + f"{exc.kind} {exc.value!r} cannot be used by the build phase, which puts it in " + f"directory names and generated code, so it must {IDENT_RULE}. fluid apply in a " + "build mode (amend-and-build, replace-and-build) refuses this id before it " + "applies anything. Rename it to fit." + ) + return None diff --git a/fluid_build/cli/apply.py b/fluid_build/cli/apply.py index 40b80600..2ae2cf1f 100644 --- a/fluid_build/cli/apply.py +++ b/fluid_build/cli/apply.py @@ -313,7 +313,8 @@ def _gate_build_ids_for_apply(contract: Dict[str, Any]) -> None: The build phase turns ``contract.id`` and ``builds[].id`` into directory names, so it takes a narrower grammar than the schema: ``1orders``, or an - id of 129 characters, passes ``fluid validate`` and ``fluid plan``. The + id of 129 characters, passes ``fluid validate`` and ``fluid plan``, which + only warn (``build_runners._ids.build_id_warning``). The build runner checks the ids itself, but on the OpenTofu engine it runs after ``tofu apply``, so such an id was refused once the product was provisioned, as an "Unexpected error". This is the runner's own check @@ -323,7 +324,11 @@ def _gate_build_ids_for_apply(contract: Dict[str, Any]) -> None: Raises: CLIError: ``apply_build_identifier_invalid``. """ - from fluid_build.build_runners._ids import IdentifierViolation, validate_build_ids + from fluid_build.build_runners._ids import ( + IDENT_RULE, + IdentifierViolation, + validate_build_ids, + ) try: validate_build_ids(contract) @@ -337,10 +342,8 @@ def _gate_build_ids_for_apply(contract: Dict[str, Any]) -> None: "hint": ( "The build phase keeps each build's run records, cursors and " "watermarks under .fluid/runs///, so both ids " - "must start with a letter or underscore, use only letters, digits, " - "'_', '.' and '-', and be at most 128 characters. The contract schema " - "allows more, so fluid validate and fluid plan accept it. Nothing was " - "applied." + f"must {IDENT_RULE}. The contract schema allows more, so fluid " + "validate and fluid plan only warn about it. Nothing was applied." ), }, ) from exc diff --git a/fluid_build/cli/plan.py b/fluid_build/cli/plan.py index 3faab1f4..456321ec 100644 --- a/fluid_build/cli/plan.py +++ b/fluid_build/cli/plan.py @@ -30,7 +30,7 @@ from pathlib import Path from typing import Any, Dict, List, Optional -from fluid_build.cli.console import cprint, warning +from fluid_build.cli.console import cprint, cprint_literal, warning # 11-stage pipeline: stage-6 → stage-7 cryptographic plan-binding. ``plan.json`` # carries ``bundleDigest`` (pins input bundle) + ``planDigest`` (catches @@ -305,6 +305,7 @@ def run(args, logger: logging.Logger) -> int: # plan generation or write. A failure raises ``CLIError`` → # non-zero exit, no plan.json written. _gate_contract_for_plan_or_apply(contract, logger, command="plan") + _warn_unusable_build_ids(contract) # --- --provider gate ------------------------------------------------- # ``--provider`` reaches ``build_provider`` only on the legacy planner @@ -761,6 +762,27 @@ def _gate_contract_for_plan_or_apply( ) +def _warn_unusable_build_ids(contract: Dict[str, Any]) -> None: + """Warn when the build phase will refuse ``contract.id`` or a ``builds[].id``. + + The schema admits ids the build phase refuses (``1orders``, or one of 129 + characters), and ``fluid apply`` in a build mode refuses such an id before + it applies anything. Plan only warns, whatever ``--mode`` it plans for, and + writes the plan. Not part of :func:`_gate_contract_for_plan_or_apply`: + apply shares that gate and refuses the id with a typed error of its own + (``cli/apply.py::_gate_build_ids_for_apply``). Printed, not logged: a + ``warn()`` event would print the same text again as a JSON line. + """ + from fluid_build.build_runners._ids import build_id_warning + + message = build_id_warning(contract) + if message is not None: + # As validate prints its warnings (validate._cprint_literal): literal, + # because the id is contract data, not markup; the terminal wraps the + # line, so Rich never breaks one inside the id; and no highlighter. + cprint_literal(f"\n⚠️ {message}", highlight=False, soft_wrap=True) + + def _validate_plan_actions(plan: Dict[str, Any], logger: logging.Logger) -> None: """Run SDK ``validate_actions()`` over the plan's action list. diff --git a/fluid_build/cli/validate.py b/fluid_build/cli/validate.py index 0183b04e..c4769640 100644 --- a/fluid_build/cli/validate.py +++ b/fluid_build/cli/validate.py @@ -1186,6 +1186,9 @@ def _run_contract_rules( "the contract was NOT checked for a continuous trigger on a batch build" ) + # --- An id the build phase refuses (WARNING) ----------------------- + _build_id_check(validation_result, contract) + # --- Packaging checks (RFC-packaging-modes.md file 9) --------------- # ``fluid validate`` had NO packaging awareness: a `shared` block with # no `pool` validated clean and then failed at `fluid plan` with an @@ -1306,6 +1309,32 @@ def _databricks_binding_check(validation_result, contract: Mapping[str, Any]) -> validation_result.add_warning(msg) +def _build_id_check(validation_result, contract: Mapping[str, Any]) -> None: + """A ``contract.id`` or ``builds[].id`` the build phase refuses, as a warning. + + The schema's ``$defs.identifier`` admits ids the build phase refuses + (``1orders``, or more than 128 characters), and the two grammars stay + different by decision. ``fluid apply`` in a build mode refuses such an id + before it applies anything, with the same rule + (``build_runners._ids.validate_build_ids``). A warning, not an error: plain + validate passes and ``--strict`` fails. A crash in the check is reported, + never swallowed. + """ + try: + from fluid_build.build_runners._ids import build_id_warning + + message = build_id_warning(contract) + except Exception as exc: # noqa: BLE001 — reported, never swallowed + validation_result.add_warning( + f"Build-id check could not run ({type(exc).__name__}: {exc}); " + "contract.id and builds[].id were NOT checked against the ids the build " + "phase accepts" + ) + return + if message is not None: + validation_result.add_warning(message) + + def _duplicate_contract_id_errors(contract: Mapping[str, Any]) -> List[str]: """Refuse two exposes whose output ports would be published under one contract id. diff --git a/tests/cli/test_validate_plan_build_ids_warning.py b/tests/cli/test_validate_plan_build_ids_warning.py new file mode 100644 index 00000000..5fb393bf --- /dev/null +++ b/tests/cli/test_validate_plan_build_ids_warning.py @@ -0,0 +1,229 @@ +# Copyright 2024-2026 Agentics Transformation Ltd +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""``fluid validate`` and ``fluid plan`` warn about an id the build phase refuses. + +The schema's ``$defs.identifier`` admits ids the build phase refuses (``1orders``, +or an id longer than 128 characters), and the two grammars stay different by +decision. ``fluid apply`` in a build mode refuses such an id before it applies +anything (``test_apply_build_ids_before_tofu.py``). Validate and plan used to say +nothing about it. Now validate warns, so ``--strict`` fails, and plan warns and +still writes the plan, whatever its ``--mode``. The warning comes from the rule +apply uses, ``build_runners._ids.validate_build_ids``, so the three agree on +which ids are refused. +""" + +from __future__ import annotations + +import copy +import json +import logging +from pathlib import Path +from types import SimpleNamespace +from typing import Any, Dict + +import pytest +import yaml + +import fluid_build.cli as cli_mod +from fluid_build.build_runners import _ids +from fluid_build.cli.plan import _gate_contract_for_plan_or_apply, _warn_unusable_build_ids +from fluid_build.cli.plan import run as plan_run +from fluid_build.cli.validate import _run_contract_rules +from fluid_build.cli.validate import run as validate_run +from fluid_build.schema_manager import ValidationResult +from tests.cli.test_apply_build_ids_before_tofu import _CONTRACT, _UNUSABLE, _with + +LOG = logging.getLogger("test.validate_plan_build_ids_warning") + +# What the warning says besides the id, in the words an author acts on. +_RULE = "start with a letter or underscore" +_REFUSED_BY = "fluid apply in a build mode" + + +def _squash(text: str) -> str: + """``text`` without whitespace: Rich folds a long id across lines.""" + return "".join(text.split()) + + +def _names(out: str, field: str, value: str) -> bool: + return _squash(f"{field} {value!r}") in _squash(out) + + +def _without_builds(contract: Dict[str, Any]) -> Dict[str, Any]: + contract = copy.deepcopy(contract) + del contract["builds"] + return contract + + +@pytest.fixture +def workspace(tmp_path: Path, monkeypatch: pytest.MonkeyPatch) -> Path: + monkeypatch.chdir(tmp_path) + monkeypatch.delenv("FLUID_ENV", raising=False) + return tmp_path + + +def _write(contract: Dict[str, Any]) -> str: + path = Path("contract.fluid.yaml") + path.write_text(yaml.safe_dump(contract, sort_keys=False), encoding="utf-8") + return str(path) + + +def _validate(contract: Dict[str, Any], *flags: str) -> int: + """``fluid validate contract.fluid.yaml --offline `` through the real parser.""" + argv = ["validate", _write(contract), "--offline", *flags] + return validate_run(cli_mod.build_parser().parse_args(argv), LOG) + + +def _plan(contract: Dict[str, Any], *flags: str) -> int: + """``fluid plan contract.fluid.yaml --out plan.json `` through the real parser.""" + argv = ["plan", _write(contract), "--out", "plan.json", *flags] + return plan_run(cli_mod.build_parser().parse_args(argv), LOG) + + +class TestTheWarning: + @pytest.mark.parametrize("field,value", _UNUSABLE) + def test_it_names_the_id_the_rule_and_the_refusal(self, field: str, value: str) -> None: + message = _ids.build_id_warning(_with(field, value)) + assert message is not None + assert message.startswith(f"{field} {value!r} ") + assert _RULE in message + assert "'_', '.' and '-'" in message + assert "at most 128 characters" in message + assert _REFUSED_BY in message + + def test_usable_ids_get_none(self) -> None: + assert _ids.build_id_warning(copy.deepcopy(_CONTRACT)) is None + + @pytest.mark.parametrize( + "contract", + [ + pytest.param(_without_builds(_with("contract.id", "1orders")), id="no-builds"), + pytest.param({"id": "1orders", "builds": []}, id="empty-builds"), + pytest.param({"id": "", "builds": [{"id": ""}]}, id="empty-ids"), + pytest.param({"builds": [{"engine": "sql"}]}, id="absent-ids"), + pytest.param({"id": "orders", "builds": ["1make_rows", None]}, id="non-mapping"), + ], + ) + def test_what_apply_does_not_check_gets_none(self, contract: Dict[str, Any]) -> None: + """The ids ``validate_build_ids`` skips are not warned about either.""" + assert _ids.build_id_warning(contract) is None + + +@pytest.mark.usefixtures("workspace") +class TestValidate: + @pytest.mark.parametrize("field,value", _UNUSABLE) + def test_plain_validate_warns_and_passes( + self, capsys: pytest.CaptureFixture[str], field: str, value: str + ) -> None: + assert _validate(_with(field, value)) == 0 + out = capsys.readouterr().out + assert _names(out, field, value) + assert _squash(_REFUSED_BY) in _squash(out) + + @pytest.mark.parametrize("field,value", _UNUSABLE) + def test_strict_fails(self, capsys: pytest.CaptureFixture[str], field: str, value: str) -> None: + assert _validate(_with(field, value), "--strict") == 1 + out = " ".join(capsys.readouterr().out.split()) + assert "warnings treated as errors (--strict)" in out + + def test_json_lists_the_warning(self, capsys: pytest.CaptureFixture[str]) -> None: + assert _validate(_with("contract.id", "1orders"), "--format", "json") == 0 + report = json.loads(capsys.readouterr().out) + assert report["valid"] is True + assert [w for w in report["warnings"] if w.startswith("contract.id '1orders' ")] + + def test_usable_ids_pass_strict(self) -> None: + """The base contract is clean, so the warning is what fails ``--strict`` above.""" + assert _validate(copy.deepcopy(_CONTRACT), "--strict") == 0 + + def test_a_contract_without_builds_is_not_warned(self) -> None: + assert _validate(_without_builds(_with("contract.id", "1orders")), "--strict") == 0 + + def test_a_schema_refused_id_is_not_called_schema_valid( + self, capsys: pytest.CaptureFixture[str] + ) -> None: + """Contract rules also run on a contract the schema refused, so the warning + must not claim the schema accepted the id.""" + assert _validate(_with("contract.id", 123)) == 1 + out = capsys.readouterr().out + assert _squash("contract.id 123 cannot be used by the build phase") in _squash(out) + assert "schema allows" not in " ".join(out.split()) + + def test_a_crash_in_the_check_is_reported(self, monkeypatch: pytest.MonkeyPatch) -> None: + def boom(contract: Any) -> None: + raise RuntimeError("boom") + + monkeypatch.setattr(_ids, "build_id_warning", boom) + result = ValidationResult(is_valid=True) + args = SimpleNamespace(verbose=False, quiet=True, strict=False, offline=True) + _run_contract_rules(copy.deepcopy(_CONTRACT), None, result, args, LOG) + assert [w for w in result.warnings if w.startswith("Build-id check could not run")] + + +@pytest.mark.usefixtures("workspace") +class TestPlan: + @pytest.mark.parametrize("field,value", _UNUSABLE) + def test_plan_warns_and_writes_the_plan( + self, capsys: pytest.CaptureFixture[str], field: str, value: str + ) -> None: + assert _plan(_with(field, value)) == 0 + assert Path("plan.json").is_file() + out = capsys.readouterr().out + assert _names(out, field, value) + assert _squash(_REFUSED_BY) in _squash(out) + + @pytest.mark.parametrize("mode", ["amend", "amend-and-build", "replace-and-build"]) + def test_every_mode_only_warns(self, capsys: pytest.CaptureFixture[str], mode: str) -> None: + """Even the build modes whose apply will refuse it (founder's call).""" + assert _plan(_with("contract.id", "1orders"), "--mode", mode) == 0 + assert json.loads(Path("plan.json").read_text(encoding="utf-8"))["mode"] == mode + assert _names(capsys.readouterr().out, "contract.id", "1orders") + + def test_it_is_printed_once( + self, capsys: pytest.CaptureFixture[str], caplog: pytest.LogCaptureFixture + ) -> None: + """Not also as a log event, which the console prints again as a JSON line.""" + with caplog.at_level(logging.DEBUG): + assert _plan(_with("build.id", "1make_rows")) == 0 + captured = capsys.readouterr() + assert _squash(captured.out + captured.err).count(_squash(_REFUSED_BY)) == 1 + assert not [r for r in caplog.records if _REFUSED_BY in r.getMessage()] + + def test_a_marked_up_id_is_printed_as_written(self, capsys: pytest.CaptureFixture[str]) -> None: + """The id is contract data: Rich must not read ``[/]`` as a tag. The schema + refuses ``[`` today, so this calls the warning past the gate.""" + _warn_unusable_build_ids(_with("build.id", "[/]make_rows")) + assert _names(capsys.readouterr().out, "build.id", "[/]make_rows") + + def test_usable_ids_are_not_warned(self, capsys: pytest.CaptureFixture[str]) -> None: + assert _plan(copy.deepcopy(_CONTRACT)) == 0 + assert _squash(_REFUSED_BY) not in _squash(capsys.readouterr().out) + + def test_a_contract_without_builds_is_not_warned( + self, capsys: pytest.CaptureFixture[str] + ) -> None: + assert _plan(_without_builds(_with("contract.id", "1orders"))) == 0 + assert _squash(_REFUSED_BY) not in _squash(capsys.readouterr().out) + + +def test_the_gate_apply_shares_with_plan_does_not_warn( + capsys: pytest.CaptureFixture[str], caplog: pytest.LogCaptureFixture +) -> None: + """``fluid apply`` refuses the id in a build mode (``CLIError``) and ignores it in + the others, so the warning stays out of the gate plan shares with apply.""" + with caplog.at_level(logging.DEBUG, logger=LOG.name): + _gate_contract_for_plan_or_apply(_with("contract.id", "1orders"), LOG, command="apply") + assert _squash(_REFUSED_BY) not in _squash(capsys.readouterr().out) + assert not [r for r in caplog.records if "build_identifier" in r.getMessage()]