From 7d6de63cc4f1e1bd93dba457f9582b4e237b93ee Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sat, 18 Jul 2026 23:00:40 -0400 Subject: [PATCH 01/19] Document Python code generator design --- designs/codegen/cli.md | 95 ++++++++++++++++++++++++++++++++++++++++ designs/codegen/index.md | 60 +++++++++++++++++++++++++ 2 files changed, 155 insertions(+) create mode 100644 designs/codegen/cli.md create mode 100644 designs/codegen/index.md diff --git a/designs/codegen/cli.md b/designs/codegen/cli.md new file mode 100644 index 000000000..dec4395fa --- /dev/null +++ b/designs/codegen/cli.md @@ -0,0 +1,95 @@ +# Code Generator CLI + +The `smithy-python` command is the process interface described in the +[Python Code Generation](index.md) overview. It supports direct use and +invocation from Smithy's +[`run` plugin](https://smithy.io/2.0/guides/smithy-build-json.html#run-plugin). + +## Commands + +Generation is organized by artifact type: + +```console +smithy-python generate client [OPTIONS] +smithy-python generate types [OPTIONS] +``` + +`client` generates a service client and its required types. `types` generates a +standalone types package. Both commands accept the following process options: + +* `--model PATH` reads a JSON AST from a file instead of standard input. +* `--output PATH` selects the output directory for direct invocation. + +Settings specific to each artifact will be added with the functionality that +consumes them. + +The command MUST return zero after successful generation and non-zero when +arguments, settings, the model, or generation are invalid. Diagnostics are +written to standard error. Invalid command syntax and invocation inputs return +2, while I/O and generation failures return 1. + +## Smithy `run` Plugin + +The Smithy `run` plugin executes an external program during a build. It sends the +projection's Smithy model as a JSON AST to the process's standard input and runs +the process in the plugin's output directory. + +A plugin ID MUST use `run::` followed by a custom artifact name. The configured +command identifies the artifact to generate: + +```json +{ + "version": "1.0", + "projections": { + "client": { + "plugins": { + "run::python-client": { + "command": ["smithy-python", "generate", "client"] + } + } + } + } +} +``` + +Artifact-specific options will be appended to `command` after they are defined. + +The `smithy-python` executable MUST be installed or otherwise available on the +Smithy process's `PATH`. Smithy passes no arguments other than those in +`command`. + +### Input and Output + +When invoked by Smithy, the CLI reads one JSON AST document from standard input. +The document represents the model after projection transforms have been applied. + +The presence of `SMITHY_PLUGIN_DIR` identifies an invocation by the `run` plugin. +Generated files are written beneath this directory, which Smithy also uses as the +process's working directory. The CLI MUST NOT write generated files outside it, +and `--model` and `--output` MUST NOT be used in this mode. + +The `run` plugin provides the following environment variables: + +| Name | Purpose | +|------|---------| +| `SMITHY_ROOT_DIR` | Root directory of the Smithy build. | +| `SMITHY_PLUGIN_DIR` | Output and working directory for the plugin. | +| `SMITHY_PROJECTION_NAME` | Name of the active projection. | +| `SMITHY_ARTIFACT_NAME` | Custom artifact name from the plugin ID. | +| `SMITHY_INCLUDES_PRELUDE` | Whether the JSON AST includes prelude shapes. | + +The CLI uses this context to interpret the model. Protocol and platform +integrations MAY also use it while generating files. + +Smithy omits prelude shapes by default. A build MAY set `sendPrelude` to `true` +in the `run` plugin configuration when those shapes are needed. + +## Direct Invocation + +When `SMITHY_PLUGIN_DIR` is absent, the CLI treats the command as a direct +invocation and requires `--output`. It follows the same +generation path as Smithy invocation and can read a JSON AST from a file instead +of standard input by using `--model`. When standard input is an interactive +terminal, `--model` is required so that an omitted input does not wait indefinitely +for input. This mode is intended for development, testing, and integration with +tools other than the Smithy CLI. diff --git a/designs/codegen/index.md b/designs/codegen/index.md new file mode 100644 index 000000000..c6924759d --- /dev/null +++ b/designs/codegen/index.md @@ -0,0 +1,60 @@ +# Python Code Generation + +Smithy Python currently generates clients with the Java implementation in +`codegen`. This document describes the Python code generator that will replace +that implementation over time. + +The Python generator is distributed as `smithy-python`. It is separate from the +runtime packages used by generated code, and is only needed while generating a +package. + +## Goals + +* Generate Python clients and standalone types packages from Smithy models. +* Integrate with standard Smithy builds without requiring a Java code generator. +* Provide extension points for protocol and platform-specific behavior. +* Produce code compatible with the existing Smithy Python runtime packages. +* Allow the Python and Java generators to coexist during migration. + +## Architecture + +The generator consumes a Smithy JSON AST and settings for an artifact. It loads +the model, applies artifact and protocol-specific behavior, and writes a Python +package. + +```text +Smithy JSON AST + settings + | + v + smithy-python generator + | + v + client or types package +``` + +Two artifact types are initially planned: + +* `client` will generate a service client and its required types. +* `types` will generate a standalone package of types selected from a model. + +The command-line interface is the generator's first entry point. Smithy's `run` +plugin invokes it as an external process, so the generator does not need to be +loaded into the Smithy CLI or implemented in Java. + +Generated packages MUST NOT depend on `smithy-python` at runtime. They MAY +depend on the handwritten runtime packages in this repository. + +## Migration + +The Java generator remains authoritative while the Python generator is under +development. Features may be implemented and reviewed incrementally without +changing the Java path. A generated artifact SHOULD move to the Python generator +only after the required behavior is supported and tested. + +The Python generator does not need to reproduce Java implementation details or +byte-for-byte output. It MUST preserve the supported Smithy semantics and public +behavior of generated packages. + +## Designs + +* [Code Generator CLI](cli.md) From bc7ff3d6153a69f7aefc916c4ca5e602a6a76a46 Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sat, 18 Jul 2026 23:01:30 -0400 Subject: [PATCH 02/19] Add experimental Python codegen CLI --- README.md | 11 +- .../smithy-python-feature-codegen-cli.json | 4 + packages/smithy-python/CHANGELOG.md | 1 + packages/smithy-python/NOTICE | 1 + packages/smithy-python/README.md | 19 ++ packages/smithy-python/pyproject.toml | 51 ++++ .../src/smithy_python/__init__.py | 5 + .../src/smithy_python/__main__.py | 7 + .../smithy-python/src/smithy_python/cli.py | 140 +++++++++++ .../src/smithy_python/environment.py | 40 ++++ .../src/smithy_python/exceptions.py | 15 ++ .../smithy-python/src/smithy_python/py.typed | 0 packages/smithy-python/tests/unit/__init__.py | 2 + packages/smithy-python/tests/unit/test_cli.py | 225 ++++++++++++++++++ .../tests/unit/test_environment.py | 43 ++++ .../tests/unit/test_exceptions.py | 14 ++ pyproject.toml | 3 +- uv.lock | 5 + 18 files changed, 582 insertions(+), 4 deletions(-) create mode 100644 packages/smithy-python/.changes/next-release/smithy-python-feature-codegen-cli.json create mode 100644 packages/smithy-python/CHANGELOG.md create mode 100644 packages/smithy-python/NOTICE create mode 100644 packages/smithy-python/README.md create mode 100644 packages/smithy-python/pyproject.toml create mode 100644 packages/smithy-python/src/smithy_python/__init__.py create mode 100644 packages/smithy-python/src/smithy_python/__main__.py create mode 100644 packages/smithy-python/src/smithy_python/cli.py create mode 100644 packages/smithy-python/src/smithy_python/environment.py create mode 100644 packages/smithy-python/src/smithy_python/exceptions.py create mode 100644 packages/smithy-python/src/smithy_python/py.typed create mode 100644 packages/smithy-python/tests/unit/__init__.py create mode 100644 packages/smithy-python/tests/unit/test_cli.py create mode 100644 packages/smithy-python/tests/unit/test_environment.py create mode 100644 packages/smithy-python/tests/unit/test_exceptions.py diff --git a/README.md b/README.md index cbda5db4c..1d32c4d75 100644 --- a/README.md +++ b/README.md @@ -12,6 +12,11 @@ This code generator, and the clients it generates, are unstable and should not be used in production systems yet. Several features, such as detailed logging, have not been implemented yet. +> [!NOTE] +> The Java generator in `codegen` remains the authoritative implementation. +> `packages/smithy-python` contains an experimental Python-native CLI scaffold +> that does not generate code yet. + ### What is this repository? This repository contains two major components: @@ -20,9 +25,9 @@ This repository contains two major components: 2) Core modules and interfaces for building service clients in Python These components facilitate generating clients for any [Smithy](https://smithy.io/) -service. The `codegen` directory contains the source code for generating clients. -The `python-packages` directory contains the source code for the handwritten python -components. +service. The `codegen` directory contains the current Java generator, +`packages/smithy-python` contains the Python-native generator scaffold, and the +other directories under `packages` contain the handwritten Python components. This repository does *not* contain any generated clients, such as for S3 or other AWS services. Rather, these are the tools that facilitate the generation of those diff --git a/packages/smithy-python/.changes/next-release/smithy-python-feature-codegen-cli.json b/packages/smithy-python/.changes/next-release/smithy-python-feature-codegen-cli.json new file mode 100644 index 000000000..e9ed269f8 --- /dev/null +++ b/packages/smithy-python/.changes/next-release/smithy-python-feature-codegen-cli.json @@ -0,0 +1,4 @@ +{ + "type": "feature", + "description": "Added the experimental smithy-python package and CLI scaffold." +} diff --git a/packages/smithy-python/CHANGELOG.md b/packages/smithy-python/CHANGELOG.md new file mode 100644 index 000000000..825c32f0d --- /dev/null +++ b/packages/smithy-python/CHANGELOG.md @@ -0,0 +1 @@ +# Changelog diff --git a/packages/smithy-python/NOTICE b/packages/smithy-python/NOTICE new file mode 100644 index 000000000..616fc5889 --- /dev/null +++ b/packages/smithy-python/NOTICE @@ -0,0 +1 @@ +Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. diff --git a/packages/smithy-python/README.md b/packages/smithy-python/README.md new file mode 100644 index 000000000..9154bc542 --- /dev/null +++ b/packages/smithy-python/README.md @@ -0,0 +1,19 @@ +# smithy-python + +> [!WARNING] +> This package is an experimental scaffold. It does not generate code yet. The +> Java generator in the repository's `codegen` directory remains authoritative. + +`smithy-python` will provide Python-native code generation for Smithy models. +The initial command-line interface exposes the planned client and types generation +commands so that their top-level shape can be developed independently from the +generator implementation. + +```console +smithy-python generate client +smithy-python generate types +``` + +Both generation commands currently exit with an error explaining that generation +has not been implemented. The package is included in workspace builds to validate +its packaging and entry points. diff --git a/packages/smithy-python/pyproject.toml b/packages/smithy-python/pyproject.toml new file mode 100644 index 000000000..3bf5dccd0 --- /dev/null +++ b/packages/smithy-python/pyproject.toml @@ -0,0 +1,51 @@ +[project] +name = "smithy-python" +dynamic = ["version"] +requires-python = ">=3.12" +authors = [ + {name = "Amazon Web Services"}, +] +description = "A Smithy code generator for Python clients and types." +readme = "README.md" +license = {text = "Apache License 2.0"} +keywords = ["smithy", "codegen", "sdk"] +classifiers = [ + "Development Status :: 2 - Pre-Alpha", + "Intended Audience :: Developers", + "Natural Language :: English", + "License :: OSI Approved :: Apache Software License", + "Operating System :: OS Independent", + "Programming Language :: Python", + "Programming Language :: Python :: 3 :: Only", + "Programming Language :: Python :: 3", + "Programming Language :: Python :: 3.12", + "Programming Language :: Python :: 3.13", + "Programming Language :: Python :: 3.14", + "Programming Language :: Python :: Implementation :: CPython", + "Programming Language :: Python :: Free Threading :: 2 - Beta", + "Topic :: Software Development :: Code Generators", +] +dependencies = [] + +[project.scripts] +smithy-python = "smithy_python.cli:main" + +[project.urls] +"Changelog" = "https://github.com/smithy-lang/smithy-python/blob/develop/packages/smithy-python/CHANGELOG.md" +"Code" = "https://github.com/smithy-lang/smithy-python/tree/develop/packages/smithy-python/" +"Issue tracker" = "https://github.com/smithy-lang/smithy-python/issues" + +[build-system] +requires = ["hatchling"] +build-backend = "hatchling.build" + +[tool.hatch.version] +path = "src/smithy_python/__init__.py" + +[tool.hatch.build] +exclude = [ + "tests", +] + +[tool.ruff] +src = ["src"] diff --git a/packages/smithy-python/src/smithy_python/__init__.py b/packages/smithy-python/src/smithy_python/__init__.py new file mode 100644 index 000000000..c3621cb0c --- /dev/null +++ b/packages/smithy-python/src/smithy_python/__init__.py @@ -0,0 +1,5 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 +"""A Smithy code generator for Python clients and types.""" + +__version__ = "0.0.0" diff --git a/packages/smithy-python/src/smithy_python/__main__.py b/packages/smithy-python/src/smithy_python/__main__.py new file mode 100644 index 000000000..b8aaf8e60 --- /dev/null +++ b/packages/smithy-python/src/smithy_python/__main__.py @@ -0,0 +1,7 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +from .cli import main + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/packages/smithy-python/src/smithy_python/cli.py b/packages/smithy-python/src/smithy_python/cli.py new file mode 100644 index 000000000..7736312c8 --- /dev/null +++ b/packages/smithy-python/src/smithy_python/cli.py @@ -0,0 +1,140 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 +"""Command-line interface for the Smithy Python code generator.""" + +from __future__ import annotations + +import argparse +import os +import sys +from collections.abc import Mapping, Sequence +from dataclasses import dataclass +from pathlib import Path +from typing import BinaryIO, Final + +from . import __version__ +from .environment import PluginEnvironment +from .exceptions import CodegenError, InvalidInvocationError + +_GENERATION_NOT_IMPLEMENTED: Final = ( + "smithy-python: error: {artifact} generation is not implemented yet\n" +) + + +@dataclass(frozen=True, slots=True) +class _Invocation: + artifact: str + model_source: bytes + output_dir: Path + environment: PluginEnvironment + + +def main( + argv: Sequence[str] | None = None, + *, + environ: Mapping[str, str] | None = None, + stdin: BinaryIO | None = None, +) -> int: + """Run the CLI with the provided process inputs and return its exit code.""" + parser = _create_parser() + + try: + args = parser.parse_args(argv) + except SystemExit as error: + return error.code if isinstance(error.code, int) else 1 + + try: + _resolve_invocation( + args, + environ=os.environ if environ is None else environ, + stdin=stdin, + ) + except InvalidInvocationError as error: + sys.stderr.write(f"smithy-python: error: {error}\n") + return 2 + except (CodegenError, OSError) as error: + sys.stderr.write(f"smithy-python: error: {error}\n") + return 1 + + sys.stderr.write(_GENERATION_NOT_IMPLEMENTED.format(artifact=args.artifact)) + return 1 + + +def _create_parser() -> argparse.ArgumentParser: + parser = argparse.ArgumentParser( + prog="smithy-python", + description="Generate Python source from Smithy models.", + ) + parser.add_argument( + "--version", + action="version", + version=f"%(prog)s {__version__}", + ) + + commands = parser.add_subparsers(required=True) + generate = commands.add_parser("generate", help="Generate Python source") + artifacts = generate.add_subparsers(dest="artifact", required=True) + for name, help_text in ( + ("client", "Generate a client package"), + ("types", "Generate a standalone types package"), + ): + artifact = artifacts.add_parser(name, help=help_text) + artifact.add_argument( + "--model", + type=Path, + help="Read the JSON AST from a file instead of standard input", + ) + artifact.add_argument( + "--output", + type=Path, + help="Output directory for direct invocation", + ) + + return parser + + +def _resolve_invocation( + args: argparse.Namespace, + *, + environ: Mapping[str, str], + stdin: BinaryIO | None, +) -> _Invocation: + environment = PluginEnvironment.from_environ(environ) + model_path: Path | None = args.model + output_path: Path | None = args.output + + if (plugin_dir := environment.plugin_dir) is not None: + if model_path is not None: + raise InvalidInvocationError( + "--model cannot be used with the Smithy run plugin" + ) + if output_path is not None: + raise InvalidInvocationError( + "--output cannot be used with the Smithy run plugin" + ) + output_dir = plugin_dir + else: + if output_path is None: + raise InvalidInvocationError("Direct invocation requires --output") + output_dir = output_path + + if model_path is not None: + if not model_path.is_file(): + raise InvalidInvocationError(f"Model path is not a file: {model_path}") + model_source = model_path.read_bytes() + else: + model_stream = sys.stdin.buffer if stdin is None else stdin + if environment.plugin_dir is None and model_stream.isatty(): + raise InvalidInvocationError( + "Direct invocation requires --model or a model piped to standard input" + ) + model_source = model_stream.read() + if not model_source: + raise InvalidInvocationError("Expected a Smithy JSON AST model") + + return _Invocation( + artifact=args.artifact, + model_source=model_source, + output_dir=output_dir, + environment=environment, + ) diff --git a/packages/smithy-python/src/smithy_python/environment.py b/packages/smithy-python/src/smithy_python/environment.py new file mode 100644 index 000000000..5137519ae --- /dev/null +++ b/packages/smithy-python/src/smithy_python/environment.py @@ -0,0 +1,40 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 +"""Smithy build environment provided to the code generator.""" + +from __future__ import annotations + +import os +from collections.abc import Mapping +from dataclasses import dataclass +from pathlib import Path +from typing import Self + + +@dataclass(frozen=True, slots=True) +class PluginEnvironment: + """Environment values supplied by Smithy's process-based run plugin.""" + + root_dir: Path | None = None + plugin_dir: Path | None = None + projection_name: str | None = None + artifact_name: str | None = None + includes_prelude: bool = False + + @classmethod + def from_environ(cls, environ: Mapping[str, str] | None = None) -> Self: + """Load the Smithy run plugin environment from a mapping.""" + + source = os.environ if environ is None else environ + + def path(name: str) -> Path | None: + return Path(value) if (value := source.get(name)) else None + + return cls( + root_dir=path("SMITHY_ROOT_DIR"), + plugin_dir=path("SMITHY_PLUGIN_DIR"), + projection_name=source.get("SMITHY_PROJECTION_NAME"), + artifact_name=source.get("SMITHY_ARTIFACT_NAME"), + includes_prelude=source.get("SMITHY_INCLUDES_PRELUDE", "false").lower() + == "true", + ) diff --git a/packages/smithy-python/src/smithy_python/exceptions.py b/packages/smithy-python/src/smithy_python/exceptions.py new file mode 100644 index 000000000..a0cff3f93 --- /dev/null +++ b/packages/smithy-python/src/smithy_python/exceptions.py @@ -0,0 +1,15 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 +"""Exceptions raised by Smithy Python code generation.""" + + +class SmithyPythonError(Exception): + """Base exception for errors raised by the Smithy Python generator.""" + + +class CodegenError(SmithyPythonError): + """Raised when code generation fails.""" + + +class InvalidInvocationError(SmithyPythonError): + """Raised when command-line inputs do not form a valid invocation.""" diff --git a/packages/smithy-python/src/smithy_python/py.typed b/packages/smithy-python/src/smithy_python/py.typed new file mode 100644 index 000000000..e69de29bb diff --git a/packages/smithy-python/tests/unit/__init__.py b/packages/smithy-python/tests/unit/__init__.py new file mode 100644 index 000000000..04f8b7b76 --- /dev/null +++ b/packages/smithy-python/tests/unit/__init__.py @@ -0,0 +1,2 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 diff --git a/packages/smithy-python/tests/unit/test_cli.py b/packages/smithy-python/tests/unit/test_cli.py new file mode 100644 index 000000000..a82c87a1a --- /dev/null +++ b/packages/smithy-python/tests/unit/test_cli.py @@ -0,0 +1,225 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +from __future__ import annotations + +import importlib +from io import BytesIO +from pathlib import Path + +import pytest +from smithy_python import __version__ +from smithy_python.cli import main + + +class _InteractiveStdin(BytesIO): + def isatty(self) -> bool: + return True + + +@pytest.mark.parametrize( + ("argv", "expected"), + [ + (("--help",), "usage: smithy-python"), + (("--version",), f"smithy-python {__version__}"), + ], +) +def test_information_commands( + argv: tuple[str, ...], expected: str, capsys: pytest.CaptureFixture[str] +) -> None: + assert main(argv) == 0 + assert capsys.readouterr().out.startswith(expected) + + +@pytest.mark.parametrize("artifact", ["client", "types"]) +def test_generation_commands_are_explicitly_unavailable( + artifact: str, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + model = tmp_path / "model.json" + model.write_text("{}") + + assert ( + main( + ( + "generate", + artifact, + "--model", + str(model), + "--output", + str(tmp_path / "output"), + ), + environ={}, + ) + == 1 + ) + assert capsys.readouterr().err == ( + f"smithy-python: error: {artifact} generation is not implemented yet\n" + ) + + +@pytest.mark.parametrize( + ("argv", "expected_usage"), + [ + ((), "usage: smithy-python"), + (("generate",), "usage: smithy-python generate"), + ], +) +def test_missing_command_identifies_available_subcommands( + argv: tuple[str, ...], + expected_usage: str, + capsys: pytest.CaptureFixture[str], +) -> None: + assert main(argv) == 2 + assert capsys.readouterr().err.startswith(expected_usage) + + +def test_main_module_can_be_imported() -> None: + importlib.import_module("smithy_python.__main__") + + +def test_run_plugin_invocation_reads_standard_input( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + assert ( + main( + ("generate", "client"), + environ={"SMITHY_PLUGIN_DIR": str(tmp_path)}, + stdin=BytesIO(b"{}"), + ) + == 1 + ) + assert "generation is not implemented yet" in capsys.readouterr().err + + +@pytest.mark.parametrize("option", ["--model", "--output"]) +def test_run_plugin_rejects_direct_invocation_options( + option: str, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + assert ( + main( + ("generate", "client", option, str(tmp_path / "value")), + environ={"SMITHY_PLUGIN_DIR": str(tmp_path)}, + stdin=BytesIO(b"{}"), + ) + == 2 + ) + assert f"{option} cannot be used with the Smithy run plugin" in ( + capsys.readouterr().err + ) + + +def test_direct_invocation_requires_output( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + model = tmp_path / "model.json" + model.write_text("{}") + + assert main(("generate", "client", "--model", str(model)), environ={}) == 2 + assert "Direct invocation requires --output" in capsys.readouterr().err + + +def test_invocation_rejects_empty_model( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + assert ( + main( + ("generate", "client", "--output", str(tmp_path)), + environ={}, + stdin=BytesIO(), + ) + == 2 + ) + assert "Expected a Smithy JSON AST model" in capsys.readouterr().err + + +def test_direct_invocation_rejects_interactive_model_input( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + assert ( + main( + ("generate", "client", "--output", str(tmp_path)), + environ={}, + stdin=_InteractiveStdin(), + ) + == 2 + ) + assert ( + "Direct invocation requires --model or a model piped to standard input" + in capsys.readouterr().err + ) + + +def test_invocation_reports_unreadable_model( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + missing = tmp_path / "missing.json" + + assert ( + main( + ( + "generate", + "client", + "--model", + str(missing), + "--output", + str(tmp_path), + ), + environ={}, + ) + == 2 + ) + assert f"Model path is not a file: {missing}" in capsys.readouterr().err + + +def test_invocation_rejects_empty_model_path( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + assert ( + main( + ( + "generate", + "client", + "--model", + "", + "--output", + str(tmp_path), + ), + environ={}, + ) + == 2 + ) + assert "Model path is not a file: ." in capsys.readouterr().err + + +def test_invocation_reports_model_io_error( + monkeypatch: pytest.MonkeyPatch, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + model = tmp_path / "model.json" + model.write_text("{}") + + def raise_io_error(self: Path) -> bytes: + raise OSError("unable to read model") + + monkeypatch.setattr(Path, "read_bytes", raise_io_error) + + assert ( + main( + ( + "generate", + "client", + "--model", + str(model), + "--output", + str(tmp_path), + ), + environ={}, + ) + == 1 + ) + assert "unable to read model" in capsys.readouterr().err diff --git a/packages/smithy-python/tests/unit/test_environment.py b/packages/smithy-python/tests/unit/test_environment.py new file mode 100644 index 000000000..ea8e270fd --- /dev/null +++ b/packages/smithy-python/tests/unit/test_environment.py @@ -0,0 +1,43 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +from pathlib import Path + +import pytest +from smithy_python.environment import PluginEnvironment + + +def test_loads_run_plugin_environment() -> None: + environment = PluginEnvironment.from_environ( + { + "SMITHY_ROOT_DIR": "/tmp/root", + "SMITHY_PLUGIN_DIR": "/tmp/plugin", + "SMITHY_PROJECTION_NAME": "client", + "SMITHY_ARTIFACT_NAME": "python-client", + "SMITHY_INCLUDES_PRELUDE": "true", + } + ) + + assert environment.root_dir == Path("/tmp/root") + assert environment.plugin_dir == Path("/tmp/plugin") + assert environment.projection_name == "client" + assert environment.artifact_name == "python-client" + assert environment.includes_prelude + + +def test_defaults_to_direct_invocation() -> None: + environment = PluginEnvironment.from_environ({}) + + assert environment.root_dir is None + assert environment.plugin_dir is None + assert environment.projection_name is None + assert environment.artifact_name is None + assert not environment.includes_prelude + + +def test_loads_os_environment_by_default( + monkeypatch: pytest.MonkeyPatch, tmp_path: Path +) -> None: + monkeypatch.setenv("SMITHY_PLUGIN_DIR", str(tmp_path)) + + assert PluginEnvironment.from_environ().plugin_dir == tmp_path diff --git a/packages/smithy-python/tests/unit/test_exceptions.py b/packages/smithy-python/tests/unit/test_exceptions.py new file mode 100644 index 000000000..bd1867f56 --- /dev/null +++ b/packages/smithy-python/tests/unit/test_exceptions.py @@ -0,0 +1,14 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +from smithy_python.exceptions import ( + CodegenError, + InvalidInvocationError, + SmithyPythonError, +) + + +def test_error_hierarchy_distinguishes_invocation_and_codegen_failures() -> None: + assert issubclass(CodegenError, SmithyPythonError) + assert issubclass(InvalidInvocationError, SmithyPythonError) + assert not issubclass(InvalidInvocationError, CodegenError) diff --git a/pyproject.toml b/pyproject.toml index 152cf5f61..42a463e81 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,5 +1,5 @@ [project] -name = "smithy-python" +name = "smithy-python-workspace" version = "0.1.0" description = "Add your description here" readme = "README.md" @@ -39,6 +39,7 @@ smithy_xml = { workspace = true } smithy_aws_core = { workspace = true } smithy_aws_event_stream = { workspace = true } aws_sdk_signers = {workspace = true } +smithy_python = { workspace = true } [tool.pyright] typeCheckingMode = "strict" diff --git a/uv.lock b/uv.lock index f55a3a435..59d64a67c 100644 --- a/uv.lock +++ b/uv.lock @@ -11,6 +11,7 @@ members = [ "smithy-http", "smithy-json", "smithy-python", + "smithy-python-workspace", "smithy-xml", ] @@ -776,6 +777,10 @@ requires-dist = [ [[package]] name = "smithy-python" +source = { editable = "packages/smithy-python" } + +[[package]] +name = "smithy-python-workspace" version = "0.1.0" source = { virtual = "." } From cbe477ccfaa274d1d69fa7f67aa5f7c6b3f7f8c9 Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sat, 18 Jul 2026 23:21:24 -0400 Subject: [PATCH 03/19] Remove redundant exception hierarchy test --- .../smithy-python/tests/unit/test_exceptions.py | 14 -------------- 1 file changed, 14 deletions(-) delete mode 100644 packages/smithy-python/tests/unit/test_exceptions.py diff --git a/packages/smithy-python/tests/unit/test_exceptions.py b/packages/smithy-python/tests/unit/test_exceptions.py deleted file mode 100644 index bd1867f56..000000000 --- a/packages/smithy-python/tests/unit/test_exceptions.py +++ /dev/null @@ -1,14 +0,0 @@ -# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. -# SPDX-License-Identifier: Apache-2.0 - -from smithy_python.exceptions import ( - CodegenError, - InvalidInvocationError, - SmithyPythonError, -) - - -def test_error_hierarchy_distinguishes_invocation_and_codegen_failures() -> None: - assert issubclass(CodegenError, SmithyPythonError) - assert issubclass(InvalidInvocationError, SmithyPythonError) - assert not issubclass(InvalidInvocationError, CodegenError) From 0f09ecee9276c20c209327fdef36df44fa18b451 Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sun, 19 Jul 2026 17:23:54 -0400 Subject: [PATCH 04/19] Clarify CLI examples and test module entry point Mark generation command examples as schematic and clarify option validation behavior. Exercise python -m smithy_python in a subprocess. --- packages/smithy-python/README.md | 10 +++++----- packages/smithy-python/tests/unit/test_cli.py | 16 +++++++++++++--- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/packages/smithy-python/README.md b/packages/smithy-python/README.md index 9154bc542..79db2d4bc 100644 --- a/packages/smithy-python/README.md +++ b/packages/smithy-python/README.md @@ -10,10 +10,10 @@ commands so that their top-level shape can be developed independently from the generator implementation. ```console -smithy-python generate client -smithy-python generate types +smithy-python generate client [OPTIONS] +smithy-python generate types [OPTIONS] ``` -Both generation commands currently exit with an error explaining that generation -has not been implemented. The package is included in workspace builds to validate -its packaging and entry points. +After validating their invocation options, both generation commands currently exit +with an error explaining that generation has not been implemented. The package is +included in workspace builds to validate its packaging and entry points. diff --git a/packages/smithy-python/tests/unit/test_cli.py b/packages/smithy-python/tests/unit/test_cli.py index a82c87a1a..74a2418cb 100644 --- a/packages/smithy-python/tests/unit/test_cli.py +++ b/packages/smithy-python/tests/unit/test_cli.py @@ -3,7 +3,8 @@ from __future__ import annotations -import importlib +import subprocess +import sys from io import BytesIO from pathlib import Path @@ -75,8 +76,17 @@ def test_missing_command_identifies_available_subcommands( assert capsys.readouterr().err.startswith(expected_usage) -def test_main_module_can_be_imported() -> None: - importlib.import_module("smithy_python.__main__") +def test_main_module_invokes_cli() -> None: + result = subprocess.run( + [sys.executable, "-m", "smithy_python", "--version"], + capture_output=True, + check=False, + text=True, + ) + + assert result.returncode == 0 + assert result.stdout == f"smithy-python {__version__}\n" + assert result.stderr == "" def test_run_plugin_invocation_reads_standard_input( From ba91974bc9ff273173dd44328d22bba1f88ed453 Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Tue, 21 Jul 2026 23:48:01 -0400 Subject: [PATCH 05/19] Address PR feedback --- designs/codegen/cli.md | 3 +- designs/codegen/index.md | 3 ++ .../smithy-python/src/smithy_python/cli.py | 35 +++++++++++++------ 3 files changed, 29 insertions(+), 12 deletions(-) diff --git a/designs/codegen/cli.md b/designs/codegen/cli.md index dec4395fa..99562194f 100644 --- a/designs/codegen/cli.md +++ b/designs/codegen/cli.md @@ -18,7 +18,8 @@ smithy-python generate types [OPTIONS] standalone types package. Both commands accept the following process options: * `--model PATH` reads a JSON AST from a file instead of standard input. -* `--output PATH` selects the output directory for direct invocation. +* `--output PATH` selects the output directory. It defaults to the Smithy run + plugin's output directory (`SMITHY_PLUGIN_DIR`) when invoked by Smithy. Settings specific to each artifact will be added with the functionality that consumes them. diff --git a/designs/codegen/index.md b/designs/codegen/index.md index c6924759d..9901b8be1 100644 --- a/designs/codegen/index.md +++ b/designs/codegen/index.md @@ -37,6 +37,9 @@ Two artifact types are initially planned: * `client` will generate a service client and its required types. * `types` will generate a standalone package of types selected from a model. +The artifact set may grow over time. A `server` artifact is a natural addition, +so the generator should not assume that only `client` and `types` exist. + The command-line interface is the generator's first entry point. Smithy's `run` plugin invokes it as an external process, so the generator does not need to be loaded into the Smithy CLI or implemented in Java. diff --git a/packages/smithy-python/src/smithy_python/cli.py b/packages/smithy-python/src/smithy_python/cli.py index 7736312c8..79cf26a62 100644 --- a/packages/smithy-python/src/smithy_python/cli.py +++ b/packages/smithy-python/src/smithy_python/cli.py @@ -74,22 +74,35 @@ def _create_parser() -> argparse.ArgumentParser: commands = parser.add_subparsers(required=True) generate = commands.add_parser("generate", help="Generate Python source") artifacts = generate.add_subparsers(dest="artifact", required=True) + common = _common_artifact_options() for name, help_text in ( ("client", "Generate a client package"), ("types", "Generate a standalone types package"), ): - artifact = artifacts.add_parser(name, help=help_text) - artifact.add_argument( - "--model", - type=Path, - help="Read the JSON AST from a file instead of standard input", - ) - artifact.add_argument( - "--output", - type=Path, - help="Output directory for direct invocation", - ) + artifacts.add_parser(name, help=help_text, parents=[common]) + + return parser + +def _common_artifact_options() -> argparse.ArgumentParser: + """Build a parent parser with the options shared by every artifact.""" + parser = argparse.ArgumentParser(add_help=False) + parser.add_argument( + "--model", + type=Path, + help=( + "The Smithy JSON AST model file to use for code generation. " + "If not set, the model is read from standard input." + ), + ) + parser.add_argument( + "--output", + type=Path, + help=( + "Output directory for generated files. Defaults to the Smithy run " + "plugin's output directory (SMITHY_PLUGIN_DIR) when invoked by Smithy." + ), + ) return parser From 05ea279dee6109056e258abc2acfe80663a024f4 Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sat, 12 Sep 2026 01:40:16 -0400 Subject: [PATCH 06/19] docs(codegen): Clarify service selection and generated shapes Document that --service is optional when the model contains a single service, that both artifacts generate every data shape in the model rather than the service closure, and that case-insensitive name conflicts are a hard error. Note that run plugin env settings may back command-line options. --- designs/codegen/cli.md | 35 +++++++++++++++++++++++++++++++++-- 1 file changed, 33 insertions(+), 2 deletions(-) diff --git a/designs/codegen/cli.md b/designs/codegen/cli.md index 99562194f..241436dbc 100644 --- a/designs/codegen/cli.md +++ b/designs/codegen/cli.md @@ -14,8 +14,9 @@ smithy-python generate client [OPTIONS] smithy-python generate types [OPTIONS] ``` -`client` generates a service client and its required types. `types` generates a -standalone types package. Both commands accept the following process options: +`client` generates a service client together with the data shapes in the model. +`types` generates a standalone package containing only the data shapes. Both +commands accept the following process options: * `--model PATH` reads a JSON AST from a file instead of standard input. * `--output PATH` selects the output directory. It defaults to the Smithy run @@ -24,6 +25,33 @@ standalone types package. Both commands accept the following process options: Settings specific to each artifact will be added with the functionality that consumes them. +### Service Selection + +The CLI does not require a service to be named. It resolves the service to +generate as follows: + +* `--service SHAPE_ID` selects a specific service shape. The shape MUST exist in + the model and MUST be a service. +* When `--service` is omitted and the model contains exactly one service shape, + that service is used. +* When `--service` is omitted and the model contains more than one service + shape, the command fails with an invocation error that lists the candidates. + +The `client` artifact requires a resolved service. The `types` artifact does +not. The CLI MUST NOT synthesize a placeholder service to satisfy generation. + +### Generated Shapes + +Both artifacts generate every data shape in the model they receive; the set is +not narrowed to the closure of the selected service. Builds that want a smaller +package apply smithy-build transforms in the projection. Trait definitions, +prelude shapes, and shapes marked `@private` or `@mixin` are never generated. + +Because the model is not limited to a service closure, shape names are not +guaranteed to be unique. When two generated shapes have case-insensitively equal +names, the command fails with an error that identifies the conflicting shape +IDs. + The command MUST return zero after successful generation and non-zero when arguments, settings, the model, or generation are invalid. Diagnostics are written to standard error. Invalid command syntax and invocation inputs return @@ -54,6 +82,9 @@ command identifies the artifact to generate: ``` Artifact-specific options will be appended to `command` after they are defined. +The `run` plugin can also pass settings through its `env` property, so an option +MAY additionally be read from an environment variable. A command-line option +takes precedence over its environment variable. The `smithy-python` executable MUST be installed or otherwise available on the Smithy process's `PATH`. Smithy passes no arguments other than those in From b020d91a7f94ee321dbae34e8d59c095a0847107 Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sat, 12 Sep 2026 22:08:01 -0400 Subject: [PATCH 07/19] docs(codegen): Generate the service closure by default Generating every shape in the model diverged from every other Smithy generator and fails on a published AWS model whose leaked, unconnected shapes collide with real ones. Document the service closure as the default selection, with a note when shapes are left out, and keep the whole-model behavior for the types artifact when no service is present. --- designs/codegen/cli.md | 31 +++++++++++++++++++------------ 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/designs/codegen/cli.md b/designs/codegen/cli.md index 241436dbc..c01fb8f7c 100644 --- a/designs/codegen/cli.md +++ b/designs/codegen/cli.md @@ -14,9 +14,9 @@ smithy-python generate client [OPTIONS] smithy-python generate types [OPTIONS] ``` -`client` generates a service client together with the data shapes in the model. -`types` generates a standalone package containing only the data shapes. Both -commands accept the following process options: +`client` generates a service client and the data shapes it uses. `types` +generates a standalone package containing only data shapes. Both commands accept +the following process options: * `--model PATH` reads a JSON AST from a file instead of standard input. * `--output PATH` selects the output directory. It defaults to the Smithy run @@ -42,15 +42,22 @@ not. The CLI MUST NOT synthesize a placeholder service to satisfy generation. ### Generated Shapes -Both artifacts generate every data shape in the model they receive; the set is -not narrowed to the closure of the selected service. Builds that want a smaller -package apply smithy-build transforms in the projection. Trait definitions, -prelude shapes, and shapes marked `@private` or `@mixin` are never generated. - -Because the model is not limited to a service closure, shape names are not -guaranteed to be unique. When two generated shapes have case-insensitively equal -names, the command fails with an error that identifies the conflicting shape -IDs. +When a service is resolved, both artifacts generate the data shapes in the +service closure: every shape reachable from the service through its operations, +resources, errors, and members. This matches the surface produced by the other +Smithy code generators. Data shapes in the model that are not connected to the +service are not generated, and the CLI reports how many were left out. + +When no service is resolved, the `types` artifact generates every data shape in +the model. Smithy guarantees case-insensitively unique shape names only within a +service closure, so in this mode the command fails when two shapes have +case-insensitively equal names, identifying the conflicting shape IDs. + +Trait definitions, prelude shapes, and shapes marked `@mixin` are never +generated. Builds that need a different set of shapes, such as types that are +not bound to any operation, apply smithy-build transforms in the projection. +An option to generate every shape in the model regardless of the service MAY be +added when there is a need for it. The command MUST return zero after successful generation and non-zero when arguments, settings, the model, or generation are invalid. Diagnostics are From e6b30266a92a4797318e1030ff1542b6c42138e0 Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sat, 12 Sep 2026 22:23:53 -0400 Subject: [PATCH 08/19] docs(codegen): Document mixin resolution during model loading --- designs/codegen/cli.md | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/designs/codegen/cli.md b/designs/codegen/cli.md index c01fb8f7c..d84f5a8e9 100644 --- a/designs/codegen/cli.md +++ b/designs/codegen/cli.md @@ -102,6 +102,13 @@ Smithy process's `PATH`. Smithy passes no arguments other than those in When invoked by Smithy, the CLI reads one JSON AST document from standard input. The document represents the model after projection transforms have been applied. +Smithy serializes only what a shape introduces, so shapes that use mixins arrive +without their inherited members, traits, and properties, and traits added to +inherited members arrive as `apply` statements. The CLI resolves mixins while +loading the model, following the rules of the +[Smithy mixins specification](https://smithy.io/2.0/spec/mixins.html), so builds +do not need the `flattenAndRemoveMixins` transform. + The presence of `SMITHY_PLUGIN_DIR` identifies an invocation by the `run` plugin. Generated files are written beneath this directory, which Smithy also uses as the process's working directory. The CLI MUST NOT write generated files outside it, From 9574e52da12b06abf2ab9713c2588295492d5747 Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sat, 12 Sep 2026 19:17:24 -0400 Subject: [PATCH 09/19] feat(codegen): Load models and resolve the service to generate Port the JSON AST model loader from the Python-native proof of concept and add the selection rules described in the CLI design. The CLI now parses the model before reporting that generation is unimplemented. - Add ordered, immutable Model, Shape, Member, and ShapeID types that resolve prelude shapes on demand and honor apply statements. - Add an optional --service option. A model with one service selects it automatically; multiple services require an explicit choice; the client artifact requires a service and types does not. - Select every data shape in the model rather than the service closure, skipping prelude shapes, trait definitions, and mixins. Private shapes are generated only when reachable from another generated shape or an operation. Case-insensitive name conflicts fail generation. - Distinguish invocation errors (exit 2) from model and generation failures (exit 1). --- .../smithy-python-feature-model-loading.json | 4 + .../smithy-python/src/smithy_python/cli.py | 56 ++- .../src/smithy_python/exceptions.py | 4 + .../smithy-python/src/smithy_python/model.py | 440 ++++++++++++++++++ .../src/smithy_python/selection.py | 119 +++++ packages/smithy-python/tests/unit/conftest.py | 106 +++++ packages/smithy-python/tests/unit/test_cli.py | 140 +++++- .../smithy-python/tests/unit/test_model.py | 244 ++++++++++ .../tests/unit/test_selection.py | 143 ++++++ 9 files changed, 1251 insertions(+), 5 deletions(-) create mode 100644 packages/smithy-python/.changes/next-release/smithy-python-feature-model-loading.json create mode 100644 packages/smithy-python/src/smithy_python/model.py create mode 100644 packages/smithy-python/src/smithy_python/selection.py create mode 100644 packages/smithy-python/tests/unit/conftest.py create mode 100644 packages/smithy-python/tests/unit/test_model.py create mode 100644 packages/smithy-python/tests/unit/test_selection.py diff --git a/packages/smithy-python/.changes/next-release/smithy-python-feature-model-loading.json b/packages/smithy-python/.changes/next-release/smithy-python-feature-model-loading.json new file mode 100644 index 000000000..451777bc1 --- /dev/null +++ b/packages/smithy-python/.changes/next-release/smithy-python-feature-model-loading.json @@ -0,0 +1,4 @@ +{ + "type": "feature", + "description": "Added JSON AST model loading, service resolution via an optional `--service` option, and generated-shape selection with name-conflict detection." +} diff --git a/packages/smithy-python/src/smithy_python/cli.py b/packages/smithy-python/src/smithy_python/cli.py index 79cf26a62..6cf39045c 100644 --- a/packages/smithy-python/src/smithy_python/cli.py +++ b/packages/smithy-python/src/smithy_python/cli.py @@ -14,12 +14,16 @@ from . import __version__ from .environment import PluginEnvironment -from .exceptions import CodegenError, InvalidInvocationError +from .exceptions import CodegenError, InvalidInvocationError, ModelError +from .model import Model, Shape, ShapeID +from .selection import resolve_service, select_generated_shapes _GENERATION_NOT_IMPLEMENTED: Final = ( "smithy-python: error: {artifact} generation is not implemented yet\n" ) +_CLIENT_ARTIFACT: Final = "client" + @dataclass(frozen=True, slots=True) class _Invocation: @@ -27,6 +31,17 @@ class _Invocation: model_source: bytes output_dir: Path environment: PluginEnvironment + service: ShapeID | None + + +@dataclass(frozen=True, slots=True) +class _Request: + """A fully resolved generation request.""" + + invocation: _Invocation + model: Model + service: Shape | None + shapes: tuple[Shape, ...] def main( @@ -44,11 +59,12 @@ def main( return error.code if isinstance(error.code, int) else 1 try: - _resolve_invocation( + invocation = _resolve_invocation( args, environ=os.environ if environ is None else environ, stdin=stdin, ) + _resolve_request(invocation) except InvalidInvocationError as error: sys.stderr.write(f"smithy-python: error: {error}\n") return 2 @@ -60,6 +76,19 @@ def main( return 1 +def _resolve_request(invocation: _Invocation) -> _Request: + """Load the model and resolve what the artifact will generate.""" + # The raw bytes are dropped as soon as the model is parsed. + model = Model.from_json(invocation.model_source) + service = resolve_service( + model, + invocation.service, + required=invocation.artifact == _CLIENT_ARTIFACT, + ) + shapes = select_generated_shapes(model) + return _Request(invocation=invocation, model=model, service=service, shapes=shapes) + + def _create_parser() -> argparse.ArgumentParser: parser = argparse.ArgumentParser( prog="smithy-python", @@ -103,6 +132,14 @@ def _common_artifact_options() -> argparse.ArgumentParser: "plugin's output directory (SMITHY_PLUGIN_DIR) when invoked by Smithy." ), ) + parser.add_argument( + "--service", + metavar="SHAPE_ID", + help=( + "Absolute shape ID of the service to generate. Required only when the " + "model contains more than one service." + ), + ) return parser @@ -150,4 +187,19 @@ def _resolve_invocation( model_source=model_source, output_dir=output_dir, environment=environment, + service=_parse_service(args.service), ) + + +def _parse_service(value: str | None) -> ShapeID | None: + if value is None: + return None + try: + service = ShapeID.parse(value) + except ModelError as error: + raise InvalidInvocationError(f"Invalid --service value: {error}") from error + if service.member is not None: + raise InvalidInvocationError( + f"--service must identify a shape, not a member: {value}" + ) + return service diff --git a/packages/smithy-python/src/smithy_python/exceptions.py b/packages/smithy-python/src/smithy_python/exceptions.py index a0cff3f93..7c07f3575 100644 --- a/packages/smithy-python/src/smithy_python/exceptions.py +++ b/packages/smithy-python/src/smithy_python/exceptions.py @@ -11,5 +11,9 @@ class CodegenError(SmithyPythonError): """Raised when code generation fails.""" +class ModelError(CodegenError): + """Raised when a Smithy JSON AST model is invalid or unsupported.""" + + class InvalidInvocationError(SmithyPythonError): """Raised when command-line inputs do not form a valid invocation.""" diff --git a/packages/smithy-python/src/smithy_python/model.py b/packages/smithy-python/src/smithy_python/model.py new file mode 100644 index 000000000..7e316f977 --- /dev/null +++ b/packages/smithy-python/src/smithy_python/model.py @@ -0,0 +1,440 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 +"""Ordered, immutable objects for Smithy's JSON AST representation.""" + +from __future__ import annotations + +import json +import re +from collections.abc import Iterable, Iterator, Mapping +from dataclasses import dataclass, field, replace +from enum import StrEnum +from types import MappingProxyType +from typing import Self, cast + +from .exceptions import ModelError + +type JSONValue = ( + None | bool | int | float | str | list[JSONValue] | dict[str, JSONValue] +) + +PRELUDE_NAMESPACE = "smithy.api" + + +class ShapeType(StrEnum): + """Shape types supported by the Smithy JSON AST.""" + + BLOB = "blob" + BOOLEAN = "boolean" + STRING = "string" + TIMESTAMP = "timestamp" + BYTE = "byte" + SHORT = "short" + INTEGER = "integer" + LONG = "long" + FLOAT = "float" + DOUBLE = "double" + BIG_INTEGER = "bigInteger" + BIG_DECIMAL = "bigDecimal" + DOCUMENT = "document" + ENUM = "enum" + INT_ENUM = "intEnum" + LIST = "list" + MAP = "map" + STRUCTURE = "structure" + UNION = "union" + SERVICE = "service" + RESOURCE = "resource" + OPERATION = "operation" + + @property + def is_service_category(self) -> bool: + """Whether the type describes an API rather than data.""" + return self in {ShapeType.SERVICE, ShapeType.RESOURCE, ShapeType.OPERATION} + + +_IDENTIFIER = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") +_NAMESPACE = re.compile(rf"{_IDENTIFIER.pattern}(?:\.{_IDENTIFIER.pattern})*") + + +@dataclass(frozen=True, slots=True, order=True) +class ShapeID: + """An absolute Smithy shape ID, optionally identifying a member.""" + + namespace: str + name: str + member: str | None = None + + def __post_init__(self) -> None: + if not _NAMESPACE.fullmatch(self.namespace) or not _IDENTIFIER.fullmatch( + self.name + ): + raise ModelError(f"Invalid shape ID: {self}") + if self.member is not None and not _IDENTIFIER.fullmatch(self.member): + raise ModelError(f"Invalid shape ID: {self}") + + @classmethod + def parse(cls, value: str) -> Self: + """Parse an absolute Smithy shape ID.""" + if "#" not in value: + raise ModelError(f"Expected an absolute shape ID, found: {value!r}") + namespace, shape_name = value.split("#", 1) + name, separator, member = shape_name.partition("$") + return cls(namespace=namespace, name=name, member=member if separator else None) + + def with_member(self, member: str) -> Self: + return type(self)(namespace=self.namespace, name=self.name, member=member) + + def without_member(self) -> Self: + return type(self)(namespace=self.namespace, name=self.name) + + @property + def is_prelude(self) -> bool: + return self.namespace == PRELUDE_NAMESPACE + + def __str__(self) -> str: + value = f"{self.namespace}#{self.name}" + return f"{value}${self.member}" if self.member is not None else value + + +def _mapping( + value: Mapping[str, JSONValue] | None = None, +) -> Mapping[str, JSONValue]: + # A fresh dict preserves JSON insertion order while MappingProxyType prevents + # accidental mutation through a frozen dataclass. + return MappingProxyType(dict(value or {})) + + +@dataclass(frozen=True, slots=True) +class Member: + """A member of an aggregate shape, in modeled order.""" + + name: str + target: ShapeID + traits: Mapping[str, JSONValue] = field(default_factory=_mapping) + + def has_trait(self, trait: str) -> bool: + return trait in self.traits + + def trait(self, trait: str, default: JSONValue = None) -> JSONValue: + return self.traits.get(trait, default) + + +@dataclass(frozen=True, slots=True) +class Shape: + """A Smithy shape with ordered members and lossless shape-specific fields.""" + + id: ShapeID + type: ShapeType + traits: Mapping[str, JSONValue] = field(default_factory=_mapping) + mixins: tuple[ShapeID, ...] = () + members: tuple[Member, ...] = () + attributes: Mapping[str, JSONValue] = field(default_factory=_mapping) + + def has_trait(self, trait: str) -> bool: + return trait in self.traits + + def trait(self, trait: str, default: JSONValue = None) -> JSONValue: + return self.traits.get(trait, default) + + def member(self, name: str) -> Member: + for member in self.members: + if member.name == name: + return member + raise ModelError(f"Member not found: {self.id}${name}") + + def references(self) -> tuple[ShapeID, ...]: + """Return all structural references in stable modeled order.""" + result = [*self.mixins, *(member.target for member in self.members)] + for key in ( + "operations", + "resources", + "errors", + "collectionOperations", + ): + result.extend(_reference_list(self.attributes.get(key), f"{self.id}.{key}")) + for key in ( + "input", + "output", + "create", + "put", + "read", + "update", + "delete", + "list", + ): + value = self.attributes.get(key) + if value is not None: + result.append(_reference(value, f"{self.id}.{key}")) + for key in ("identifiers", "properties"): + values = self.attributes.get(key) + if isinstance(values, dict): + result.extend( + _reference(value, f"{self.id}.{key}.{name}") + for name, value in values.items() + ) + return tuple(dict.fromkeys(result)) + + +# Prelude shapes are omitted from the JSON AST unless the build opts in, so they +# are resolved on demand when a member targets one. +_PRELUDE_TYPES: dict[str, ShapeType] = { + "Blob": ShapeType.BLOB, + "Boolean": ShapeType.BOOLEAN, + "String": ShapeType.STRING, + "Timestamp": ShapeType.TIMESTAMP, + "Byte": ShapeType.BYTE, + "Short": ShapeType.SHORT, + "Integer": ShapeType.INTEGER, + "Long": ShapeType.LONG, + "Float": ShapeType.FLOAT, + "Double": ShapeType.DOUBLE, + "BigInteger": ShapeType.BIG_INTEGER, + "BigDecimal": ShapeType.BIG_DECIMAL, + "Document": ShapeType.DOCUMENT, + "PrimitiveBoolean": ShapeType.BOOLEAN, + "PrimitiveByte": ShapeType.BYTE, + "PrimitiveShort": ShapeType.SHORT, + "PrimitiveInteger": ShapeType.INTEGER, + "PrimitiveLong": ShapeType.LONG, + "PrimitiveFloat": ShapeType.FLOAT, + "PrimitiveDouble": ShapeType.DOUBLE, + "Unit": ShapeType.STRUCTURE, +} + + +@dataclass(frozen=True, slots=True) +class Model: + """An ordered Smithy model parsed from a JSON AST document.""" + + smithy: str + metadata: Mapping[str, JSONValue] + shapes: tuple[Shape, ...] + _index: Mapping[ShapeID, Shape] = field(init=False, repr=False, compare=False) + + def __post_init__(self) -> None: + index: dict[ShapeID, Shape] = {} + for shape in self.shapes: + if shape.id in index: + raise ModelError(f"Duplicate shape: {shape.id}") + index[shape.id] = shape + object.__setattr__(self, "_index", MappingProxyType(index)) + + @classmethod + def from_json(cls, source: str | bytes | bytearray) -> Self: + """Parse a Smithy JSON AST document.""" + try: + document = cast(object, json.loads(source)) + except (TypeError, UnicodeDecodeError, json.JSONDecodeError) as error: + raise ModelError(f"Invalid Smithy JSON AST: {error}") from error + return cls.from_dict(_object_mapping(document, "Smithy JSON AST")) + + @classmethod + def from_dict(cls, document: Mapping[str, object]) -> Self: + """Build a model from a decoded Smithy JSON AST document.""" + version = document.get("smithy") + if not isinstance(version, str): + raise ModelError("The Smithy JSON AST is missing a string 'smithy' version") + shapes_node = _object_mapping(document.get("shapes", {}), "Smithy model shapes") + metadata = _json_object(document.get("metadata", {}), "Smithy model metadata") + + shapes: list[Shape] = [] + applies: list[tuple[ShapeID, Mapping[str, JSONValue]]] = [] + for shape_id, unparsed_node in shapes_node.items(): + node = _object_mapping(unparsed_node, f"shape {shape_id}") + parsed_id = ShapeID.parse(shape_id) + if node.get("type") == "apply": + applies.append( + (parsed_id, _expect_traits(node.get("traits", {}), parsed_id)) + ) + continue + shapes.append(_parse_shape(parsed_id, node)) + + if applies: + shapes = _apply_traits(shapes, applies) + return cls(smithy=version, metadata=_mapping(metadata), shapes=tuple(shapes)) + + def __iter__(self) -> Iterator[Shape]: + return iter(self.shapes) + + def __len__(self) -> int: + return len(self.shapes) + + def get(self, shape_id: ShapeID | str) -> Shape | None: + """Return a shape by ID, resolving prelude shapes even when omitted.""" + shape_id = ShapeID.parse(shape_id) if isinstance(shape_id, str) else shape_id + if shape_id.member is not None: + return self._index.get(shape_id.without_member()) + if (shape := self._index.get(shape_id)) is not None: + return shape + if shape_id.is_prelude and shape_id.name in _PRELUDE_TYPES: + return Shape(id=shape_id, type=_PRELUDE_TYPES[shape_id.name]) + return None + + def expect(self, shape_id: ShapeID | str) -> Shape: + """Return a shape by ID or raise :class:`ModelError` if it is absent.""" + if (shape := self.get(shape_id)) is None: + raise ModelError(f"Shape not found: {shape_id}") + return shape + + def services(self) -> tuple[Shape, ...]: + """Return every service shape in modeled order.""" + return tuple(shape for shape in self if shape.type is ShapeType.SERVICE) + + def replace_shapes(self, shapes: Iterable[Shape]) -> Self: + """Return a copy of the model with a different set of shapes.""" + return type(self)( + smithy=self.smithy, metadata=self.metadata, shapes=tuple(shapes) + ) + + +def _parse_shape(shape_id: ShapeID, node: Mapping[str, object]) -> Shape: + type_value = node.get("type") + try: + shape_type = ShapeType(type_value) + except (TypeError, ValueError) as error: + raise ModelError( + f"Unsupported shape type {type_value!r} on {shape_id}" + ) from error + traits = _expect_traits(node.get("traits", {}), shape_id) + mixins = tuple( + _reference(value, f"{shape_id}.mixins") + for value in _expect_list(node.get("mixins", []), f"{shape_id}.mixins") + ) + + members: list[Member] = [] + consumed = {"type", "traits", "mixins"} + if shape_type is ShapeType.LIST: + members.append(_parse_member("member", node.get("member"), shape_id)) + consumed.add("member") + elif shape_type is ShapeType.MAP: + members.extend( + ( + _parse_member("key", node.get("key"), shape_id), + _parse_member("value", node.get("value"), shape_id), + ) + ) + consumed.update(("key", "value")) + elif shape_type in { + ShapeType.STRUCTURE, + ShapeType.UNION, + ShapeType.ENUM, + ShapeType.INT_ENUM, + }: + members_node = _object_mapping( + node.get("members", {}), f"members of {shape_id}" + ) + members.extend( + _parse_member(name, member_node, shape_id) + for name, member_node in members_node.items() + ) + consumed.add("members") + + attributes = { + key: _json_value(value, f"{shape_id}.{key}") + for key, value in node.items() + if key not in consumed + } + return Shape( + id=shape_id, + type=shape_type, + traits=traits, + mixins=mixins, + members=tuple(members), + attributes=_mapping(attributes), + ) + + +def _parse_member(name: str, unparsed_node: object, container: ShapeID) -> Member: + node = _object_mapping(unparsed_node, f"member {container}${name}") + return Member( + name=name, + target=_target(node.get("target"), f"{container}${name}"), + traits=_expect_traits(node.get("traits", {}), container.with_member(name)), + ) + + +def _expect_traits(value: object, target: ShapeID) -> Mapping[str, JSONValue]: + return _mapping(_json_object(value, f"traits on {target}")) + + +def _expect_list(value: object, location: str) -> list[object]: + if not isinstance(value, list): + raise ModelError(f"Expected a list at {location}") + return cast(list[object], value) + + +def _reference(value: object, location: str) -> ShapeID: + reference = _object_mapping(value, location) + target = reference.get("target") + if not isinstance(target, str): + raise ModelError(f"Expected a shape reference at {location}") + return ShapeID.parse(target) + + +def _target(value: object, location: str) -> ShapeID: + if not isinstance(value, str): + raise ModelError(f"Expected a shape target at {location}") + return ShapeID.parse(value) + + +def _reference_list(value: object, location: str) -> tuple[ShapeID, ...]: + if value is None: + return () + return tuple(_reference(item, location) for item in _expect_list(value, location)) + + +def _object_mapping(value: object, location: str) -> dict[str, object]: + if not isinstance(value, Mapping): + raise ModelError(f"Expected an object at {location}") + result: dict[str, object] = {} + for key, item in cast(Mapping[object, object], value).items(): + if not isinstance(key, str): + raise ModelError(f"Expected string object keys at {location}") + result[key] = item + return result + + +def _json_object(value: object, location: str) -> dict[str, JSONValue]: + return { + key: _json_value(item, f"{location}.{key}") + for key, item in _object_mapping(value, location).items() + } + + +def _json_value(value: object, location: str) -> JSONValue: + if value is None or isinstance(value, bool | int | float | str): + return value + if isinstance(value, list): + return [_json_value(item, location) for item in cast(list[object], value)] + if isinstance(value, Mapping): + return _json_object(cast(object, value), location) + raise ModelError(f"Unsupported JSON value at {location}: {type(value).__name__}") + + +def _apply_traits( + shapes: list[Shape], applies: list[tuple[ShapeID, Mapping[str, JSONValue]]] +) -> list[Shape]: + positions = {shape.id: index for index, shape in enumerate(shapes)} + for target, traits in applies: + container_id = target.without_member() + if container_id not in positions: + raise ModelError(f"Apply target not found: {target}") + position = positions[container_id] + shape = shapes[position] + if target.member is None: + shapes[position] = replace( + shape, traits=_mapping({**shape.traits, **traits}) + ) + continue + members = list(shape.members) + for index, member in enumerate(members): + if member.name == target.member: + members[index] = replace( + member, traits=_mapping({**member.traits, **traits}) + ) + shapes[position] = replace(shape, members=tuple(members)) + break + else: + raise ModelError(f"Apply target member not found: {target}") + return shapes diff --git a/packages/smithy-python/src/smithy_python/selection.py b/packages/smithy-python/src/smithy_python/selection.py new file mode 100644 index 000000000..5d80e5932 --- /dev/null +++ b/packages/smithy-python/src/smithy_python/selection.py @@ -0,0 +1,119 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 +"""Resolution of the service and shapes that an artifact generates.""" + +from __future__ import annotations + +from collections import deque +from typing import Final + +from .exceptions import CodegenError, InvalidInvocationError +from .model import Model, Shape, ShapeID, ShapeType + +TRAIT_DEFINITION: Final = "smithy.api#trait" +PRIVATE_TRAIT: Final = "smithy.api#private" +MIXIN_TRAIT: Final = "smithy.api#mixin" + +# Shapes carrying these traits describe the model rather than data and are +# never generated, even when the JSON AST includes them. +_EXCLUDED_TRAITS: Final = frozenset({TRAIT_DEFINITION, MIXIN_TRAIT}) + + +def resolve_service( + model: Model, requested: ShapeID | None, *, required: bool +) -> Shape | None: + """Return the service to generate, or ``None`` when one is not needed. + + An explicitly requested service must exist and be a service shape. When + none is requested, a model containing exactly one service uses it, a model + with several services is an error, and a model with none returns ``None`` + unless the artifact requires a service. + """ + if requested is not None: + shape = model.get(requested) + if shape is None: + raise InvalidInvocationError(f"Service not found in model: {requested}") + if shape.type is not ShapeType.SERVICE: + raise InvalidInvocationError( + f"Expected a service shape, found {shape.type}: {requested}" + ) + return shape + + services = model.services() + if len(services) == 1: + return services[0] + if len(services) > 1: + candidates = ", ".join(str(service.id) for service in services) + raise InvalidInvocationError( + f"The model contains multiple services; select one with --service: " + f"{candidates}" + ) + if required: + raise InvalidInvocationError("The model does not contain a service") + return None + + +def select_generated_shapes(model: Model) -> tuple[Shape, ...]: + """Return the data shapes to generate, in modeled order. + + Every data shape in the model is a candidate; the set is not narrowed to a + service closure. Prelude shapes, trait definitions, and mixins are never + generated. Shapes marked ``@private`` are generated only when reachable from + another generated shape, since they are not meant to be used directly but + may still be targeted by public members. + + Raises :class:`CodegenError` when two selected shapes have case-insensitively + equal names, since they cannot coexist in one Python module. + """ + candidates = tuple(shape for shape in model if _is_candidate(shape)) + # Operations and services are not generated here, but shapes they reference + # are, so they seed reachability alongside the public data shapes. + roots = tuple( + shape + for shape in model + if not shape.has_trait(PRIVATE_TRAIT) and not _is_excluded(shape) + ) + reachable = _reachable_ids(model, roots) + selected = tuple( + shape + for shape in candidates + if not shape.has_trait(PRIVATE_TRAIT) or shape.id in reachable + ) + _require_unique_names(selected) + return selected + + +def _is_candidate(shape: Shape) -> bool: + if shape.type.is_service_category or shape.id.is_prelude: + return False + return not _is_excluded(shape) + + +def _is_excluded(shape: Shape) -> bool: + return any(shape.has_trait(trait) for trait in _EXCLUDED_TRAITS) + + +def _reachable_ids(model: Model, roots: tuple[Shape, ...]) -> set[ShapeID]: + reachable: set[ShapeID] = set() + queue = deque(root.id for root in roots) + while queue: + shape_id = queue.popleft().without_member() + if shape_id in reachable: + continue + reachable.add(shape_id) + if (shape := model.get(shape_id)) is not None: + queue.extend(shape.references()) + return reachable + + +def _require_unique_names(shapes: tuple[Shape, ...]) -> None: + by_name: dict[str, list[ShapeID]] = {} + for shape in shapes: + by_name.setdefault(shape.id.name.casefold(), []).append(shape.id) + conflicts = [ids for ids in by_name.values() if len(ids) > 1] + if conflicts: + details = "; ".join(", ".join(map(str, ids)) for ids in conflicts) + raise CodegenError( + "Generated shape names must be case-insensitively unique. Rename the " + f"conflicting shapes with the renameShapes transform: {details}" + ) diff --git a/packages/smithy-python/tests/unit/conftest.py b/packages/smithy-python/tests/unit/conftest.py new file mode 100644 index 000000000..1cd8a3a71 --- /dev/null +++ b/packages/smithy-python/tests/unit/conftest.py @@ -0,0 +1,106 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +from __future__ import annotations + +import json +from typing import Any + +import pytest +from smithy_python.model import Model + + +@pytest.fixture +def model_document() -> dict[str, Any]: + """A small weather service model in JSON AST form.""" + return { + "smithy": "2.0", + "metadata": {"example": True}, + "shapes": { + "example.weather#CityId": { + "type": "string", + "traits": {"smithy.api#pattern": "^[A-Za-z ]+$"}, + }, + "example.weather#Coordinates": { + "type": "structure", + "members": { + "latitude": { + "target": "smithy.api#Float", + "traits": {"smithy.api#required": {}}, + }, + "longitude": { + "target": "smithy.api#Float", + "traits": {"smithy.api#required": {}}, + }, + }, + }, + "example.weather#Tags": { + "type": "list", + "member": {"target": "smithy.api#String"}, + }, + "example.weather#GetCityInput": { + "type": "structure", + "traits": {"smithy.api#input": {}}, + "members": { + "cityId": { + "target": "example.weather#CityId", + "traits": { + "smithy.api#required": {}, + "smithy.api#httpLabel": {}, + }, + } + }, + }, + "example.weather#GetCityOutput": { + "type": "structure", + "traits": {"smithy.api#output": {}}, + "members": { + "coordinates": { + "target": "example.weather#Coordinates", + "traits": {"smithy.api#required": {}}, + }, + "tags": {"target": "example.weather#Tags"}, + }, + }, + "example.weather#NoSuchCity": { + "type": "structure", + "traits": {"smithy.api#error": "client"}, + "members": { + "message": {"target": "smithy.api#String"}, + }, + }, + "example.weather#GetCity": { + "type": "operation", + "input": {"target": "example.weather#GetCityInput"}, + "output": {"target": "example.weather#GetCityOutput"}, + "errors": [{"target": "example.weather#NoSuchCity"}], + "traits": { + "smithy.api#http": { + "method": "GET", + "uri": "/city/{cityId}", + "code": 200, + } + }, + }, + "example.weather#Weather": { + "type": "service", + "version": "2026-01-01", + "operations": [{"target": "example.weather#GetCity"}], + "traits": { + "aws.protocols#restJson1": {}, + "smithy.api#documentation": "Provides weather forecasts.", + }, + }, + "example.unused#Unused": {"type": "string"}, + }, + } + + +@pytest.fixture +def model(model_document: dict[str, Any]) -> Model: + return Model.from_dict(model_document) + + +@pytest.fixture +def model_json(model_document: dict[str, Any]) -> bytes: + return json.dumps(model_document).encode() diff --git a/packages/smithy-python/tests/unit/test_cli.py b/packages/smithy-python/tests/unit/test_cli.py index 74a2418cb..ce2ea64ad 100644 --- a/packages/smithy-python/tests/unit/test_cli.py +++ b/packages/smithy-python/tests/unit/test_cli.py @@ -3,10 +3,12 @@ from __future__ import annotations +import json import subprocess import sys from io import BytesIO from pathlib import Path +from typing import Any import pytest from smithy_python import __version__ @@ -35,11 +37,12 @@ def test_information_commands( @pytest.mark.parametrize("artifact", ["client", "types"]) def test_generation_commands_are_explicitly_unavailable( artifact: str, + model_json: bytes, tmp_path: Path, capsys: pytest.CaptureFixture[str], ) -> None: model = tmp_path / "model.json" - model.write_text("{}") + model.write_bytes(model_json) assert ( main( @@ -90,13 +93,13 @@ def test_main_module_invokes_cli() -> None: def test_run_plugin_invocation_reads_standard_input( - tmp_path: Path, capsys: pytest.CaptureFixture[str] + model_json: bytes, tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: assert ( main( ("generate", "client"), environ={"SMITHY_PLUGIN_DIR": str(tmp_path)}, - stdin=BytesIO(b"{}"), + stdin=BytesIO(model_json), ) == 1 ) @@ -233,3 +236,134 @@ def raise_io_error(self: Path) -> bytes: == 1 ) assert "unable to read model" in capsys.readouterr().err + + +def test_help_documents_service_option(capsys: pytest.CaptureFixture[str]) -> None: + assert main(("generate", "client", "--help")) == 0 + assert "--service SHAPE_ID" in capsys.readouterr().out + + +def test_invalid_model_is_a_generation_failure( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + assert ( + main( + ("generate", "types", "--output", str(tmp_path)), + environ={}, + stdin=BytesIO(b"{}"), + ) + == 1 + ) + assert "missing a string 'smithy' version" in capsys.readouterr().err + + +def test_client_requires_a_service( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + assert ( + main( + ("generate", "client", "--output", str(tmp_path)), + environ={}, + stdin=BytesIO(b'{"smithy": "2.0"}'), + ) + == 2 + ) + assert "does not contain a service" in capsys.readouterr().err + + +def test_types_does_not_require_a_service( + tmp_path: Path, capsys: pytest.CaptureFixture[str] +) -> None: + assert ( + main( + ("generate", "types", "--output", str(tmp_path)), + environ={}, + stdin=BytesIO(b'{"smithy": "2.0"}'), + ) + == 1 + ) + assert "types generation is not implemented yet" in capsys.readouterr().err + + +def test_multiple_services_require_service_option( + model_document: dict[str, Any], + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + model_document["shapes"]["example.other#Other"] = { + "type": "service", + "version": "1", + } + source = json.dumps(model_document).encode() + + assert ( + main( + ("generate", "client", "--output", str(tmp_path)), + environ={}, + stdin=BytesIO(source), + ) + == 2 + ) + assert "select one with --service" in capsys.readouterr().err + + assert ( + main( + ( + "generate", + "client", + "--output", + str(tmp_path), + "--service", + "example.weather#Weather", + ), + environ={}, + stdin=BytesIO(source), + ) + == 1 + ) + assert "client generation is not implemented yet" in capsys.readouterr().err + + +@pytest.mark.parametrize( + ("value", "message"), + [ + ("Weather", "Invalid --service value"), + ("example.weather#Weather$member", "not a member"), + ("example.weather#Nope", "Service not found"), + ("example.weather#Coordinates", "Expected a service shape"), + ], +) +def test_invalid_service_option_is_an_invocation_error( + value: str, + message: str, + model_json: bytes, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + assert ( + main( + ("generate", "client", "--output", str(tmp_path), "--service", value), + environ={}, + stdin=BytesIO(model_json), + ) + == 2 + ) + assert message in capsys.readouterr().err + + +def test_shape_name_conflicts_are_a_generation_failure( + model_document: dict[str, Any], + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + model_document["shapes"]["example.other#coordinates"] = {"type": "string"} + + assert ( + main( + ("generate", "types", "--output", str(tmp_path)), + environ={}, + stdin=BytesIO(json.dumps(model_document).encode()), + ) + == 1 + ) + assert "case-insensitively unique" in capsys.readouterr().err diff --git a/packages/smithy-python/tests/unit/test_model.py b/packages/smithy-python/tests/unit/test_model.py new file mode 100644 index 000000000..d72832197 --- /dev/null +++ b/packages/smithy-python/tests/unit/test_model.py @@ -0,0 +1,244 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +from __future__ import annotations + +from typing import Any + +import pytest +from smithy_python.exceptions import CodegenError, ModelError +from smithy_python.model import Model, ShapeID, ShapeType + + +def test_model_error_is_a_codegen_error() -> None: + assert issubclass(ModelError, CodegenError) + + +class TestShapeID: + def test_parse_shape_and_member_ids(self) -> None: + assert ShapeID.parse("example#Foo") == ShapeID("example", "Foo") + assert ShapeID.parse("example#Foo$bar") == ShapeID("example", "Foo", "bar") + + @pytest.mark.parametrize( + "value", + ["Foo", "#Foo", "example#", "a#b#c", "example#Foo$", "example#F$o$o", "a-b#C"], + ) + def test_parse_rejects_invalid_ids(self, value: str) -> None: + with pytest.raises(ModelError, match="shape ID"): + ShapeID.parse(value) + + def test_round_trips_through_str(self) -> None: + for value in ("example#Foo", "example.nested#Foo$bar"): + assert str(ShapeID.parse(value)) == value + + def test_member_helpers(self) -> None: + shape = ShapeID.parse("example#Foo") + member = shape.with_member("bar") + assert member.member == "bar" + assert member.without_member() == shape + assert ShapeID.parse("smithy.api#String").is_prelude + assert not shape.is_prelude + + +class TestParsing: + def test_preserves_shape_member_and_trait_order(self, model: Model) -> None: + assert [shape.id.name for shape in model][:3] == [ + "CityId", + "Coordinates", + "Tags", + ] + coordinates = model.expect("example.weather#Coordinates") + assert [member.name for member in coordinates.members] == [ + "latitude", + "longitude", + ] + assert coordinates.type is ShapeType.STRUCTURE + assert coordinates.member("latitude").has_trait("smithy.api#required") + + def test_parsed_objects_are_immutable(self, model: Model) -> None: + coordinates = model.expect("example.weather#Coordinates") + with pytest.raises(TypeError): + coordinates.traits["example#trait"] = {} # type: ignore[index] + + def test_from_json_accepts_bytes(self, model_json: bytes, model: Model) -> None: + assert Model.from_json(model_json) == model + + def test_len_and_metadata(self, model: Model) -> None: + assert len(model) == 9 + assert model.metadata == {"example": True} + + def test_operation_attributes_are_kept_losslessly(self, model: Model) -> None: + operation = model.expect("example.weather#GetCity") + assert operation.attributes["input"] == { + "target": "example.weather#GetCityInput" + } + assert operation.trait("smithy.api#http") == { + "method": "GET", + "uri": "/city/{cityId}", + "code": 200, + } + + def test_references_follow_structural_relationships(self, model: Model) -> None: + service = model.expect("example.weather#Weather") + assert service.references() == (ShapeID.parse("example.weather#GetCity"),) + operation = model.expect("example.weather#GetCity") + assert set(operation.references()) == { + ShapeID.parse("example.weather#GetCityInput"), + ShapeID.parse("example.weather#GetCityOutput"), + ShapeID.parse("example.weather#NoSuchCity"), + } + + def test_resources_and_maps_are_parsed( + self, model_document: dict[str, Any] + ) -> None: + shapes = model_document["shapes"] + shapes["example.weather#Forecasts"] = { + "type": "map", + "key": {"target": "example.weather#CityId"}, + "value": {"target": "smithy.api#String"}, + } + shapes["example.weather#City"] = { + "type": "resource", + "identifiers": {"cityId": {"target": "example.weather#CityId"}}, + "properties": {"coordinates": {"target": "example.weather#Coordinates"}}, + "read": {"target": "example.weather#GetCity"}, + "collectionOperations": [], + } + model = Model.from_dict(model_document) + + forecasts = model.expect("example.weather#Forecasts") + assert [member.name for member in forecasts.members] == ["key", "value"] + assert forecasts.member("key").target == ShapeID.parse("example.weather#CityId") + + city = model.expect("example.weather#City") + assert city.references() == ( + ShapeID.parse("example.weather#GetCity"), + ShapeID.parse("example.weather#CityId"), + ShapeID.parse("example.weather#Coordinates"), + ) + + def test_references_report_malformed_targets( + self, model_document: dict[str, Any] + ) -> None: + model_document["shapes"]["example.weather#GetCity"]["errors"] = [{"target": 1}] + with pytest.raises(ModelError, match="Expected a shape reference"): + Model.from_dict(model_document).expect( + "example.weather#GetCity" + ).references() + + def test_apply_merges_traits_onto_members( + self, model_document: dict[str, Any] + ) -> None: + model_document["shapes"]["example.weather#Coordinates$latitude"] = { + "type": "apply", + "traits": {"smithy.api#documentation": "Latitude"}, + } + + model = Model.from_dict(model_document) + latitude = model.expect("example.weather#Coordinates").member("latitude") + assert latitude.trait("smithy.api#documentation") == "Latitude" + assert latitude.has_trait("smithy.api#required") + + def test_apply_to_missing_member_is_an_error( + self, model_document: dict[str, Any] + ) -> None: + model_document["shapes"]["example.weather#Coordinates$altitude"] = { + "type": "apply", + "traits": {}, + } + with pytest.raises(ModelError, match="Apply target member not found"): + Model.from_dict(model_document) + + def test_apply_to_missing_target_is_an_error( + self, model_document: dict[str, Any] + ) -> None: + model_document["shapes"]["example.weather#Missing"] = { + "type": "apply", + "traits": {}, + } + with pytest.raises(ModelError, match="Apply target not found"): + Model.from_dict(model_document) + + def test_shapes_key_is_optional(self) -> None: + assert len(Model.from_dict({"smithy": "2.0"})) == 0 + + @pytest.mark.parametrize( + ("document", "message"), + [ + ({}, "missing a string 'smithy' version"), + ({"smithy": 2}, "missing a string 'smithy' version"), + ({"smithy": "2.0", "shapes": []}, "Expected an object"), + ( + {"smithy": "2.0", "shapes": {"example#Bad": {"type": "nope"}}}, + "Unsupported shape type", + ), + ( + {"smithy": "2.0", "shapes": {"example#Bad": {"type": "list"}}}, + "Expected an object at member", + ), + ( + { + "smithy": "2.0", + "shapes": {"example#Bad": {"type": "list", "member": {}}}, + }, + "shape target", + ), + ( + { + "smithy": "2.0", + "shapes": { + "example#Bad": {"type": "structure", "mixins": "example#M"} + }, + }, + "Expected a list", + ), + ], + ) + def test_invalid_documents_are_reported( + self, document: dict[str, Any], message: str + ) -> None: + with pytest.raises(ModelError, match=message): + Model.from_dict(document) + + @pytest.mark.parametrize("source", [b"", b"not json", b"[]", b"\xff"]) + def test_invalid_json_is_reported(self, source: bytes) -> None: + with pytest.raises(ModelError, match="Smithy JSON AST"): + Model.from_json(source) + + +class TestLookup: + def test_resolves_prelude_without_inserting_it(self, model: Model) -> None: + assert model.expect("smithy.api#String").type is ShapeType.STRING + assert model.expect("smithy.api#Unit").type is ShapeType.STRUCTURE + assert all(not shape.id.is_prelude for shape in model) + + def test_modeled_prelude_takes_precedence( + self, model_document: dict[str, Any] + ) -> None: + model_document["shapes"]["smithy.api#String"] = { + "type": "string", + "traits": {"smithy.api#documentation": "from the prelude"}, + } + model = Model.from_dict(model_document) + assert model.expect("smithy.api#String").has_trait("smithy.api#documentation") + + def test_get_returns_none_for_unknown_shapes(self, model: Model) -> None: + assert model.get("example.weather#Nope") is None + assert model.get("smithy.api#Nope") is None + + def test_member_id_resolves_to_container(self, model: Model) -> None: + shape = model.expect("example.weather#Coordinates$latitude") + assert shape.id == ShapeID.parse("example.weather#Coordinates") + + def test_expect_reports_missing_shapes(self, model: Model) -> None: + with pytest.raises(ModelError, match="Shape not found"): + model.expect("example.weather#Nope") + with pytest.raises(ModelError, match="Member not found"): + model.expect("example.weather#Coordinates").member("altitude") + + def test_services_are_listed_in_model_order(self, model: Model) -> None: + assert [shape.id.name for shape in model.services()] == ["Weather"] + + def test_duplicate_shapes_are_rejected(self, model: Model) -> None: + with pytest.raises(ModelError, match="Duplicate shape"): + model.replace_shapes((*model.shapes, model.shapes[0])) diff --git a/packages/smithy-python/tests/unit/test_selection.py b/packages/smithy-python/tests/unit/test_selection.py new file mode 100644 index 000000000..7b7ac821c --- /dev/null +++ b/packages/smithy-python/tests/unit/test_selection.py @@ -0,0 +1,143 @@ +# Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. +# SPDX-License-Identifier: Apache-2.0 + +from __future__ import annotations + +from typing import Any + +import pytest +from smithy_python.exceptions import CodegenError, InvalidInvocationError +from smithy_python.model import Model, ShapeID +from smithy_python.selection import resolve_service, select_generated_shapes + +WEATHER = ShapeID.parse("example.weather#Weather") + + +def _names(model: Model) -> list[str]: + return [shape.id.name for shape in select_generated_shapes(model)] + + +class TestResolveService: + def test_single_service_is_detected(self, model: Model) -> None: + for required in (True, False): + service = resolve_service(model, None, required=required) + assert service is not None and service.id == WEATHER + + def test_explicit_service_is_used(self, model: Model) -> None: + service = resolve_service(model, WEATHER, required=True) + assert service is not None and service.id == WEATHER + + def test_explicit_service_must_exist(self, model: Model) -> None: + with pytest.raises(InvalidInvocationError, match="Service not found"): + resolve_service(model, ShapeID.parse("example#Nope"), required=True) + + def test_explicit_service_must_be_a_service(self, model: Model) -> None: + with pytest.raises(InvalidInvocationError, match="found structure"): + resolve_service( + model, ShapeID.parse("example.weather#Coordinates"), required=True + ) + + def test_multiple_services_require_a_selection( + self, model_document: dict[str, Any] + ) -> None: + model_document["shapes"]["example.other#Other"] = { + "type": "service", + "version": "1", + } + model = Model.from_dict(model_document) + + with pytest.raises(InvalidInvocationError) as info: + resolve_service(model, None, required=False) + assert "example.weather#Weather, example.other#Other" in str(info.value) + + service = resolve_service(model, WEATHER, required=True) + assert service is not None and service.id == WEATHER + + def test_no_service_is_allowed_only_when_optional(self) -> None: + model = Model.from_dict({"smithy": "2.0"}) + assert resolve_service(model, None, required=False) is None + with pytest.raises(InvalidInvocationError, match="does not contain a service"): + resolve_service(model, None, required=True) + + +class TestSelectGeneratedShapes: + def test_selects_every_data_shape_in_model_order(self, model: Model) -> None: + assert _names(model) == [ + "CityId", + "Coordinates", + "Tags", + "GetCityInput", + "GetCityOutput", + "NoSuchCity", + "Unused", + ] + + def test_skips_traits_mixins_and_prelude( + self, model_document: dict[str, Any] + ) -> None: + shapes = model_document["shapes"] + shapes["example.weather#myTrait"] = { + "type": "structure", + "traits": {"smithy.api#trait": {}}, + } + shapes["example.weather#Auditable"] = { + "type": "structure", + "traits": {"smithy.api#mixin": {}}, + "members": {"createdAt": {"target": "smithy.api#Timestamp"}}, + } + shapes["smithy.api#String"] = {"type": "string"} + names = _names(Model.from_dict(model_document)) + assert "myTrait" not in names + assert "Auditable" not in names + assert "String" not in names + + def test_private_shapes_are_generated_only_when_referenced( + self, model_document: dict[str, Any] + ) -> None: + shapes = model_document["shapes"] + shapes["example.weather#Hidden"] = { + "type": "structure", + "traits": {"smithy.api#private": {}}, + } + shapes["example.weather#Nested"] = { + "type": "structure", + "traits": {"smithy.api#private": {}}, + } + shapes["example.weather#Used"] = { + "type": "structure", + "traits": {"smithy.api#private": {}}, + "members": {"nested": {"target": "example.weather#Nested"}}, + } + shapes["example.weather#Coordinates"]["members"]["used"] = { + "target": "example.weather#Used" + } + names = _names(Model.from_dict(model_document)) + assert "Used" in names + assert "Nested" in names + assert "Hidden" not in names + + def test_private_shapes_referenced_by_operations_are_generated( + self, model_document: dict[str, Any] + ) -> None: + shapes = model_document["shapes"] + shapes["example.weather#NoSuchCity"]["traits"]["smithy.api#private"] = {} + assert "NoSuchCity" in _names(Model.from_dict(model_document)) + + def test_case_insensitive_name_conflicts_are_an_error( + self, model_document: dict[str, Any] + ) -> None: + model_document["shapes"]["example.other#coordinates"] = {"type": "string"} + with pytest.raises(CodegenError) as info: + select_generated_shapes(Model.from_dict(model_document)) + message = str(info.value) + assert "renameShapes" in message + assert "example.weather#Coordinates, example.other#coordinates" in message + + def test_conflicts_with_skipped_shapes_are_ignored( + self, model_document: dict[str, Any] + ) -> None: + model_document["shapes"]["example.other#Coordinates"] = { + "type": "structure", + "traits": {"smithy.api#private": {}}, + } + assert "Coordinates" in _names(Model.from_dict(model_document)) From 95f7590f8b1a3747963cabba9027d4ec507d6cd5 Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sat, 12 Sep 2026 19:44:20 -0400 Subject: [PATCH 10/19] fix(codegen): Resolve mixins when loading models Smithy serializes only what a shape introduces, so shapes that use mixins arrive without their inherited members, traits, errors, and service operations, and traits added to inherited members arrive as apply statements. Resolve mixins while loading, following the specification's precedence and ordering rules, and apply traits in two passes so that customizations of inherited members are honored. Mixin services are abstract, so they are no longer service candidates and cannot be selected with --service. --- .../smithy-python/src/smithy_python/model.py | 130 +++++++- .../src/smithy_python/selection.py | 21 +- .../smithy-python/tests/unit/test_model.py | 298 ++++++++++++++++++ .../tests/unit/test_selection.py | 19 ++ 4 files changed, 454 insertions(+), 14 deletions(-) diff --git a/packages/smithy-python/src/smithy_python/model.py b/packages/smithy-python/src/smithy_python/model.py index 7e316f977..4c458cfc9 100644 --- a/packages/smithy-python/src/smithy_python/model.py +++ b/packages/smithy-python/src/smithy_python/model.py @@ -19,6 +19,7 @@ ) PRELUDE_NAMESPACE = "smithy.api" +MIXIN_TRAIT = "smithy.api#mixin" class ShapeType(StrEnum): @@ -239,7 +240,7 @@ def from_dict(cls, document: Mapping[str, object]) -> Self: metadata = _json_object(document.get("metadata", {}), "Smithy model metadata") shapes: list[Shape] = [] - applies: list[tuple[ShapeID, Mapping[str, JSONValue]]] = [] + applies: list[_Apply] = [] for shape_id, unparsed_node in shapes_node.items(): node = _object_mapping(unparsed_node, f"shape {shape_id}") parsed_id = ShapeID.parse(shape_id) @@ -250,8 +251,14 @@ def from_dict(cls, document: Mapping[str, object]) -> Self: continue shapes.append(_parse_shape(parsed_id, node)) - if applies: - shapes = _apply_traits(shapes, applies) + # Serialized models omit everything a shape inherits from its mixins, and + # traits added to inherited members arrive as apply statements. Traits + # applied to members that exist before resolution (including members of + # the mixins themselves) are merged first so that they are inherited; + # the rest target inherited members and are merged after resolution. + shapes, deferred = _apply_traits(shapes, applies, defer_missing_members=True) + shapes = _resolve_mixins(shapes) + shapes, _ = _apply_traits(shapes, deferred, defer_missing_members=False) return cls(smithy=version, metadata=_mapping(metadata), shapes=tuple(shapes)) def __iter__(self) -> Iterator[Shape]: @@ -412,10 +419,15 @@ def _json_value(value: object, location: str) -> JSONValue: raise ModelError(f"Unsupported JSON value at {location}: {type(value).__name__}") +type _Apply = tuple[ShapeID, Mapping[str, JSONValue]] + + def _apply_traits( - shapes: list[Shape], applies: list[tuple[ShapeID, Mapping[str, JSONValue]]] -) -> list[Shape]: + shapes: list[Shape], applies: list[_Apply], *, defer_missing_members: bool +) -> tuple[list[Shape], list[_Apply]]: + """Merge apply statements into shapes, returning any that were deferred.""" positions = {shape.id: index for index, shape in enumerate(shapes)} + deferred: list[_Apply] = [] for target, traits in applies: container_id = target.without_member() if container_id not in positions: @@ -436,5 +448,109 @@ def _apply_traits( shapes[position] = replace(shape, members=tuple(members)) break else: - raise ModelError(f"Apply target member not found: {target}") - return shapes + if not defer_missing_members: + raise ModelError(f"Apply target member not found: {target}") + deferred.append((target, traits)) + return shapes, deferred + + +def _resolve_mixins(shapes: list[Shape]) -> list[Shape]: + """Copy inherited traits, members, and properties onto shapes using mixins. + + Follows the resolution rules of the Smithy mixins specification: inherited + members precede local members in a depth-first traversal of the mixins, + later mixins take precedence over earlier ones, local definitions take + precedence over anything inherited, and the ``mixin`` trait itself and any + ``localTraits`` are not inherited. + """ + by_id = {shape.id: shape for shape in shapes} + resolved: dict[ShapeID, Shape] = {} + resolving: set[ShapeID] = set() + + def resolve(shape: Shape) -> Shape: + if (done := resolved.get(shape.id)) is not None: + return done + if not shape.mixins: + resolved[shape.id] = shape + return shape + if shape.id in resolving: + raise ModelError(f"Mixin cycle detected at {shape.id}") + resolving.add(shape.id) + + traits: dict[str, JSONValue] = {} + members: dict[str, Member] = {} + attributes: dict[str, JSONValue] = {} + for mixin_id in shape.mixins: + mixin = by_id.get(mixin_id) + if mixin is None: + raise ModelError(f"Mixin not found: {mixin_id} (used by {shape.id})") + if not mixin.has_trait(MIXIN_TRAIT): + raise ModelError( + f"{shape.id} uses {mixin_id} as a mixin, but it lacks the " + f"{MIXIN_TRAIT} trait" + ) + mixin = resolve(mixin) + traits.update(_inherited_traits(mixin)) + for member in mixin.members: + members[member.name] = _merge_members(members.get(member.name), member) + _merge_attributes(attributes, mixin.attributes) + + traits.update(shape.traits) + for member in shape.members: + members[member.name] = _merge_members(members.get(member.name), member) + _merge_attributes(attributes, shape.attributes) + + result = replace( + shape, + traits=_mapping(traits), + members=tuple(members.values()), + attributes=_mapping(attributes), + ) + resolving.discard(shape.id) + resolved[shape.id] = result + return result + + return [resolve(shape) for shape in shapes] + + +def _inherited_traits(mixin: Shape) -> dict[str, JSONValue]: + excluded = {MIXIN_TRAIT} + mixin_trait = mixin.trait(MIXIN_TRAIT) + if isinstance(mixin_trait, dict): + local_traits = mixin_trait.get("localTraits", []) + if isinstance(local_traits, list): + excluded.update(name for name in local_traits if isinstance(name, str)) + return {name: value for name, value in mixin.traits.items() if name not in excluded} + + +def _merge_members(inherited: Member | None, member: Member) -> Member: + """Merge a redefined member onto the one it inherits, keeping its position.""" + if inherited is None: + return member + if inherited.target != member.target: + raise ModelError( + f"Member {member.name} redefines an inherited member with a different " + f"target: {inherited.target} != {member.target}" + ) + return replace(member, traits=_mapping({**inherited.traits, **member.traits})) + + +def _merge_attributes( + target: dict[str, JSONValue], source: Mapping[str, JSONValue] +) -> None: + """Merge shape properties, giving ``source`` precedence. + + Lists are concatenated without duplicates, objects are merged key by key, + and scalars from ``source`` replace existing values. + """ + for key, value in source.items(): + existing = target.get(key) + if isinstance(existing, list) and isinstance(value, list): + target[key] = [ + *existing, + *(item for item in value if item not in existing), + ] + elif isinstance(existing, dict) and isinstance(value, dict): + target[key] = {**existing, **value} + else: + target[key] = value diff --git a/packages/smithy-python/src/smithy_python/selection.py b/packages/smithy-python/src/smithy_python/selection.py index 5d80e5932..e894d02af 100644 --- a/packages/smithy-python/src/smithy_python/selection.py +++ b/packages/smithy-python/src/smithy_python/selection.py @@ -8,11 +8,10 @@ from typing import Final from .exceptions import CodegenError, InvalidInvocationError -from .model import Model, Shape, ShapeID, ShapeType +from .model import MIXIN_TRAIT, Model, Shape, ShapeID, ShapeType TRAIT_DEFINITION: Final = "smithy.api#trait" PRIVATE_TRAIT: Final = "smithy.api#private" -MIXIN_TRAIT: Final = "smithy.api#mixin" # Shapes carrying these traits describe the model rather than data and are # never generated, even when the JSON AST includes them. @@ -24,10 +23,11 @@ def resolve_service( ) -> Shape | None: """Return the service to generate, or ``None`` when one is not needed. - An explicitly requested service must exist and be a service shape. When - none is requested, a model containing exactly one service uses it, a model - with several services is an error, and a model with none returns ``None`` - unless the artifact requires a service. + An explicitly requested service must exist and be a concrete service shape. + When none is requested, a model containing exactly one concrete service uses + it, a model with several is an error, and a model with none returns ``None`` + unless the artifact requires a service. Services marked ``@mixin`` are + abstract and never candidates. """ if requested is not None: shape = model.get(requested) @@ -37,9 +37,16 @@ def resolve_service( raise InvalidInvocationError( f"Expected a service shape, found {shape.type}: {requested}" ) + if shape.has_trait(MIXIN_TRAIT): + raise InvalidInvocationError( + f"Cannot generate a mixin service; select a service that uses it: " + f"{requested}" + ) return shape - services = model.services() + services = tuple( + service for service in model.services() if not service.has_trait(MIXIN_TRAIT) + ) if len(services) == 1: return services[0] if len(services) > 1: diff --git a/packages/smithy-python/tests/unit/test_model.py b/packages/smithy-python/tests/unit/test_model.py index d72832197..292160209 100644 --- a/packages/smithy-python/tests/unit/test_model.py +++ b/packages/smithy-python/tests/unit/test_model.py @@ -242,3 +242,301 @@ def test_services_are_listed_in_model_order(self, model: Model) -> None: def test_duplicate_shapes_are_rejected(self, model: Model) -> None: with pytest.raises(ModelError, match="Duplicate shape"): model.replace_shapes((*model.shapes, model.shapes[0])) + + +class TestMixins: + @staticmethod + def _document(shapes: dict[str, Any]) -> dict[str, Any]: + return {"smithy": "2.0", "shapes": shapes} + + def test_inherited_members_precede_local_members_depth_first(self) -> None: + model = Model.from_dict( + self._document( + { + "example#FilteredByName": { + "type": "structure", + "traits": {"smithy.api#mixin": {}}, + "members": {"nameFilter": {"target": "smithy.api#String"}}, + }, + "example#Paginated": { + "type": "structure", + "traits": {"smithy.api#mixin": {}}, + "members": { + "nextToken": {"target": "smithy.api#String"}, + "pageSize": {"target": "smithy.api#Integer"}, + }, + }, + "example#ListInput": { + "type": "structure", + "mixins": [ + {"target": "example#Paginated"}, + {"target": "example#FilteredByName"}, + ], + "members": {"sizeFilter": {"target": "smithy.api#Integer"}}, + }, + } + ) + ) + shape = model.expect("example#ListInput") + assert [member.name for member in shape.members] == [ + "nextToken", + "pageSize", + "nameFilter", + "sizeFilter", + ] + assert shape.mixins == ( + ShapeID.parse("example#Paginated"), + ShapeID.parse("example#FilteredByName"), + ) + # Mixins themselves are left untouched. + assert len(model.expect("example#Paginated").members) == 2 + + def test_traits_are_inherited_with_local_and_later_precedence(self) -> None: + model = Model.from_dict( + self._document( + { + "example#A": { + "type": "structure", + "traits": { + "smithy.api#mixin": {"localTraits": ["smithy.api#private"]}, + "smithy.api#private": {}, + "smithy.api#documentation": "A", + "example#foo": 1, + "example#onlyA": True, + }, + }, + "example#B": { + "type": "structure", + "traits": {"smithy.api#mixin": {}, "example#foo": 2}, + }, + "example#C": { + "type": "structure", + "mixins": [{"target": "example#A"}, {"target": "example#B"}], + "traits": { + "smithy.api#mixin": {}, + "smithy.api#documentation": "C", + }, + }, + "example#D": { + "type": "structure", + "mixins": [{"target": "example#C"}], + }, + } + ) + ) + c = model.expect("example#C") + assert c.trait("smithy.api#documentation") == "C" + assert c.trait("example#foo") == 2 + assert c.has_trait("example#onlyA") + assert not c.has_trait("smithy.api#private") + # Inheritance is transitive, and the mixin trait itself is not inherited. + d = model.expect("example#D") + assert d.trait("smithy.api#documentation") == "C" + assert d.trait("example#foo") == 2 + assert d.has_trait("example#onlyA") + assert not d.has_trait("smithy.api#private") + assert not d.has_trait("smithy.api#mixin") + + def test_apply_targets_inherited_members(self) -> None: + model = Model.from_dict( + self._document( + { + "example#M": { + "type": "structure", + "traits": {"smithy.api#mixin": {}}, + "members": {"foo": {"target": "smithy.api#String"}}, + }, + "example#S": { + "type": "structure", + "mixins": [{"target": "example#M"}], + }, + "example#S$foo": { + "type": "apply", + "traits": {"smithy.api#required": {}}, + }, + "example#M$foo": { + "type": "apply", + "traits": {"smithy.api#documentation": "docs"}, + }, + } + ) + ) + foo = model.expect("example#S").member("foo") + assert foo.has_trait("smithy.api#required") + assert foo.trait("smithy.api#documentation") == "docs" + assert ( + not model.expect("example#M").member("foo").has_trait("smithy.api#required") + ) + + def test_redefined_members_merge_traits_and_keep_position(self) -> None: + model = Model.from_dict( + self._document( + { + "example#M": { + "type": "structure", + "traits": {"smithy.api#mixin": {}}, + "members": { + "a": { + "target": "smithy.api#String", + "traits": {"smithy.api#documentation": "docs"}, + }, + "b": {"target": "smithy.api#String"}, + }, + }, + "example#S": { + "type": "structure", + "mixins": [{"target": "example#M"}], + "members": { + "c": {"target": "smithy.api#String"}, + "a": { + "target": "smithy.api#String", + "traits": {"smithy.api#required": {}}, + }, + }, + }, + } + ) + ) + shape = model.expect("example#S") + assert [member.name for member in shape.members] == ["a", "b", "c"] + a = shape.member("a") + assert a.has_trait("smithy.api#required") + assert a.has_trait("smithy.api#documentation") + + def test_redefined_members_must_keep_their_target(self) -> None: + with pytest.raises(ModelError, match="different target"): + Model.from_dict( + self._document( + { + "example#M": { + "type": "structure", + "traits": {"smithy.api#mixin": {}}, + "members": {"a": {"target": "smithy.api#String"}}, + }, + "example#S": { + "type": "structure", + "mixins": [{"target": "example#M"}], + "members": {"a": {"target": "smithy.api#Integer"}}, + }, + } + ) + ) + + def test_service_properties_are_merged(self) -> None: + model = Model.from_dict( + self._document( + { + "example#A": { + "type": "service", + "version": "A", + "operations": [{"target": "example#OpA"}], + "traits": {"smithy.api#mixin": {}}, + }, + "example#B": { + "type": "service", + "version": "B", + "rename": {"example#X": "Y"}, + "operations": [{"target": "example#OpB"}], + "mixins": [{"target": "example#A"}], + "traits": {"smithy.api#mixin": {}}, + }, + "example#C": { + "type": "service", + "version": "C", + "rename": {"example#Z": "W"}, + "operations": [ + {"target": "example#OpC"}, + {"target": "example#OpA"}, + ], + "mixins": [{"target": "example#B"}], + }, + "example#OpA": {"type": "operation"}, + "example#OpB": {"type": "operation"}, + "example#OpC": {"type": "operation"}, + } + ) + ) + service = model.expect("example#C") + assert service.attributes["version"] == "C" + assert service.attributes["rename"] == {"example#X": "Y", "example#Z": "W"} + assert service.references() == ( + ShapeID.parse("example#B"), + ShapeID.parse("example#OpA"), + ShapeID.parse("example#OpB"), + ShapeID.parse("example#OpC"), + ) + + def test_operation_errors_are_inherited(self) -> None: + model = Model.from_dict( + self._document( + { + "example#Validated": { + "type": "operation", + "errors": [{"target": "example#ValidationError"}], + "traits": {"smithy.api#mixin": {}}, + }, + "example#GetUser": { + "type": "operation", + "errors": [{"target": "example#NotFound"}], + "mixins": [{"target": "example#Validated"}], + }, + "example#ValidationError": { + "type": "structure", + "traits": {"smithy.api#error": "client"}, + }, + "example#NotFound": { + "type": "structure", + "traits": {"smithy.api#error": "client"}, + }, + } + ) + ) + assert model.expect("example#GetUser").attributes["errors"] == [ + {"target": "example#ValidationError"}, + {"target": "example#NotFound"}, + ] + + @pytest.mark.parametrize( + ("shapes", "message"), + [ + ( + { + "example#S": { + "type": "structure", + "mixins": [{"target": "example#Missing"}], + } + }, + "Mixin not found", + ), + ( + { + "example#M": {"type": "structure"}, + "example#S": { + "type": "structure", + "mixins": [{"target": "example#M"}], + }, + }, + "lacks the smithy.api#mixin trait", + ), + ( + { + "example#A": { + "type": "structure", + "traits": {"smithy.api#mixin": {}}, + "mixins": [{"target": "example#B"}], + }, + "example#B": { + "type": "structure", + "traits": {"smithy.api#mixin": {}}, + "mixins": [{"target": "example#A"}], + }, + }, + "Mixin cycle", + ), + ], + ) + def test_invalid_mixins_are_reported( + self, shapes: dict[str, Any], message: str + ) -> None: + with pytest.raises(ModelError, match=message): + Model.from_dict(self._document(shapes)) diff --git a/packages/smithy-python/tests/unit/test_selection.py b/packages/smithy-python/tests/unit/test_selection.py index 7b7ac821c..51a058661 100644 --- a/packages/smithy-python/tests/unit/test_selection.py +++ b/packages/smithy-python/tests/unit/test_selection.py @@ -59,6 +59,25 @@ def test_no_service_is_allowed_only_when_optional(self) -> None: with pytest.raises(InvalidInvocationError, match="does not contain a service"): resolve_service(model, None, required=True) + def test_mixin_services_are_not_candidates( + self, model_document: dict[str, Any] + ) -> None: + model_document["shapes"]["example.weather#Base"] = { + "type": "service", + "version": "1", + "traits": {"smithy.api#mixin": {}}, + } + model_document["shapes"]["example.weather#Weather"]["mixins"] = [ + {"target": "example.weather#Base"} + ] + model = Model.from_dict(model_document) + + service = resolve_service(model, None, required=True) + assert service is not None and service.id == WEATHER + + with pytest.raises(InvalidInvocationError, match="mixin service"): + resolve_service(model, ShapeID.parse("example.weather#Base"), required=True) + class TestSelectGeneratedShapes: def test_selects_every_data_shape_in_model_order(self, model: Model) -> None: From 25868bfe39bc7b177fbe8e15441dec62b2d5f60e Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sat, 12 Sep 2026 19:56:46 -0400 Subject: [PATCH 11/19] fix(codegen): Suggest removeUnusedShapes for name conflicts --- packages/smithy-python/src/smithy_python/selection.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/smithy-python/src/smithy_python/selection.py b/packages/smithy-python/src/smithy_python/selection.py index e894d02af..3ba8a1e04 100644 --- a/packages/smithy-python/src/smithy_python/selection.py +++ b/packages/smithy-python/src/smithy_python/selection.py @@ -122,5 +122,6 @@ def _require_unique_names(shapes: tuple[Shape, ...]) -> None: details = "; ".join(", ".join(map(str, ids)) for ids in conflicts) raise CodegenError( "Generated shape names must be case-insensitively unique. Rename the " - f"conflicting shapes with the renameShapes transform: {details}" + "conflicting shapes with the renameShapes transform, or drop shapes not " + f"connected to a service with the removeUnusedShapes transform: {details}" ) From 69e6fb54e65922ba5705cf65f2c49efe1a2f362c Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sat, 12 Sep 2026 21:07:40 -0400 Subject: [PATCH 12/19] refactor(codegen): Generate the service closure by default Generating every shape in the model diverged from every other Smithy generator and failed on a published AWS model whose leaked, unconnected shapes collide with real ones. Select the service closure when a service is resolved, matching the existing SDK surface, and report how many unconnected shapes were left out. Without a service, the types artifact still generates every data shape and fails on case-insensitive name conflicts, which Smithy only guarantees within a closure. Synthesized prelude shapes now carry their real traits: defaults on the Primitive* shapes and unitType on Unit. --- .../smithy-python/src/smithy_python/cli.py | 15 +- .../smithy-python/src/smithy_python/model.py | 47 +++---- .../src/smithy_python/selection.py | 72 +++++----- packages/smithy-python/tests/unit/test_cli.py | 22 ++- .../smithy-python/tests/unit/test_model.py | 12 +- .../tests/unit/test_selection.py | 128 +++++++++++------- 6 files changed, 180 insertions(+), 116 deletions(-) diff --git a/packages/smithy-python/src/smithy_python/cli.py b/packages/smithy-python/src/smithy_python/cli.py index 6cf39045c..83f2cd396 100644 --- a/packages/smithy-python/src/smithy_python/cli.py +++ b/packages/smithy-python/src/smithy_python/cli.py @@ -16,7 +16,7 @@ from .environment import PluginEnvironment from .exceptions import CodegenError, InvalidInvocationError, ModelError from .model import Model, Shape, ShapeID -from .selection import resolve_service, select_generated_shapes +from .selection import Selection, resolve_service, select_generated_shapes _GENERATION_NOT_IMPLEMENTED: Final = ( "smithy-python: error: {artifact} generation is not implemented yet\n" @@ -41,7 +41,7 @@ class _Request: invocation: _Invocation model: Model service: Shape | None - shapes: tuple[Shape, ...] + selection: Selection def main( @@ -85,8 +85,15 @@ def _resolve_request(invocation: _Invocation) -> _Request: invocation.service, required=invocation.artifact == _CLIENT_ARTIFACT, ) - shapes = select_generated_shapes(model) - return _Request(invocation=invocation, model=model, service=service, shapes=shapes) + selection = select_generated_shapes(model, service) + if selection.excluded and service is not None: + sys.stderr.write( + f"smithy-python: note: {len(selection.excluded)} shape(s) not connected " + f"to {service.id} will not be generated\n" + ) + return _Request( + invocation=invocation, model=model, service=service, selection=selection + ) def _create_parser() -> argparse.ArgumentParser: diff --git a/packages/smithy-python/src/smithy_python/model.py b/packages/smithy-python/src/smithy_python/model.py index 4c458cfc9..45e97eceb 100644 --- a/packages/smithy-python/src/smithy_python/model.py +++ b/packages/smithy-python/src/smithy_python/model.py @@ -179,28 +179,28 @@ def references(self) -> tuple[ShapeID, ...]: # Prelude shapes are omitted from the JSON AST unless the build opts in, so they # are resolved on demand when a member targets one. -_PRELUDE_TYPES: dict[str, ShapeType] = { - "Blob": ShapeType.BLOB, - "Boolean": ShapeType.BOOLEAN, - "String": ShapeType.STRING, - "Timestamp": ShapeType.TIMESTAMP, - "Byte": ShapeType.BYTE, - "Short": ShapeType.SHORT, - "Integer": ShapeType.INTEGER, - "Long": ShapeType.LONG, - "Float": ShapeType.FLOAT, - "Double": ShapeType.DOUBLE, - "BigInteger": ShapeType.BIG_INTEGER, - "BigDecimal": ShapeType.BIG_DECIMAL, - "Document": ShapeType.DOCUMENT, - "PrimitiveBoolean": ShapeType.BOOLEAN, - "PrimitiveByte": ShapeType.BYTE, - "PrimitiveShort": ShapeType.SHORT, - "PrimitiveInteger": ShapeType.INTEGER, - "PrimitiveLong": ShapeType.LONG, - "PrimitiveFloat": ShapeType.FLOAT, - "PrimitiveDouble": ShapeType.DOUBLE, - "Unit": ShapeType.STRUCTURE, +_PRELUDE_TYPES: dict[str, tuple[ShapeType, dict[str, JSONValue]]] = { + "Blob": (ShapeType.BLOB, {}), + "Boolean": (ShapeType.BOOLEAN, {}), + "String": (ShapeType.STRING, {}), + "Timestamp": (ShapeType.TIMESTAMP, {}), + "Byte": (ShapeType.BYTE, {}), + "Short": (ShapeType.SHORT, {}), + "Integer": (ShapeType.INTEGER, {}), + "Long": (ShapeType.LONG, {}), + "Float": (ShapeType.FLOAT, {}), + "Double": (ShapeType.DOUBLE, {}), + "BigInteger": (ShapeType.BIG_INTEGER, {}), + "BigDecimal": (ShapeType.BIG_DECIMAL, {}), + "Document": (ShapeType.DOCUMENT, {}), + "PrimitiveBoolean": (ShapeType.BOOLEAN, {"smithy.api#default": False}), + "PrimitiveByte": (ShapeType.BYTE, {"smithy.api#default": 0}), + "PrimitiveShort": (ShapeType.SHORT, {"smithy.api#default": 0}), + "PrimitiveInteger": (ShapeType.INTEGER, {"smithy.api#default": 0}), + "PrimitiveLong": (ShapeType.LONG, {"smithy.api#default": 0}), + "PrimitiveFloat": (ShapeType.FLOAT, {"smithy.api#default": 0}), + "PrimitiveDouble": (ShapeType.DOUBLE, {"smithy.api#default": 0}), + "Unit": (ShapeType.STRUCTURE, {"smithy.api#unitType": {}}), } @@ -275,7 +275,8 @@ def get(self, shape_id: ShapeID | str) -> Shape | None: if (shape := self._index.get(shape_id)) is not None: return shape if shape_id.is_prelude and shape_id.name in _PRELUDE_TYPES: - return Shape(id=shape_id, type=_PRELUDE_TYPES[shape_id.name]) + shape_type, traits = _PRELUDE_TYPES[shape_id.name] + return Shape(id=shape_id, type=shape_type, traits=_mapping(traits)) return None def expect(self, shape_id: ShapeID | str) -> Shape: diff --git a/packages/smithy-python/src/smithy_python/selection.py b/packages/smithy-python/src/smithy_python/selection.py index 3ba8a1e04..397aa1a49 100644 --- a/packages/smithy-python/src/smithy_python/selection.py +++ b/packages/smithy-python/src/smithy_python/selection.py @@ -5,13 +5,13 @@ from __future__ import annotations from collections import deque +from dataclasses import dataclass from typing import Final from .exceptions import CodegenError, InvalidInvocationError from .model import MIXIN_TRAIT, Model, Shape, ShapeID, ShapeType TRAIT_DEFINITION: Final = "smithy.api#trait" -PRIVATE_TRAIT: Final = "smithy.api#private" # Shapes carrying these traits describe the model rather than data and are # never generated, even when the JSON AST includes them. @@ -60,57 +60,56 @@ def resolve_service( return None -def select_generated_shapes(model: Model) -> tuple[Shape, ...]: +@dataclass(frozen=True, slots=True) +class Selection: + """The data shapes to generate and the ones left out.""" + + shapes: tuple[Shape, ...] + excluded: tuple[Shape, ...] + + +def select_generated_shapes(model: Model, service: Shape | None) -> Selection: """Return the data shapes to generate, in modeled order. - Every data shape in the model is a candidate; the set is not narrowed to a - service closure. Prelude shapes, trait definitions, and mixins are never - generated. Shapes marked ``@private`` are generated only when reachable from - another generated shape, since they are not meant to be used directly but - may still be targeted by public members. + With a service, the selection is the service closure: every data shape + reachable from the service through its operations, resources, and members, + which matches the surface every other Smithy generator produces. Data shapes + in the model that are not connected to the service are reported as excluded. - Raises :class:`CodegenError` when two selected shapes have case-insensitively - equal names, since they cannot coexist in one Python module. + Without a service, every data shape in the model is selected. Names are then + checked for case-insensitive uniqueness, which Smithy guarantees only within + a service closure, since conflicting names cannot coexist in one module. + + Prelude shapes, trait definitions, and mixins are never generated. """ candidates = tuple(shape for shape in model if _is_candidate(shape)) - # Operations and services are not generated here, but shapes they reference - # are, so they seed reachability alongside the public data shapes. - roots = tuple( - shape - for shape in model - if not shape.has_trait(PRIVATE_TRAIT) and not _is_excluded(shape) - ) - reachable = _reachable_ids(model, roots) - selected = tuple( - shape - for shape in candidates - if not shape.has_trait(PRIVATE_TRAIT) or shape.id in reachable - ) - _require_unique_names(selected) - return selected + if service is None: + _require_unique_names(candidates) + return Selection(shapes=candidates, excluded=()) + + closure = _closure(model, service) + shapes = tuple(shape for shape in candidates if shape.id in closure) + excluded = tuple(shape for shape in candidates if shape.id not in closure) + return Selection(shapes=shapes, excluded=excluded) def _is_candidate(shape: Shape) -> bool: if shape.type.is_service_category or shape.id.is_prelude: return False - return not _is_excluded(shape) - - -def _is_excluded(shape: Shape) -> bool: - return any(shape.has_trait(trait) for trait in _EXCLUDED_TRAITS) + return not any(shape.has_trait(trait) for trait in _EXCLUDED_TRAITS) -def _reachable_ids(model: Model, roots: tuple[Shape, ...]) -> set[ShapeID]: - reachable: set[ShapeID] = set() - queue = deque(root.id for root in roots) +def _closure(model: Model, service: Shape) -> set[ShapeID]: + closure: set[ShapeID] = set() + queue = deque([service.id]) while queue: shape_id = queue.popleft().without_member() - if shape_id in reachable: + if shape_id in closure: continue - reachable.add(shape_id) + closure.add(shape_id) if (shape := model.get(shape_id)) is not None: queue.extend(shape.references()) - return reachable + return closure def _require_unique_names(shapes: tuple[Shape, ...]) -> None: @@ -122,6 +121,5 @@ def _require_unique_names(shapes: tuple[Shape, ...]) -> None: details = "; ".join(", ".join(map(str, ids)) for ids in conflicts) raise CodegenError( "Generated shape names must be case-insensitively unique. Rename the " - "conflicting shapes with the renameShapes transform, or drop shapes not " - f"connected to a service with the removeUnusedShapes transform: {details}" + f"conflicting shapes with the renameShapes transform: {details}" ) diff --git a/packages/smithy-python/tests/unit/test_cli.py b/packages/smithy-python/tests/unit/test_cli.py index ce2ea64ad..3522f1a74 100644 --- a/packages/smithy-python/tests/unit/test_cli.py +++ b/packages/smithy-python/tests/unit/test_cli.py @@ -58,7 +58,7 @@ def test_generation_commands_are_explicitly_unavailable( ) == 1 ) - assert capsys.readouterr().err == ( + assert capsys.readouterr().err.endswith( f"smithy-python: error: {artifact} generation is not implemented yet\n" ) @@ -351,11 +351,31 @@ def test_invalid_service_option_is_an_invocation_error( assert message in capsys.readouterr().err +def test_unconnected_shapes_are_reported( + model_json: bytes, + tmp_path: Path, + capsys: pytest.CaptureFixture[str], +) -> None: + assert ( + main( + ("generate", "client", "--output", str(tmp_path)), + environ={}, + stdin=BytesIO(model_json), + ) + == 1 + ) + assert ( + "note: 1 shape(s) not connected to example.weather#Weather will not be " + "generated" + ) in capsys.readouterr().err + + def test_shape_name_conflicts_are_a_generation_failure( model_document: dict[str, Any], tmp_path: Path, capsys: pytest.CaptureFixture[str], ) -> None: + del model_document["shapes"]["example.weather#Weather"] model_document["shapes"]["example.other#coordinates"] = {"type": "string"} assert ( diff --git a/packages/smithy-python/tests/unit/test_model.py b/packages/smithy-python/tests/unit/test_model.py index 292160209..4d731689c 100644 --- a/packages/smithy-python/tests/unit/test_model.py +++ b/packages/smithy-python/tests/unit/test_model.py @@ -209,7 +209,17 @@ def test_invalid_json_is_reported(self, source: bytes) -> None: class TestLookup: def test_resolves_prelude_without_inserting_it(self, model: Model) -> None: assert model.expect("smithy.api#String").type is ShapeType.STRING - assert model.expect("smithy.api#Unit").type is ShapeType.STRUCTURE + assert not model.expect("smithy.api#String").traits + unit = model.expect("smithy.api#Unit") + assert unit.type is ShapeType.STRUCTURE + assert unit.has_trait("smithy.api#unitType") + assert ( + model.expect("smithy.api#PrimitiveInteger").trait("smithy.api#default") == 0 + ) + assert ( + model.expect("smithy.api#PrimitiveBoolean").trait("smithy.api#default") + is False + ) assert all(not shape.id.is_prelude for shape in model) def test_modeled_prelude_takes_precedence( diff --git a/packages/smithy-python/tests/unit/test_selection.py b/packages/smithy-python/tests/unit/test_selection.py index 51a058661..e69521bf5 100644 --- a/packages/smithy-python/tests/unit/test_selection.py +++ b/packages/smithy-python/tests/unit/test_selection.py @@ -7,14 +7,20 @@ import pytest from smithy_python.exceptions import CodegenError, InvalidInvocationError -from smithy_python.model import Model, ShapeID +from smithy_python.model import Model, Shape, ShapeID from smithy_python.selection import resolve_service, select_generated_shapes WEATHER = ShapeID.parse("example.weather#Weather") -def _names(model: Model) -> list[str]: - return [shape.id.name for shape in select_generated_shapes(model)] +def _service(model: Model) -> Shape: + service = resolve_service(model, None, required=True) + assert service is not None + return service + + +def _names(shapes: tuple[Shape, ...]) -> list[str]: + return [shape.id.name for shape in shapes] class TestResolveService: @@ -79,84 +85,106 @@ def test_mixin_services_are_not_candidates( resolve_service(model, ShapeID.parse("example.weather#Base"), required=True) -class TestSelectGeneratedShapes: - def test_selects_every_data_shape_in_model_order(self, model: Model) -> None: - assert _names(model) == [ +class TestSelectWithService: + def test_selects_the_service_closure_in_model_order(self, model: Model) -> None: + selection = select_generated_shapes(model, _service(model)) + assert _names(selection.shapes) == [ "CityId", "Coordinates", "Tags", "GetCityInput", "GetCityOutput", "NoSuchCity", - "Unused", ] + assert _names(selection.excluded) == ["Unused"] - def test_skips_traits_mixins_and_prelude( + def test_closure_follows_resources_and_service_errors( self, model_document: dict[str, Any] ) -> None: shapes = model_document["shapes"] - shapes["example.weather#myTrait"] = { + shapes["example.weather#Throttled"] = { "type": "structure", - "traits": {"smithy.api#trait": {}}, + "traits": {"smithy.api#error": "client"}, } - shapes["example.weather#Auditable"] = { - "type": "structure", - "traits": {"smithy.api#mixin": {}}, - "members": {"createdAt": {"target": "smithy.api#Timestamp"}}, + shapes["example.weather#Weather"]["errors"] = [ + {"target": "example.weather#Throttled"} + ] + shapes["example.weather#Forecast"] = {"type": "structure"} + shapes["example.weather#City"] = { + "type": "resource", + "identifiers": {"cityId": {"target": "example.weather#CityId"}}, + "properties": {"forecast": {"target": "example.weather#Forecast"}}, } - shapes["smithy.api#String"] = {"type": "string"} - names = _names(Model.from_dict(model_document)) - assert "myTrait" not in names - assert "Auditable" not in names - assert "String" not in names + shapes["example.weather#Weather"]["resources"] = [ + {"target": "example.weather#City"} + ] + model = Model.from_dict(model_document) + + names = _names(select_generated_shapes(model, _service(model)).shapes) + assert "Throttled" in names + assert "Forecast" in names + assert "City" not in names - def test_private_shapes_are_generated_only_when_referenced( + def test_excludes_traits_mixins_and_prelude_even_when_connected( self, model_document: dict[str, Any] ) -> None: shapes = model_document["shapes"] - shapes["example.weather#Hidden"] = { - "type": "structure", - "traits": {"smithy.api#private": {}}, - } - shapes["example.weather#Nested"] = { + shapes["example.weather#Auditable"] = { "type": "structure", - "traits": {"smithy.api#private": {}}, + "traits": {"smithy.api#mixin": {}}, + "members": {"createdAt": {"target": "smithy.api#Timestamp"}}, } - shapes["example.weather#Used"] = { + shapes["example.weather#Coordinates"]["mixins"] = [ + {"target": "example.weather#Auditable"} + ] + shapes["example.weather#myTrait"] = { "type": "structure", - "traits": {"smithy.api#private": {}}, - "members": {"nested": {"target": "example.weather#Nested"}}, - } - shapes["example.weather#Coordinates"]["members"]["used"] = { - "target": "example.weather#Used" + "traits": {"smithy.api#trait": {}}, } - names = _names(Model.from_dict(model_document)) - assert "Used" in names - assert "Nested" in names - assert "Hidden" not in names + shapes["smithy.api#String"] = {"type": "string"} + model = Model.from_dict(model_document) + + selection = select_generated_shapes(model, _service(model)) + all_names = _names(selection.shapes) + _names(selection.excluded) + assert "Auditable" not in all_names + assert "myTrait" not in all_names + assert "String" not in all_names - def test_private_shapes_referenced_by_operations_are_generated( + def test_conflicting_names_outside_the_closure_are_harmless( self, model_document: dict[str, Any] ) -> None: - shapes = model_document["shapes"] - shapes["example.weather#NoSuchCity"]["traits"]["smithy.api#private"] = {} - assert "NoSuchCity" in _names(Model.from_dict(model_document)) + model_document["shapes"]["example.other#coordinates"] = {"type": "string"} + model = Model.from_dict(model_document) + selection = select_generated_shapes(model, _service(model)) + assert "Coordinates" in _names(selection.shapes) + assert "coordinates" in _names(selection.excluded) + + +class TestSelectWithoutService: + def test_selects_every_data_shape_in_model_order( + self, model_document: dict[str, Any] + ) -> None: + del model_document["shapes"]["example.weather#Weather"] + model = Model.from_dict(model_document) + selection = select_generated_shapes(model, None) + assert _names(selection.shapes) == [ + "CityId", + "Coordinates", + "Tags", + "GetCityInput", + "GetCityOutput", + "NoSuchCity", + "Unused", + ] + assert selection.excluded == () def test_case_insensitive_name_conflicts_are_an_error( self, model_document: dict[str, Any] ) -> None: model_document["shapes"]["example.other#coordinates"] = {"type": "string"} + model = Model.from_dict(model_document) with pytest.raises(CodegenError) as info: - select_generated_shapes(Model.from_dict(model_document)) + select_generated_shapes(model, None) message = str(info.value) assert "renameShapes" in message assert "example.weather#Coordinates, example.other#coordinates" in message - - def test_conflicts_with_skipped_shapes_are_ignored( - self, model_document: dict[str, Any] - ) -> None: - model_document["shapes"]["example.other#Coordinates"] = { - "type": "structure", - "traits": {"smithy.api#private": {}}, - } - assert "Coordinates" in _names(Model.from_dict(model_document)) From b1880459fcf1093c3f5c31cf058bf2cc8e540caa Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sat, 12 Sep 2026 22:41:20 -0400 Subject: [PATCH 13/19] fix(codegen): Merge applies while resolving each mixin Applying traits to inherited members after all mixins were resolved meant a shape using an intermediate mixin copied that mixin's members before its apply statements were merged. Merge each shape's applies as part of resolving it, before anything inherits from it, and drop the two-pass bookkeeping. --- .../smithy-python/src/smithy_python/model.py | 154 +++++++++--------- .../smithy-python/tests/unit/test_model.py | 32 ++++ 2 files changed, 107 insertions(+), 79 deletions(-) diff --git a/packages/smithy-python/src/smithy_python/model.py b/packages/smithy-python/src/smithy_python/model.py index 45e97eceb..504e15e28 100644 --- a/packages/smithy-python/src/smithy_python/model.py +++ b/packages/smithy-python/src/smithy_python/model.py @@ -6,7 +6,7 @@ import json import re -from collections.abc import Iterable, Iterator, Mapping +from collections.abc import Callable, Iterable, Iterator, Mapping from dataclasses import dataclass, field, replace from enum import StrEnum from types import MappingProxyType @@ -240,25 +240,22 @@ def from_dict(cls, document: Mapping[str, object]) -> Self: metadata = _json_object(document.get("metadata", {}), "Smithy model metadata") shapes: list[Shape] = [] - applies: list[_Apply] = [] + applies: dict[ShapeID, list[_Apply]] = {} for shape_id, unparsed_node in shapes_node.items(): node = _object_mapping(unparsed_node, f"shape {shape_id}") parsed_id = ShapeID.parse(shape_id) if node.get("type") == "apply": - applies.append( - (parsed_id, _expect_traits(node.get("traits", {}), parsed_id)) + traits = _expect_traits(node.get("traits", {}), parsed_id) + applies.setdefault(parsed_id.without_member(), []).append( + (parsed_id, traits) ) continue shapes.append(_parse_shape(parsed_id, node)) # Serialized models omit everything a shape inherits from its mixins, and - # traits added to inherited members arrive as apply statements. Traits - # applied to members that exist before resolution (including members of - # the mixins themselves) are merged first so that they are inherited; - # the rest target inherited members and are merged after resolution. - shapes, deferred = _apply_traits(shapes, applies, defer_missing_members=True) - shapes = _resolve_mixins(shapes) - shapes, _ = _apply_traits(shapes, deferred, defer_missing_members=False) + # traits added to inherited members arrive as apply statements. Both are + # resolved together so that shapes using a mixin see its applied traits. + shapes = _resolve_shapes(shapes, applies) return cls(smithy=version, metadata=_mapping(metadata), shapes=tuple(shapes)) def __iter__(self) -> Iterator[Shape]: @@ -423,97 +420,96 @@ def _json_value(value: object, location: str) -> JSONValue: type _Apply = tuple[ShapeID, Mapping[str, JSONValue]] -def _apply_traits( - shapes: list[Shape], applies: list[_Apply], *, defer_missing_members: bool -) -> tuple[list[Shape], list[_Apply]]: - """Merge apply statements into shapes, returning any that were deferred.""" - positions = {shape.id: index for index, shape in enumerate(shapes)} - deferred: list[_Apply] = [] - for target, traits in applies: - container_id = target.without_member() - if container_id not in positions: - raise ModelError(f"Apply target not found: {target}") - position = positions[container_id] - shape = shapes[position] - if target.member is None: - shapes[position] = replace( - shape, traits=_mapping({**shape.traits, **traits}) - ) - continue - members = list(shape.members) - for index, member in enumerate(members): - if member.name == target.member: - members[index] = replace( - member, traits=_mapping({**member.traits, **traits}) - ) - shapes[position] = replace(shape, members=tuple(members)) - break - else: - if not defer_missing_members: - raise ModelError(f"Apply target member not found: {target}") - deferred.append((target, traits)) - return shapes, deferred - - -def _resolve_mixins(shapes: list[Shape]) -> list[Shape]: - """Copy inherited traits, members, and properties onto shapes using mixins. +def _resolve_shapes( + shapes: list[Shape], applies: Mapping[ShapeID, list[_Apply]] +) -> list[Shape]: + """Copy inherited definitions onto shapes using mixins and merge applies. Follows the resolution rules of the Smithy mixins specification: inherited members precede local members in a depth-first traversal of the mixins, later mixins take precedence over earlier ones, local definitions take precedence over anything inherited, and the ``mixin`` trait itself and any - ``localTraits`` are not inherited. + ``localTraits`` are not inherited. Apply statements targeting a shape are + merged as part of resolving that shape, so shapes that use it as a mixin + inherit the applied traits. """ by_id = {shape.id: shape for shape in shapes} + for container in applies: + if container not in by_id: + raise ModelError(f"Apply target not found: {container}") resolved: dict[ShapeID, Shape] = {} resolving: set[ShapeID] = set() def resolve(shape: Shape) -> Shape: if (done := resolved.get(shape.id)) is not None: return done - if not shape.mixins: - resolved[shape.id] = shape - return shape if shape.id in resolving: raise ModelError(f"Mixin cycle detected at {shape.id}") resolving.add(shape.id) - traits: dict[str, JSONValue] = {} - members: dict[str, Member] = {} - attributes: dict[str, JSONValue] = {} - for mixin_id in shape.mixins: - mixin = by_id.get(mixin_id) - if mixin is None: - raise ModelError(f"Mixin not found: {mixin_id} (used by {shape.id})") - if not mixin.has_trait(MIXIN_TRAIT): - raise ModelError( - f"{shape.id} uses {mixin_id} as a mixin, but it lacks the " - f"{MIXIN_TRAIT} trait" - ) - mixin = resolve(mixin) - traits.update(_inherited_traits(mixin)) - for member in mixin.members: - members[member.name] = _merge_members(members.get(member.name), member) - _merge_attributes(attributes, mixin.attributes) - - traits.update(shape.traits) - for member in shape.members: - members[member.name] = _merge_members(members.get(member.name), member) - _merge_attributes(attributes, shape.attributes) + if shape.mixins: + shape = _merge_mixins(shape, by_id, resolve) + if shape.id in applies: + shape = _apply_traits(shape, applies[shape.id]) - result = replace( - shape, - traits=_mapping(traits), - members=tuple(members.values()), - attributes=_mapping(attributes), - ) resolving.discard(shape.id) - resolved[shape.id] = result - return result + resolved[shape.id] = shape + return shape return [resolve(shape) for shape in shapes] +def _merge_mixins( + shape: Shape, by_id: Mapping[ShapeID, Shape], resolve: Callable[[Shape], Shape] +) -> Shape: + traits: dict[str, JSONValue] = {} + members: dict[str, Member] = {} + attributes: dict[str, JSONValue] = {} + for mixin_id in shape.mixins: + mixin = by_id.get(mixin_id) + if mixin is None: + raise ModelError(f"Mixin not found: {mixin_id} (used by {shape.id})") + if not mixin.has_trait(MIXIN_TRAIT): + raise ModelError( + f"{shape.id} uses {mixin_id} as a mixin, but it lacks the " + f"{MIXIN_TRAIT} trait" + ) + mixin = resolve(mixin) + traits.update(_inherited_traits(mixin)) + for member in mixin.members: + members[member.name] = _merge_members(members.get(member.name), member) + _merge_attributes(attributes, mixin.attributes) + + traits.update(shape.traits) + for member in shape.members: + members[member.name] = _merge_members(members.get(member.name), member) + _merge_attributes(attributes, shape.attributes) + + return replace( + shape, + traits=_mapping(traits), + members=tuple(members.values()), + attributes=_mapping(attributes), + ) + + +def _apply_traits(shape: Shape, applies: list[_Apply]) -> Shape: + """Merge apply statements targeting a shape or its members.""" + members = {member.name: member for member in shape.members} + traits = dict(shape.traits) + for target, applied in applies: + if target.member is None: + traits.update(applied) + continue + member = members.get(target.member) + if member is None: + raise ModelError(f"Apply target member not found: {target}") + members[target.member] = replace( + member, traits=_mapping({**member.traits, **applied}) + ) + return replace(shape, traits=_mapping(traits), members=tuple(members.values())) + + def _inherited_traits(mixin: Shape) -> dict[str, JSONValue]: excluded = {MIXIN_TRAIT} mixin_trait = mixin.trait(MIXIN_TRAIT) diff --git a/packages/smithy-python/tests/unit/test_model.py b/packages/smithy-python/tests/unit/test_model.py index 4d731689c..cdacb83de 100644 --- a/packages/smithy-python/tests/unit/test_model.py +++ b/packages/smithy-python/tests/unit/test_model.py @@ -378,6 +378,38 @@ def test_apply_targets_inherited_members(self) -> None: not model.expect("example#M").member("foo").has_trait("smithy.api#required") ) + def test_apply_on_intermediate_mixin_propagates_to_users(self) -> None: + # A <- B <- C, with an apply on B$foo, which B inherits from A. + model = Model.from_dict( + self._document( + { + "example#A": { + "type": "structure", + "traits": {"smithy.api#mixin": {}}, + "members": {"foo": {"target": "smithy.api#String"}}, + }, + "example#B": { + "type": "structure", + "traits": {"smithy.api#mixin": {}}, + "mixins": [{"target": "example#A"}], + }, + "example#C": { + "type": "structure", + "mixins": [{"target": "example#B"}], + }, + "example#B$foo": { + "type": "apply", + "traits": {"smithy.api#required": {}}, + }, + } + ) + ) + assert model.expect("example#B").member("foo").has_trait("smithy.api#required") + assert model.expect("example#C").member("foo").has_trait("smithy.api#required") + assert ( + not model.expect("example#A").member("foo").has_trait("smithy.api#required") + ) + def test_redefined_members_merge_traits_and_keep_position(self) -> None: model = Model.from_dict( self._document( From d309f6e372b509742ea0cd8ead2d8eb7fa550076 Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sat, 12 Sep 2026 22:43:07 -0400 Subject: [PATCH 14/19] docs(codegen): State that nested model values are shared The model's containers are read-only views, but the JSON values inside them are not copied. Say so rather than claiming full immutability. --- packages/smithy-python/src/smithy_python/model.py | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/packages/smithy-python/src/smithy_python/model.py b/packages/smithy-python/src/smithy_python/model.py index 504e15e28..ff4f8184a 100644 --- a/packages/smithy-python/src/smithy_python/model.py +++ b/packages/smithy-python/src/smithy_python/model.py @@ -1,6 +1,12 @@ # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 -"""Ordered, immutable objects for Smithy's JSON AST representation.""" +"""Ordered, read-only objects for Smithy's JSON AST representation. + +Shapes, members, and models are frozen dataclasses whose trait, metadata, and +attribute containers are read-only views. Values nested inside those containers +are plain JSON objects shared with the parsed document and are not copied; they +MUST be treated as read-only by callers. +""" from __future__ import annotations @@ -102,7 +108,8 @@ def _mapping( value: Mapping[str, JSONValue] | None = None, ) -> Mapping[str, JSONValue]: # A fresh dict preserves JSON insertion order while MappingProxyType prevents - # accidental mutation through a frozen dataclass. + # accidental mutation of the container. Nested values are not copied: they are + # ordinary JSON objects that callers must treat as read-only. return MappingProxyType(dict(value or {})) From 353f943d9c4792142d9b6dc90b3dd6eb1e66d51b Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sat, 12 Sep 2026 22:53:20 -0400 Subject: [PATCH 15/19] fix(codegen): Make nested model values immutable Trait and attribute values inherited through mixins are shared between the mixin and every shape that uses it, so a mutable nested value let one shape's consumer corrupt its siblings. Freeze JSON objects as read-only mappings and arrays as tuples when parsing, and narrow JSONValue to the read-only abstract types so pyright rejects mutation statically as well. --- .../smithy-python/src/smithy_python/model.py | 66 ++++++++++--------- .../smithy-python/tests/unit/test_model.py | 48 +++++++++++++- 2 files changed, 81 insertions(+), 33 deletions(-) diff --git a/packages/smithy-python/src/smithy_python/model.py b/packages/smithy-python/src/smithy_python/model.py index ff4f8184a..2d25165d7 100644 --- a/packages/smithy-python/src/smithy_python/model.py +++ b/packages/smithy-python/src/smithy_python/model.py @@ -1,18 +1,18 @@ # Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 -"""Ordered, read-only objects for Smithy's JSON AST representation. +"""Ordered, immutable objects for Smithy's JSON AST representation. -Shapes, members, and models are frozen dataclasses whose trait, metadata, and -attribute containers are read-only views. Values nested inside those containers -are plain JSON objects shared with the parsed document and are not copied; they -MUST be treated as read-only by callers. +Shapes, members, and models are frozen dataclasses. Trait, metadata, and +attribute values are deeply immutable: JSON objects are exposed as read-only +mappings and JSON arrays as tuples, so values inherited through mixins can be +shared between shapes without one shape's consumer affecting another. """ from __future__ import annotations import json import re -from collections.abc import Callable, Iterable, Iterator, Mapping +from collections.abc import Callable, Iterable, Iterator, Mapping, Sequence from dataclasses import dataclass, field, replace from enum import StrEnum from types import MappingProxyType @@ -21,7 +21,7 @@ from .exceptions import ModelError type JSONValue = ( - None | bool | int | float | str | list[JSONValue] | dict[str, JSONValue] + None | bool | int | float | str | tuple[JSONValue, ...] | Mapping[str, JSONValue] ) PRELUDE_NAMESPACE = "smithy.api" @@ -108,8 +108,7 @@ def _mapping( value: Mapping[str, JSONValue] | None = None, ) -> Mapping[str, JSONValue]: # A fresh dict preserves JSON insertion order while MappingProxyType prevents - # accidental mutation of the container. Nested values are not copied: they are - # ordinary JSON objects that callers must treat as read-only. + # mutation. Nested values are already frozen by _json_value. return MappingProxyType(dict(value or {})) @@ -176,7 +175,7 @@ def references(self) -> tuple[ShapeID, ...]: result.append(_reference(value, f"{self.id}.{key}")) for key in ("identifiers", "properties"): values = self.attributes.get(key) - if isinstance(values, dict): + if isinstance(values, Mapping): result.extend( _reference(value, f"{self.id}.{key}.{name}") for name, value in values.items() @@ -186,7 +185,7 @@ def references(self) -> tuple[ShapeID, ...]: # Prelude shapes are omitted from the JSON AST unless the build opts in, so they # are resolved on demand when a member targets one. -_PRELUDE_TYPES: dict[str, tuple[ShapeType, dict[str, JSONValue]]] = { +_PRELUDE_TYPES: dict[str, tuple[ShapeType, Mapping[str, JSONValue]]] = { "Blob": (ShapeType.BLOB, {}), "Boolean": (ShapeType.BOOLEAN, {}), "String": (ShapeType.STRING, {}), @@ -207,7 +206,7 @@ def references(self) -> tuple[ShapeID, ...]: "PrimitiveLong": (ShapeType.LONG, {"smithy.api#default": 0}), "PrimitiveFloat": (ShapeType.FLOAT, {"smithy.api#default": 0}), "PrimitiveDouble": (ShapeType.DOUBLE, {"smithy.api#default": 0}), - "Unit": (ShapeType.STRUCTURE, {"smithy.api#unitType": {}}), + "Unit": (ShapeType.STRUCTURE, {"smithy.api#unitType": MappingProxyType({})}), } @@ -370,10 +369,10 @@ def _expect_traits(value: object, target: ShapeID) -> Mapping[str, JSONValue]: return _mapping(_json_object(value, f"traits on {target}")) -def _expect_list(value: object, location: str) -> list[object]: - if not isinstance(value, list): +def _expect_list(value: object, location: str) -> Sequence[object]: + if not isinstance(value, list | tuple): raise ModelError(f"Expected a list at {location}") - return cast(list[object], value) + return cast(Sequence[object], value) def _reference(value: object, location: str) -> ShapeID: @@ -407,18 +406,23 @@ def _object_mapping(value: object, location: str) -> dict[str, object]: return result -def _json_object(value: object, location: str) -> dict[str, JSONValue]: - return { - key: _json_value(item, f"{location}.{key}") - for key, item in _object_mapping(value, location).items() - } +def _json_object(value: object, location: str) -> Mapping[str, JSONValue]: + return MappingProxyType( + { + key: _json_value(item, f"{location}.{key}") + for key, item in _object_mapping(value, location).items() + } + ) def _json_value(value: object, location: str) -> JSONValue: + """Convert a decoded JSON value into its deeply immutable form.""" if value is None or isinstance(value, bool | int | float | str): return value - if isinstance(value, list): - return [_json_value(item, location) for item in cast(list[object], value)] + if isinstance(value, list | tuple): + return tuple( + _json_value(item, location) for item in cast(Sequence[object], value) + ) if isinstance(value, Mapping): return _json_object(cast(object, value), location) raise ModelError(f"Unsupported JSON value at {location}: {type(value).__name__}") @@ -520,9 +524,9 @@ def _apply_traits(shape: Shape, applies: list[_Apply]) -> Shape: def _inherited_traits(mixin: Shape) -> dict[str, JSONValue]: excluded = {MIXIN_TRAIT} mixin_trait = mixin.trait(MIXIN_TRAIT) - if isinstance(mixin_trait, dict): - local_traits = mixin_trait.get("localTraits", []) - if isinstance(local_traits, list): + if isinstance(mixin_trait, Mapping): + local_traits = mixin_trait.get("localTraits", ()) + if isinstance(local_traits, tuple): excluded.update(name for name in local_traits if isinstance(name, str)) return {name: value for name, value in mixin.traits.items() if name not in excluded} @@ -544,17 +548,17 @@ def _merge_attributes( ) -> None: """Merge shape properties, giving ``source`` precedence. - Lists are concatenated without duplicates, objects are merged key by key, + Arrays are concatenated without duplicates, objects are merged key by key, and scalars from ``source`` replace existing values. """ for key, value in source.items(): existing = target.get(key) - if isinstance(existing, list) and isinstance(value, list): - target[key] = [ + if isinstance(existing, tuple) and isinstance(value, tuple): + target[key] = ( *existing, *(item for item in value if item not in existing), - ] - elif isinstance(existing, dict) and isinstance(value, dict): - target[key] = {**existing, **value} + ) + elif isinstance(existing, Mapping) and isinstance(value, Mapping): + target[key] = MappingProxyType({**existing, **value}) else: target[key] = value diff --git a/packages/smithy-python/tests/unit/test_model.py b/packages/smithy-python/tests/unit/test_model.py index cdacb83de..82ae9e9c5 100644 --- a/packages/smithy-python/tests/unit/test_model.py +++ b/packages/smithy-python/tests/unit/test_model.py @@ -3,6 +3,7 @@ from __future__ import annotations +from collections.abc import Mapping from typing import Any import pytest @@ -60,6 +61,15 @@ def test_parsed_objects_are_immutable(self, model: Model) -> None: with pytest.raises(TypeError): coordinates.traits["example#trait"] = {} # type: ignore[index] + def test_nested_values_are_immutable(self, model: Model) -> None: + http = model.expect("example.weather#GetCity").trait("smithy.api#http") + assert isinstance(http, Mapping) + with pytest.raises(TypeError): + http["method"] = "POST" # type: ignore[index] + errors = model.expect("example.weather#GetCity").attributes["errors"] + assert isinstance(errors, tuple) + assert model.metadata["example"] is True + def test_from_json_accepts_bytes(self, model_json: bytes, model: Model) -> None: assert Model.from_json(model_json) == model @@ -410,6 +420,40 @@ def test_apply_on_intermediate_mixin_propagates_to_users(self) -> None: not model.expect("example#A").member("foo").has_trait("smithy.api#required") ) + def test_inherited_values_cannot_be_mutated_through_a_sibling(self) -> None: + model = Model.from_dict( + self._document( + { + "example#M": { + "type": "structure", + "traits": { + "smithy.api#mixin": {}, + "smithy.api#tags": ["shared"], + "smithy.api#http": {"method": "GET", "uri": "/"}, + }, + }, + "example#S": { + "type": "structure", + "mixins": [{"target": "example#M"}], + }, + "example#T": { + "type": "structure", + "mixins": [{"target": "example#M"}], + }, + } + ) + ) + s_http = model.expect("example#S").trait("smithy.api#http") + assert isinstance(s_http, Mapping) + with pytest.raises(TypeError): + s_http["method"] = "POST" # type: ignore[index] + tags = model.expect("example#S").trait("smithy.api#tags") + assert isinstance(tags, tuple) + assert model.expect("example#T").trait("smithy.api#http") == { + "method": "GET", + "uri": "/", + } + def test_redefined_members_merge_traits_and_keep_position(self) -> None: model = Model.from_dict( self._document( @@ -533,10 +577,10 @@ def test_operation_errors_are_inherited(self) -> None: } ) ) - assert model.expect("example#GetUser").attributes["errors"] == [ + assert model.expect("example#GetUser").attributes["errors"] == ( {"target": "example#ValidationError"}, {"target": "example#NotFound"}, - ] + ) @pytest.mark.parametrize( ("shapes", "message"), From 560ac4dd7d5d37cdc8264887744f389dad271b60 Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sun, 13 Sep 2026 02:01:07 -0400 Subject: [PATCH 16/19] refactor(codegen): Simplify the model loader and the CLI Share one empty mapping across the whole model instead of allocating a proxy per shape, layer the prelude under the shape index so a lookup is a single step rather than a fallback chain, and replace the branch ladder in Shape.references with a table naming the attributes that hold references. Move the trait accessors that Member and Shape each defined to a shared base, and the member scan that Shape and Model each open-coded to Shape.get_member. Parsing a 2.5 MB model is about 35% faster and retains 23% less memory as a result. Collapse the CLI's two-layer invocation handoff into one request built by one function, and route its output through helpers that own the program prefix so it is spelled once. Tighten the behaviors the loader was lenient about along the way: an apply statement must target a member, since a shape's own traits are serialized with its definition; a mixin must have the same shape type as the shape using it; a list or map that inherits its members no longer needs to restate them; and a member ID resolves only when the shape actually defines that member. Require --output for direct invocation rather than defaulting it, and reject it alongside SMITHY_PLUGIN_DIR instead of silently letting the plugin's directory win. --- designs/codegen/cli.md | 5 +- designs/codegen/index.md | 7 + .../smithy-python/src/smithy_python/cli.py | 140 ++++--- .../smithy-python/src/smithy_python/model.py | 317 +++++++++------- .../src/smithy_python/selection.py | 6 +- packages/smithy-python/tests/unit/conftest.py | 32 +- packages/smithy-python/tests/unit/test_cli.py | 341 +++++++----------- .../smithy-python/tests/unit/test_model.py | 117 +++++- 8 files changed, 556 insertions(+), 409 deletions(-) diff --git a/designs/codegen/cli.md b/designs/codegen/cli.md index d84f5a8e9..5ee56ea49 100644 --- a/designs/codegen/cli.md +++ b/designs/codegen/cli.md @@ -19,8 +19,9 @@ generates a standalone package containing only data shapes. Both commands accept the following process options: * `--model PATH` reads a JSON AST from a file instead of standard input. -* `--output PATH` selects the output directory. It defaults to the Smithy run - plugin's output directory (`SMITHY_PLUGIN_DIR`) when invoked by Smithy. +* `--output PATH` selects the output directory. It is required for direct + invocation and MUST NOT be used when the Smithy run plugin supplies the output + directory (`SMITHY_PLUGIN_DIR`). Settings specific to each artifact will be added with the functionality that consumes them. diff --git a/designs/codegen/index.md b/designs/codegen/index.md index 9901b8be1..3cf55e267 100644 --- a/designs/codegen/index.md +++ b/designs/codegen/index.md @@ -47,6 +47,13 @@ loaded into the Smithy CLI or implemented in Java. Generated packages MUST NOT depend on `smithy-python` at runtime. They MAY depend on the handwritten runtime packages in this repository. +The generator has no runtime dependencies of its own, including on those +packages. It therefore defines its own shape IDs, shape types, and prelude +rather than reusing `smithy-core`'s. Those are shaped for serializing values at +runtime, whereas the generator needs the JSON AST's own vocabulary: wire-format +type names, member IDs, and lossless shape attributes. The overlap between the +two is intentional. + ## Migration The Java generator remains authoritative while the Python generator is under diff --git a/packages/smithy-python/src/smithy_python/cli.py b/packages/smithy-python/src/smithy_python/cli.py index 83f2cd396..cd6b3ec7e 100644 --- a/packages/smithy-python/src/smithy_python/cli.py +++ b/packages/smithy-python/src/smithy_python/cli.py @@ -5,7 +5,6 @@ from __future__ import annotations import argparse -import os import sys from collections.abc import Mapping, Sequence from dataclasses import dataclass @@ -18,28 +17,27 @@ from .model import Model, Shape, ShapeID from .selection import Selection, resolve_service, select_generated_shapes -_GENERATION_NOT_IMPLEMENTED: Final = ( - "smithy-python: error: {artifact} generation is not implemented yet\n" -) +_PROGRAM: Final = "smithy-python" _CLIENT_ARTIFACT: Final = "client" -@dataclass(frozen=True, slots=True) -class _Invocation: - artifact: str - model_source: bytes - output_dir: Path - environment: PluginEnvironment - service: ShapeID | None +def _write_error(message: str) -> None: + sys.stderr.write(f"{_PROGRAM}: error: {message}\n") + + +def _write_note(message: str) -> None: + sys.stderr.write(f"{_PROGRAM}: note: {message}\n") @dataclass(frozen=True, slots=True) class _Request: - """A fully resolved generation request.""" + """The fully resolved generation request of a single run.""" - invocation: _Invocation + artifact: str model: Model + output_dir: Path + environment: PluginEnvironment service: Shape | None selection: Selection @@ -59,46 +57,21 @@ def main( return error.code if isinstance(error.code, int) else 1 try: - invocation = _resolve_invocation( - args, - environ=os.environ if environ is None else environ, - stdin=stdin, - ) - _resolve_request(invocation) + request = _resolve_request(args, environ=environ, stdin=stdin) except InvalidInvocationError as error: - sys.stderr.write(f"smithy-python: error: {error}\n") + _write_error(str(error)) return 2 except (CodegenError, OSError) as error: - sys.stderr.write(f"smithy-python: error: {error}\n") + _write_error(str(error)) return 1 - sys.stderr.write(_GENERATION_NOT_IMPLEMENTED.format(artifact=args.artifact)) + _write_error(f"{request.artifact} generation is not implemented yet") return 1 -def _resolve_request(invocation: _Invocation) -> _Request: - """Load the model and resolve what the artifact will generate.""" - # The raw bytes are dropped as soon as the model is parsed. - model = Model.from_json(invocation.model_source) - service = resolve_service( - model, - invocation.service, - required=invocation.artifact == _CLIENT_ARTIFACT, - ) - selection = select_generated_shapes(model, service) - if selection.excluded and service is not None: - sys.stderr.write( - f"smithy-python: note: {len(selection.excluded)} shape(s) not connected " - f"to {service.id} will not be generated\n" - ) - return _Request( - invocation=invocation, model=model, service=service, selection=selection - ) - - def _create_parser() -> argparse.ArgumentParser: parser = argparse.ArgumentParser( - prog="smithy-python", + prog=_PROGRAM, description="Generate Python source from Smithy models.", ) parser.add_argument( @@ -112,7 +85,7 @@ def _create_parser() -> argparse.ArgumentParser: artifacts = generate.add_subparsers(dest="artifact", required=True) common = _common_artifact_options() for name, help_text in ( - ("client", "Generate a client package"), + (_CLIENT_ARTIFACT, "Generate a client package"), ("types", "Generate a standalone types package"), ): artifacts.add_parser(name, help=help_text, parents=[common]) @@ -135,8 +108,8 @@ def _common_artifact_options() -> argparse.ArgumentParser: "--output", type=Path, help=( - "Output directory for generated files. Defaults to the Smithy run " - "plugin's output directory (SMITHY_PLUGIN_DIR) when invoked by Smithy." + "Output directory for generated files. Required unless the Smithy run " + "plugin provides one (SMITHY_PLUGIN_DIR), which this cannot override." ), ) parser.add_argument( @@ -150,31 +123,61 @@ def _common_artifact_options() -> argparse.ArgumentParser: return parser -def _resolve_invocation( +def _resolve_request( args: argparse.Namespace, *, - environ: Mapping[str, str], + environ: Mapping[str, str] | None, stdin: BinaryIO | None, -) -> _Invocation: +) -> _Request: + """Resolve the process inputs of a single run into a generation request.""" environment = PluginEnvironment.from_environ(environ) - model_path: Path | None = args.model - output_path: Path | None = args.output + output_dir = _resolve_output_dir(args, environment) + requested_service = _parse_service(args.service) + # Parsing here keeps the undecoded model out of memory for the rest of the run. + model = Model.from_json(_read_model(args.model, environment, stdin)) + + service = resolve_service( + model, requested_service, required=args.artifact == _CLIENT_ARTIFACT + ) + selection = select_generated_shapes(model, service) + if selection.excluded and service is not None: + _write_note( + f"{len(selection.excluded)} shape(s) not connected to {service.id} " + f"will not be generated" + ) + + return _Request( + artifact=args.artifact, + model=model, + output_dir=output_dir, + environment=environment, + service=service, + selection=selection, + ) + +def _resolve_output_dir( + args: argparse.Namespace, environment: PluginEnvironment +) -> Path: + """Return the directory generated files are written to.""" if (plugin_dir := environment.plugin_dir) is not None: - if model_path is not None: - raise InvalidInvocationError( - "--model cannot be used with the Smithy run plugin" - ) - if output_path is not None: - raise InvalidInvocationError( - "--output cannot be used with the Smithy run plugin" - ) - output_dir = plugin_dir - else: - if output_path is None: - raise InvalidInvocationError("Direct invocation requires --output") - output_dir = output_path + for flag, value in (("--model", args.model), ("--output", args.output)): + if value is not None: + raise InvalidInvocationError( + f"{flag} cannot be used with the Smithy run plugin" + ) + return plugin_dir + + output_path: Path | None = args.output + if output_path is None: + raise InvalidInvocationError("Direct invocation requires --output") + return output_path + +def _read_model( + model_path: Path | None, environment: PluginEnvironment, stdin: BinaryIO | None +) -> bytes: + """Read the JSON AST document from the model file or standard input.""" if model_path is not None: if not model_path.is_file(): raise InvalidInvocationError(f"Model path is not a file: {model_path}") @@ -188,14 +191,7 @@ def _resolve_invocation( model_source = model_stream.read() if not model_source: raise InvalidInvocationError("Expected a Smithy JSON AST model") - - return _Invocation( - artifact=args.artifact, - model_source=model_source, - output_dir=output_dir, - environment=environment, - service=_parse_service(args.service), - ) + return model_source def _parse_service(value: str | None) -> ShapeID | None: diff --git a/packages/smithy-python/src/smithy_python/model.py b/packages/smithy-python/src/smithy_python/model.py index 2d25165d7..3f19e986f 100644 --- a/packages/smithy-python/src/smithy_python/model.py +++ b/packages/smithy-python/src/smithy_python/model.py @@ -15,8 +15,9 @@ from collections.abc import Callable, Iterable, Iterator, Mapping, Sequence from dataclasses import dataclass, field, replace from enum import StrEnum +from functools import total_ordering from types import MappingProxyType -from typing import Self, cast +from typing import Final, Self, cast from .exceptions import ModelError @@ -26,6 +27,7 @@ PRELUDE_NAMESPACE = "smithy.api" MIXIN_TRAIT = "smithy.api#mixin" +TRAIT_DEFINITION = "smithy.api#trait" class ShapeType(StrEnum): @@ -64,7 +66,8 @@ def is_service_category(self) -> bool: _NAMESPACE = re.compile(rf"{_IDENTIFIER.pattern}(?:\.{_IDENTIFIER.pattern})*") -@dataclass(frozen=True, slots=True, order=True) +@total_ordering +@dataclass(frozen=True, slots=True) class ShapeID: """An absolute Smithy shape ID, optionally identifying a member.""" @@ -73,12 +76,12 @@ class ShapeID: member: str | None = None def __post_init__(self) -> None: - if not _NAMESPACE.fullmatch(self.namespace) or not _IDENTIFIER.fullmatch( - self.name + if not ( + _NAMESPACE.fullmatch(self.namespace) + and _IDENTIFIER.fullmatch(self.name) + and (self.member is None or _IDENTIFIER.fullmatch(self.member)) ): raise ModelError(f"Invalid shape ID: {self}") - if self.member is not None and not _IDENTIFIER.fullmatch(self.member): - raise ModelError(f"Invalid shape ID: {self}") @classmethod def parse(cls, value: str) -> Self: @@ -93,32 +96,58 @@ def with_member(self, member: str) -> Self: return type(self)(namespace=self.namespace, name=self.name, member=member) def without_member(self) -> Self: + if self.member is None: + return self return type(self)(namespace=self.namespace, name=self.name) @property def is_prelude(self) -> bool: return self.namespace == PRELUDE_NAMESPACE + def __lt__(self, other: ShapeID) -> bool: + """Order by namespace, then shape name, then member name.""" + return (self.namespace, self.name, self.member or "") < ( + other.namespace, + other.name, + other.member or "", + ) + def __str__(self) -> str: value = f"{self.namespace}#{self.name}" return f"{value}${self.member}" if self.member is not None else value +# Frozen values are shared rather than copied, so every empty mapping in the +# model can be the same object. +_EMPTY_MAPPING: Final[Mapping[str, JSONValue]] = MappingProxyType({}) + + +def _freeze(value: dict[str, JSONValue]) -> Mapping[str, JSONValue]: + """Expose a dict this module owns as a read-only mapping.""" + return MappingProxyType(value) if value else _EMPTY_MAPPING + + def _mapping( value: Mapping[str, JSONValue] | None = None, ) -> Mapping[str, JSONValue]: # A fresh dict preserves JSON insertion order while MappingProxyType prevents # mutation. Nested values are already frozen by _json_value. - return MappingProxyType(dict(value or {})) + return _freeze(dict(value)) if value else _EMPTY_MAPPING -@dataclass(frozen=True, slots=True) -class Member: - """A member of an aggregate shape, in modeled order.""" +def _merged( + base: Mapping[str, JSONValue], overrides: Mapping[str, JSONValue] +) -> Mapping[str, JSONValue]: + """Freeze the union of two frozen mappings, ``overrides`` taking precedence.""" + return _freeze({**base, **overrides}) - name: str - target: ShapeID - traits: Mapping[str, JSONValue] = field(default_factory=_mapping) + +class _Traited: + """Trait lookups shared by the AST nodes that carry traits.""" + + __slots__ = () + + traits: Mapping[str, JSONValue] def has_trait(self, trait: str) -> bool: return trait in self.traits @@ -128,7 +157,16 @@ def trait(self, trait: str, default: JSONValue = None) -> JSONValue: @dataclass(frozen=True, slots=True) -class Shape: +class Member(_Traited): + """A member of an aggregate shape, in modeled order.""" + + name: str + target: ShapeID + traits: Mapping[str, JSONValue] = field(default_factory=_mapping) + + +@dataclass(frozen=True, slots=True) +class Shape(_Traited): """A Smithy shape with ordered members and lossless shape-specific fields.""" id: ShapeID @@ -138,53 +176,33 @@ class Shape: members: tuple[Member, ...] = () attributes: Mapping[str, JSONValue] = field(default_factory=_mapping) - def has_trait(self, trait: str) -> bool: - return trait in self.traits - - def trait(self, trait: str, default: JSONValue = None) -> JSONValue: - return self.traits.get(trait, default) - - def member(self, name: str) -> Member: + def get_member(self, name: str) -> Member | None: + """Return a member by name, or ``None`` when the shape lacks it.""" for member in self.members: if member.name == name: return member - raise ModelError(f"Member not found: {self.id}${name}") + return None + + def member(self, name: str) -> Member: + """Return a member by name or raise :class:`ModelError` if it is absent.""" + if (member := self.get_member(name)) is None: + raise ModelError(f"Member not found: {self.id}${name}") + return member def references(self) -> tuple[ShapeID, ...]: """Return all structural references in stable modeled order.""" result = [*self.mixins, *(member.target for member in self.members)] - for key in ( - "operations", - "resources", - "errors", - "collectionOperations", - ): - result.extend(_reference_list(self.attributes.get(key), f"{self.id}.{key}")) - for key in ( - "input", - "output", - "create", - "put", - "read", - "update", - "delete", - "list", - ): - value = self.attributes.get(key) - if value is not None: - result.append(_reference(value, f"{self.id}.{key}")) - for key in ("identifiers", "properties"): - values = self.attributes.get(key) - if isinstance(values, Mapping): - result.extend( - _reference(value, f"{self.id}.{key}.{name}") - for name, value in values.items() - ) + # Only shapes in the service category carry reference attributes, so most + # shapes skip the table entirely. + if self.attributes: + for key, extract in _REFERENCE_ATTRIBUTES.items(): + if (value := self.attributes.get(key)) is not None: + result.extend(extract(value, f"{self.id}.{key}")) return tuple(dict.fromkeys(result)) # Prelude shapes are omitted from the JSON AST unless the build opts in, so they -# are resolved on demand when a member targets one. +# are layered underneath every model's shapes to resolve on lookup. _PRELUDE_TYPES: dict[str, tuple[ShapeType, Mapping[str, JSONValue]]] = { "Blob": (ShapeType.BLOB, {}), "Boolean": (ShapeType.BOOLEAN, {}), @@ -206,10 +224,22 @@ def references(self) -> tuple[ShapeID, ...]: "PrimitiveLong": (ShapeType.LONG, {"smithy.api#default": 0}), "PrimitiveFloat": (ShapeType.FLOAT, {"smithy.api#default": 0}), "PrimitiveDouble": (ShapeType.DOUBLE, {"smithy.api#default": 0}), - "Unit": (ShapeType.STRUCTURE, {"smithy.api#unitType": MappingProxyType({})}), + "Unit": (ShapeType.STRUCTURE, {"smithy.api#unitType": _EMPTY_MAPPING}), } +def _prelude_shapes() -> Mapping[ShapeID, Shape]: + shapes: dict[ShapeID, Shape] = {} + for name, (shape_type, traits) in _PRELUDE_TYPES.items(): + shape_id = ShapeID(namespace=PRELUDE_NAMESPACE, name=name) + shapes[shape_id] = Shape(id=shape_id, type=shape_type, traits=_mapping(traits)) + return MappingProxyType(shapes) + + +# Built once so that every reference to a prelude shape resolves to one object. +_PRELUDE_SHAPES: Final = _prelude_shapes() + + @dataclass(frozen=True, slots=True) class Model: """An ordered Smithy model parsed from a JSON AST document.""" @@ -225,7 +255,11 @@ def __post_init__(self) -> None: if shape.id in index: raise ModelError(f"Duplicate shape: {shape.id}") index[shape.id] = shape - object.__setattr__(self, "_index", MappingProxyType(index)) + # Layering the prelude underneath keeps lookups total over it without + # adding shapes the model did not declare to `shapes`. + object.__setattr__( + self, "_index", MappingProxyType({**_PRELUDE_SHAPES, **index}) + ) @classmethod def from_json(cls, source: str | bytes | bytearray) -> Self: @@ -251,9 +285,8 @@ def from_dict(cls, document: Mapping[str, object]) -> Self: node = _object_mapping(unparsed_node, f"shape {shape_id}") parsed_id = ShapeID.parse(shape_id) if node.get("type") == "apply": - traits = _expect_traits(node.get("traits", {}), parsed_id) applies.setdefault(parsed_id.without_member(), []).append( - (parsed_id, traits) + _parse_apply(parsed_id, node) ) continue shapes.append(_parse_shape(parsed_id, node)) @@ -262,7 +295,7 @@ def from_dict(cls, document: Mapping[str, object]) -> Self: # traits added to inherited members arrive as apply statements. Both are # resolved together so that shapes using a mixin see its applied traits. shapes = _resolve_shapes(shapes, applies) - return cls(smithy=version, metadata=_mapping(metadata), shapes=tuple(shapes)) + return cls(smithy=version, metadata=metadata, shapes=tuple(shapes)) def __iter__(self) -> Iterator[Shape]: return iter(self.shapes) @@ -271,16 +304,16 @@ def __len__(self) -> int: return len(self.shapes) def get(self, shape_id: ShapeID | str) -> Shape | None: - """Return a shape by ID, resolving prelude shapes even when omitted.""" + """Return a shape by ID, resolving prelude shapes even when omitted. + + A member ID resolves to the shape containing it, or ``None`` when that + shape does not define the member. + """ shape_id = ShapeID.parse(shape_id) if isinstance(shape_id, str) else shape_id - if shape_id.member is not None: - return self._index.get(shape_id.without_member()) - if (shape := self._index.get(shape_id)) is not None: + shape = self._index.get(shape_id.without_member()) + if shape is None or shape_id.member is None: return shape - if shape_id.is_prelude and shape_id.name in _PRELUDE_TYPES: - shape_type, traits = _PRELUDE_TYPES[shape_id.name] - return Shape(id=shape_id, type=shape_type, traits=_mapping(traits)) - return None + return shape if shape.get_member(shape_id.member) is not None else None def expect(self, shape_id: ShapeID | str) -> Shape: """Return a shape by ID or raise :class:`ModelError` if it is absent.""" @@ -294,9 +327,7 @@ def services(self) -> tuple[Shape, ...]: def replace_shapes(self, shapes: Iterable[Shape]) -> Self: """Return a copy of the model with a different set of shapes.""" - return type(self)( - smithy=self.smithy, metadata=self.metadata, shapes=tuple(shapes) - ) + return replace(self, shapes=tuple(shapes)) def _parse_shape(shape_id: ShapeID, node: Mapping[str, object]) -> Shape: @@ -307,25 +338,18 @@ def _parse_shape(shape_id: ShapeID, node: Mapping[str, object]) -> Shape: raise ModelError( f"Unsupported shape type {type_value!r} on {shape_id}" ) from error - traits = _expect_traits(node.get("traits", {}), shape_id) - mixins = tuple( - _reference(value, f"{shape_id}.mixins") - for value in _expect_list(node.get("mixins", []), f"{shape_id}.mixins") - ) + traits = _json_object(node.get("traits", {}), f"traits on {shape_id}") + mixins = _reference_list(node.get("mixins", []), f"{shape_id}.mixins") members: list[Member] = [] consumed = {"type", "traits", "mixins"} - if shape_type is ShapeType.LIST: - members.append(_parse_member("member", node.get("member"), shape_id)) - consumed.add("member") - elif shape_type is ShapeType.MAP: - members.extend( - ( - _parse_member("key", node.get("key"), shape_id), - _parse_member("value", node.get("value"), shape_id), - ) - ) - consumed.update(("key", "value")) + if shape_type is ShapeType.LIST or shape_type is ShapeType.MAP: + names = ("member",) if shape_type is ShapeType.LIST else ("key", "value") + consumed.update(names) + for name in names: + # A shape using mixins is serialized without the members it inherits. + if name in node or not mixins: + members.append(_parse_member(name, node.get(name), shape_id)) elif shape_type in { ShapeType.STRUCTURE, ShapeType.UNION, @@ -352,23 +376,20 @@ def _parse_shape(shape_id: ShapeID, node: Mapping[str, object]) -> Shape: traits=traits, mixins=mixins, members=tuple(members), - attributes=_mapping(attributes), + attributes=_freeze(attributes), ) def _parse_member(name: str, unparsed_node: object, container: ShapeID) -> Member: - node = _object_mapping(unparsed_node, f"member {container}${name}") + location = f"{container}${name}" + node = _object_mapping(unparsed_node, f"member {location}") return Member( name=name, - target=_target(node.get("target"), f"{container}${name}"), - traits=_expect_traits(node.get("traits", {}), container.with_member(name)), + target=_target(node.get("target"), location), + traits=_json_object(node.get("traits", {}), f"traits on {location}"), ) -def _expect_traits(value: object, target: ShapeID) -> Mapping[str, JSONValue]: - return _mapping(_json_object(value, f"traits on {target}")) - - def _expect_list(value: object, location: str) -> Sequence[object]: if not isinstance(value, list | tuple): raise ModelError(f"Expected a list at {location}") @@ -389,28 +410,65 @@ def _target(value: object, location: str) -> ShapeID: return ShapeID.parse(value) +def _reference_one(value: object, location: str) -> tuple[ShapeID, ...]: + return (_reference(value, location),) + + def _reference_list(value: object, location: str) -> tuple[ShapeID, ...]: - if value is None: - return () return tuple(_reference(item, location) for item in _expect_list(value, location)) -def _object_mapping(value: object, location: str) -> dict[str, object]: +def _reference_map(value: object, location: str) -> tuple[ShapeID, ...]: + return tuple( + _reference(item, f"{location}.{name}") + for name, item in _object_mapping(value, location).items() + ) + + +type _ReferenceExtractor = Callable[[object, str], tuple[ShapeID, ...]] + +# The shape attributes that hold structural references, and how each spells +# them. Every other attribute holds plain data. Iteration order fixes the order +# references are reported in. +_REFERENCE_ATTRIBUTES: Final[Mapping[str, _ReferenceExtractor]] = MappingProxyType( + { + "operations": _reference_list, + "resources": _reference_list, + "errors": _reference_list, + "collectionOperations": _reference_list, + "input": _reference_one, + "output": _reference_one, + "create": _reference_one, + "put": _reference_one, + "read": _reference_one, + "update": _reference_one, + "delete": _reference_one, + "list": _reference_one, + "identifiers": _reference_map, + "properties": _reference_map, + } +) + + +def _object_items(value: object, location: str) -> Iterator[tuple[str, object]]: + """Validate that a decoded JSON value is an object keyed by strings.""" if not isinstance(value, Mapping): raise ModelError(f"Expected an object at {location}") - result: dict[str, object] = {} for key, item in cast(Mapping[object, object], value).items(): if not isinstance(key, str): raise ModelError(f"Expected string object keys at {location}") - result[key] = item - return result + yield (key, item) + + +def _object_mapping(value: object, location: str) -> dict[str, object]: + return dict(_object_items(value, location)) def _json_object(value: object, location: str) -> Mapping[str, JSONValue]: - return MappingProxyType( + return _freeze( { key: _json_value(item, f"{location}.{key}") - for key, item in _object_mapping(value, location).items() + for key, item in _object_items(value, location) } ) @@ -428,7 +486,18 @@ def _json_value(value: object, location: str) -> JSONValue: raise ModelError(f"Unsupported JSON value at {location}: {type(value).__name__}") -type _Apply = tuple[ShapeID, Mapping[str, JSONValue]] +type _Apply = tuple[str, Mapping[str, JSONValue]] + + +def _parse_apply(target: ShapeID, node: Mapping[str, object]) -> _Apply: + """Parse an apply statement, which adds traits to a member of a shape. + + A shape's own traits are serialized with its definition, so an apply keyed by + a shape ID would need the key that definition already occupies. + """ + if target.member is None: + raise ModelError(f"Expected an apply statement to target a member: {target}") + return (target.member, _json_object(node.get("traits", {}), f"traits on {target}")) def _resolve_shapes( @@ -440,9 +509,9 @@ def _resolve_shapes( members precede local members in a depth-first traversal of the mixins, later mixins take precedence over earlier ones, local definitions take precedence over anything inherited, and the ``mixin`` trait itself and any - ``localTraits`` are not inherited. Apply statements targeting a shape are - merged as part of resolving that shape, so shapes that use it as a mixin - inherit the applied traits. + ``localTraits`` are not inherited. Apply statements targeting the members of + a shape are merged as part of resolving that shape, so shapes that use it as + a mixin inherit the applied traits. """ by_id = {shape.id: shape for shape in shapes} for container in applies: @@ -485,6 +554,11 @@ def _merge_mixins( f"{shape.id} uses {mixin_id} as a mixin, but it lacks the " f"{MIXIN_TRAIT} trait" ) + if mixin.type is not shape.type: + raise ModelError( + f"{shape.id} is a {shape.type} but uses the {mixin.type} shape " + f"{mixin_id} as a mixin" + ) mixin = resolve(mixin) traits.update(_inherited_traits(mixin)) for member in mixin.members: @@ -498,36 +572,33 @@ def _merge_mixins( return replace( shape, - traits=_mapping(traits), + traits=_freeze(traits), members=tuple(members.values()), - attributes=_mapping(attributes), + attributes=_freeze(attributes), ) def _apply_traits(shape: Shape, applies: list[_Apply]) -> Shape: - """Merge apply statements targeting a shape or its members.""" + """Merge apply statements onto the members of a shape.""" members = {member.name: member for member in shape.members} - traits = dict(shape.traits) - for target, applied in applies: - if target.member is None: - traits.update(applied) - continue - member = members.get(target.member) + for name, applied in applies: + member = members.get(name) if member is None: - raise ModelError(f"Apply target member not found: {target}") - members[target.member] = replace( - member, traits=_mapping({**member.traits, **applied}) - ) - return replace(shape, traits=_mapping(traits), members=tuple(members.values())) + raise ModelError( + f"Apply target member not found: {shape.id.with_member(name)}" + ) + members[name] = replace(member, traits=_merged(member.traits, applied)) + return replace(shape, members=tuple(members.values())) def _inherited_traits(mixin: Shape) -> dict[str, JSONValue]: - excluded = {MIXIN_TRAIT} mixin_trait = mixin.trait(MIXIN_TRAIT) - if isinstance(mixin_trait, Mapping): - local_traits = mixin_trait.get("localTraits", ()) - if isinstance(local_traits, tuple): - excluded.update(name for name in local_traits if isinstance(name, str)) + local_traits = ( + mixin_trait.get("localTraits", ()) if isinstance(mixin_trait, Mapping) else () + ) + excluded = {MIXIN_TRAIT} + if isinstance(local_traits, tuple): + excluded.update(name for name in local_traits if isinstance(name, str)) return {name: value for name, value in mixin.traits.items() if name not in excluded} @@ -540,7 +611,7 @@ def _merge_members(inherited: Member | None, member: Member) -> Member: f"Member {member.name} redefines an inherited member with a different " f"target: {inherited.target} != {member.target}" ) - return replace(member, traits=_mapping({**inherited.traits, **member.traits})) + return replace(member, traits=_merged(inherited.traits, member.traits)) def _merge_attributes( @@ -559,6 +630,6 @@ def _merge_attributes( *(item for item in value if item not in existing), ) elif isinstance(existing, Mapping) and isinstance(value, Mapping): - target[key] = MappingProxyType({**existing, **value}) + target[key] = _merged(existing, value) else: target[key] = value diff --git a/packages/smithy-python/src/smithy_python/selection.py b/packages/smithy-python/src/smithy_python/selection.py index 397aa1a49..9ee9ea98c 100644 --- a/packages/smithy-python/src/smithy_python/selection.py +++ b/packages/smithy-python/src/smithy_python/selection.py @@ -9,9 +9,7 @@ from typing import Final from .exceptions import CodegenError, InvalidInvocationError -from .model import MIXIN_TRAIT, Model, Shape, ShapeID, ShapeType - -TRAIT_DEFINITION: Final = "smithy.api#trait" +from .model import MIXIN_TRAIT, TRAIT_DEFINITION, Model, Shape, ShapeID, ShapeType # Shapes carrying these traits describe the model rather than data and are # never generated, even when the JSON AST includes them. @@ -96,7 +94,7 @@ def select_generated_shapes(model: Model, service: Shape | None) -> Selection: def _is_candidate(shape: Shape) -> bool: if shape.type.is_service_category or shape.id.is_prelude: return False - return not any(shape.has_trait(trait) for trait in _EXCLUDED_TRAITS) + return shape.traits.keys().isdisjoint(_EXCLUDED_TRAITS) def _closure(model: Model, service: Shape) -> set[ShapeID]: diff --git a/packages/smithy-python/tests/unit/conftest.py b/packages/smithy-python/tests/unit/conftest.py index 1cd8a3a71..9e4182003 100644 --- a/packages/smithy-python/tests/unit/conftest.py +++ b/packages/smithy-python/tests/unit/conftest.py @@ -4,9 +4,12 @@ from __future__ import annotations import json -from typing import Any +from collections.abc import Mapping +from io import BytesIO +from typing import Any, BinaryIO, Protocol import pytest +from smithy_python.cli import main from smithy_python.model import Model @@ -104,3 +107,30 @@ def model(model_document: dict[str, Any]) -> Model: @pytest.fixture def model_json(model_document: dict[str, Any]) -> bytes: return json.dumps(model_document).encode() + + +class CliRunner(Protocol): + """Runs the CLI and reports its exit code with what it wrote to stderr.""" + + def __call__( + self, + *argv: str, + environ: Mapping[str, str] | None = None, + stdin: bytes | BinaryIO | None = None, + ) -> tuple[int, str]: ... + + +@pytest.fixture +def run_cli(capsys: pytest.CaptureFixture[str]) -> CliRunner: + """Run the CLI with an empty environment unless one is given.""" + + def run( + *argv: str, + environ: Mapping[str, str] | None = None, + stdin: bytes | BinaryIO | None = None, + ) -> tuple[int, str]: + stream = BytesIO(stdin) if isinstance(stdin, bytes) else stdin + exit_code = main(argv, environ={} if environ is None else environ, stdin=stream) + return exit_code, capsys.readouterr().err + + return run diff --git a/packages/smithy-python/tests/unit/test_cli.py b/packages/smithy-python/tests/unit/test_cli.py index 3522f1a74..2a9fec320 100644 --- a/packages/smithy-python/tests/unit/test_cli.py +++ b/packages/smithy-python/tests/unit/test_cli.py @@ -14,6 +14,8 @@ from smithy_python import __version__ from smithy_python.cli import main +from .conftest import CliRunner + class _InteractiveStdin(BytesIO): def isatty(self) -> bool: @@ -36,29 +38,22 @@ def test_information_commands( @pytest.mark.parametrize("artifact", ["client", "types"]) def test_generation_commands_are_explicitly_unavailable( - artifact: str, - model_json: bytes, - tmp_path: Path, - capsys: pytest.CaptureFixture[str], + artifact: str, model_json: bytes, tmp_path: Path, run_cli: CliRunner ) -> None: model = tmp_path / "model.json" model.write_bytes(model_json) - assert ( - main( - ( - "generate", - artifact, - "--model", - str(model), - "--output", - str(tmp_path / "output"), - ), - environ={}, - ) - == 1 + exit_code, stderr = run_cli( + "generate", + artifact, + "--model", + str(model), + "--output", + str(tmp_path / "output"), ) - assert capsys.readouterr().err.endswith( + + assert exit_code == 1 + assert stderr.endswith( f"smithy-python: error: {artifact} generation is not implemented yet\n" ) @@ -71,12 +66,12 @@ def test_generation_commands_are_explicitly_unavailable( ], ) def test_missing_command_identifies_available_subcommands( - argv: tuple[str, ...], - expected_usage: str, - capsys: pytest.CaptureFixture[str], + argv: tuple[str, ...], expected_usage: str, run_cli: CliRunner ) -> None: - assert main(argv) == 2 - assert capsys.readouterr().err.startswith(expected_usage) + exit_code, stderr = run_cli(*argv) + + assert exit_code == 2 + assert stderr.startswith(expected_usage) def test_main_module_invokes_cli() -> None: @@ -93,125 +88,95 @@ def test_main_module_invokes_cli() -> None: def test_run_plugin_invocation_reads_standard_input( - model_json: bytes, tmp_path: Path, capsys: pytest.CaptureFixture[str] + model_json: bytes, tmp_path: Path, run_cli: CliRunner ) -> None: - assert ( - main( - ("generate", "client"), - environ={"SMITHY_PLUGIN_DIR": str(tmp_path)}, - stdin=BytesIO(model_json), - ) - == 1 + exit_code, stderr = run_cli( + "generate", + "client", + environ={"SMITHY_PLUGIN_DIR": str(tmp_path)}, + stdin=model_json, ) - assert "generation is not implemented yet" in capsys.readouterr().err + + assert exit_code == 1 + assert "generation is not implemented yet" in stderr @pytest.mark.parametrize("option", ["--model", "--output"]) def test_run_plugin_rejects_direct_invocation_options( - option: str, - tmp_path: Path, - capsys: pytest.CaptureFixture[str], + option: str, tmp_path: Path, run_cli: CliRunner ) -> None: - assert ( - main( - ("generate", "client", option, str(tmp_path / "value")), - environ={"SMITHY_PLUGIN_DIR": str(tmp_path)}, - stdin=BytesIO(b"{}"), - ) - == 2 - ) - assert f"{option} cannot be used with the Smithy run plugin" in ( - capsys.readouterr().err + exit_code, stderr = run_cli( + "generate", + "client", + option, + str(tmp_path / "value"), + environ={"SMITHY_PLUGIN_DIR": str(tmp_path)}, + stdin=b"{}", ) + assert exit_code == 2 + assert f"{option} cannot be used with the Smithy run plugin" in stderr -def test_direct_invocation_requires_output( - tmp_path: Path, capsys: pytest.CaptureFixture[str] -) -> None: + +def test_direct_invocation_requires_output(tmp_path: Path, run_cli: CliRunner) -> None: model = tmp_path / "model.json" model.write_text("{}") - assert main(("generate", "client", "--model", str(model)), environ={}) == 2 - assert "Direct invocation requires --output" in capsys.readouterr().err + exit_code, stderr = run_cli("generate", "client", "--model", str(model)) + assert exit_code == 2 + assert "Direct invocation requires --output" in stderr -def test_invocation_rejects_empty_model( - tmp_path: Path, capsys: pytest.CaptureFixture[str] -) -> None: - assert ( - main( - ("generate", "client", "--output", str(tmp_path)), - environ={}, - stdin=BytesIO(), - ) - == 2 + +def test_invocation_rejects_empty_model(tmp_path: Path, run_cli: CliRunner) -> None: + exit_code, stderr = run_cli( + "generate", "client", "--output", str(tmp_path), stdin=b"" ) - assert "Expected a Smithy JSON AST model" in capsys.readouterr().err + + assert exit_code == 2 + assert "Expected a Smithy JSON AST model" in stderr def test_direct_invocation_rejects_interactive_model_input( - tmp_path: Path, capsys: pytest.CaptureFixture[str] + tmp_path: Path, run_cli: CliRunner ) -> None: - assert ( - main( - ("generate", "client", "--output", str(tmp_path)), - environ={}, - stdin=_InteractiveStdin(), - ) - == 2 + exit_code, stderr = run_cli( + "generate", "client", "--output", str(tmp_path), stdin=_InteractiveStdin() ) + + assert exit_code == 2 assert ( "Direct invocation requires --model or a model piped to standard input" - in capsys.readouterr().err + in stderr ) def test_invocation_reports_unreadable_model( - tmp_path: Path, capsys: pytest.CaptureFixture[str] + tmp_path: Path, run_cli: CliRunner ) -> None: missing = tmp_path / "missing.json" - assert ( - main( - ( - "generate", - "client", - "--model", - str(missing), - "--output", - str(tmp_path), - ), - environ={}, - ) - == 2 + exit_code, stderr = run_cli( + "generate", "client", "--model", str(missing), "--output", str(tmp_path) ) - assert f"Model path is not a file: {missing}" in capsys.readouterr().err + + assert exit_code == 2 + assert f"Model path is not a file: {missing}" in stderr def test_invocation_rejects_empty_model_path( - tmp_path: Path, capsys: pytest.CaptureFixture[str] + tmp_path: Path, run_cli: CliRunner ) -> None: - assert ( - main( - ( - "generate", - "client", - "--model", - "", - "--output", - str(tmp_path), - ), - environ={}, - ) - == 2 + exit_code, stderr = run_cli( + "generate", "client", "--model", "", "--output", str(tmp_path) ) - assert "Model path is not a file: ." in capsys.readouterr().err + + assert exit_code == 2 + assert "Model path is not a file: ." in stderr def test_invocation_reports_model_io_error( - monkeypatch: pytest.MonkeyPatch, - tmp_path: Path, - capsys: pytest.CaptureFixture[str], + monkeypatch: pytest.MonkeyPatch, tmp_path: Path, run_cli: CliRunner ) -> None: model = tmp_path / "model.json" model.write_text("{}") @@ -221,21 +186,12 @@ def raise_io_error(self: Path) -> bytes: monkeypatch.setattr(Path, "read_bytes", raise_io_error) - assert ( - main( - ( - "generate", - "client", - "--model", - str(model), - "--output", - str(tmp_path), - ), - environ={}, - ) - == 1 + exit_code, stderr = run_cli( + "generate", "client", "--model", str(model), "--output", str(tmp_path) ) - assert "unable to read model" in capsys.readouterr().err + + assert exit_code == 1 + assert "unable to read model" in stderr def test_help_documents_service_option(capsys: pytest.CaptureFixture[str]) -> None: @@ -244,51 +200,36 @@ def test_help_documents_service_option(capsys: pytest.CaptureFixture[str]) -> No def test_invalid_model_is_a_generation_failure( - tmp_path: Path, capsys: pytest.CaptureFixture[str] + tmp_path: Path, run_cli: CliRunner ) -> None: - assert ( - main( - ("generate", "types", "--output", str(tmp_path)), - environ={}, - stdin=BytesIO(b"{}"), - ) - == 1 + exit_code, stderr = run_cli( + "generate", "types", "--output", str(tmp_path), stdin=b"{}" ) - assert "missing a string 'smithy' version" in capsys.readouterr().err + assert exit_code == 1 + assert "missing a string 'smithy' version" in stderr -def test_client_requires_a_service( - tmp_path: Path, capsys: pytest.CaptureFixture[str] -) -> None: - assert ( - main( - ("generate", "client", "--output", str(tmp_path)), - environ={}, - stdin=BytesIO(b'{"smithy": "2.0"}'), - ) - == 2 + +def test_client_requires_a_service(tmp_path: Path, run_cli: CliRunner) -> None: + exit_code, stderr = run_cli( + "generate", "client", "--output", str(tmp_path), stdin=b'{"smithy": "2.0"}' ) - assert "does not contain a service" in capsys.readouterr().err + assert exit_code == 2 + assert "does not contain a service" in stderr -def test_types_does_not_require_a_service( - tmp_path: Path, capsys: pytest.CaptureFixture[str] -) -> None: - assert ( - main( - ("generate", "types", "--output", str(tmp_path)), - environ={}, - stdin=BytesIO(b'{"smithy": "2.0"}'), - ) - == 1 + +def test_types_does_not_require_a_service(tmp_path: Path, run_cli: CliRunner) -> None: + exit_code, stderr = run_cli( + "generate", "types", "--output", str(tmp_path), stdin=b'{"smithy": "2.0"}' ) - assert "types generation is not implemented yet" in capsys.readouterr().err + + assert exit_code == 1 + assert "types generation is not implemented yet" in stderr def test_multiple_services_require_service_option( - model_document: dict[str, Any], - tmp_path: Path, - capsys: pytest.CaptureFixture[str], + model_document: dict[str, Any], tmp_path: Path, run_cli: CliRunner ) -> None: model_document["shapes"]["example.other#Other"] = { "type": "service", @@ -296,32 +237,23 @@ def test_multiple_services_require_service_option( } source = json.dumps(model_document).encode() - assert ( - main( - ("generate", "client", "--output", str(tmp_path)), - environ={}, - stdin=BytesIO(source), - ) - == 2 + exit_code, stderr = run_cli( + "generate", "client", "--output", str(tmp_path), stdin=source ) - assert "select one with --service" in capsys.readouterr().err - - assert ( - main( - ( - "generate", - "client", - "--output", - str(tmp_path), - "--service", - "example.weather#Weather", - ), - environ={}, - stdin=BytesIO(source), - ) - == 1 + assert exit_code == 2 + assert "select one with --service" in stderr + + exit_code, stderr = run_cli( + "generate", + "client", + "--output", + str(tmp_path), + "--service", + "example.weather#Weather", + stdin=source, ) - assert "client generation is not implemented yet" in capsys.readouterr().err + assert exit_code == 1 + assert "client generation is not implemented yet" in stderr @pytest.mark.parametrize( @@ -338,52 +270,49 @@ def test_invalid_service_option_is_an_invocation_error( message: str, model_json: bytes, tmp_path: Path, - capsys: pytest.CaptureFixture[str], + run_cli: CliRunner, ) -> None: - assert ( - main( - ("generate", "client", "--output", str(tmp_path), "--service", value), - environ={}, - stdin=BytesIO(model_json), - ) - == 2 + exit_code, stderr = run_cli( + "generate", + "client", + "--output", + str(tmp_path), + "--service", + value, + stdin=model_json, ) - assert message in capsys.readouterr().err + + assert exit_code == 2 + assert message in stderr def test_unconnected_shapes_are_reported( - model_json: bytes, - tmp_path: Path, - capsys: pytest.CaptureFixture[str], + model_json: bytes, tmp_path: Path, run_cli: CliRunner ) -> None: - assert ( - main( - ("generate", "client", "--output", str(tmp_path)), - environ={}, - stdin=BytesIO(model_json), - ) - == 1 + exit_code, stderr = run_cli( + "generate", "client", "--output", str(tmp_path), stdin=model_json ) + + assert exit_code == 1 assert ( "note: 1 shape(s) not connected to example.weather#Weather will not be " "generated" - ) in capsys.readouterr().err + ) in stderr def test_shape_name_conflicts_are_a_generation_failure( - model_document: dict[str, Any], - tmp_path: Path, - capsys: pytest.CaptureFixture[str], + model_document: dict[str, Any], tmp_path: Path, run_cli: CliRunner ) -> None: del model_document["shapes"]["example.weather#Weather"] model_document["shapes"]["example.other#coordinates"] = {"type": "string"} - assert ( - main( - ("generate", "types", "--output", str(tmp_path)), - environ={}, - stdin=BytesIO(json.dumps(model_document).encode()), - ) - == 1 + exit_code, stderr = run_cli( + "generate", + "types", + "--output", + str(tmp_path), + stdin=json.dumps(model_document).encode(), ) - assert "case-insensitively unique" in capsys.readouterr().err + + assert exit_code == 1 + assert "case-insensitively unique" in stderr diff --git a/packages/smithy-python/tests/unit/test_model.py b/packages/smithy-python/tests/unit/test_model.py index 82ae9e9c5..bb9e7f68e 100644 --- a/packages/smithy-python/tests/unit/test_model.py +++ b/packages/smithy-python/tests/unit/test_model.py @@ -37,9 +37,25 @@ def test_member_helpers(self) -> None: member = shape.with_member("bar") assert member.member == "bar" assert member.without_member() == shape + assert shape.without_member() is shape assert ShapeID.parse("smithy.api#String").is_prelude assert not shape.is_prelude + def test_shape_and_member_ids_sort_together(self) -> None: + ids = [ + ShapeID.parse("b#A"), + ShapeID.parse("a#B$c"), + ShapeID.parse("a#B"), + ShapeID.parse("a#A"), + ] + assert [str(shape_id) for shape_id in sorted(ids)] == [ + "a#A", + "a#B", + "a#B$c", + "b#A", + ] + assert ShapeID.parse("a#B") >= ShapeID.parse("a#A$c") + class TestParsing: def test_preserves_shape_member_and_trait_order(self, model: Model) -> None: @@ -136,6 +152,16 @@ def test_references_report_malformed_targets( "example.weather#GetCity" ).references() + def test_references_report_malformed_identifiers( + self, model_document: dict[str, Any] + ) -> None: + model_document["shapes"]["example.weather#City"] = { + "type": "resource", + "identifiers": [{"target": "example.weather#CityId"}], + } + with pytest.raises(ModelError, match="Expected an object at"): + Model.from_dict(model_document).expect("example.weather#City").references() + def test_apply_merges_traits_onto_members( self, model_document: dict[str, Any] ) -> None: @@ -162,13 +188,21 @@ def test_apply_to_missing_member_is_an_error( def test_apply_to_missing_target_is_an_error( self, model_document: dict[str, Any] ) -> None: - model_document["shapes"]["example.weather#Missing"] = { + model_document["shapes"]["example.weather#Missing$foo"] = { "type": "apply", "traits": {}, } with pytest.raises(ModelError, match="Apply target not found"): Model.from_dict(model_document) + def test_apply_must_target_a_member(self, model_document: dict[str, Any]) -> None: + model_document["shapes"]["example.weather#Other"] = { + "type": "apply", + "traits": {"smithy.api#documentation": "docs"}, + } + with pytest.raises(ModelError, match="apply statement to target a member"): + Model.from_dict(model_document) + def test_shapes_key_is_optional(self) -> None: assert len(Model.from_dict({"smithy": "2.0"})) == 0 @@ -202,6 +236,22 @@ def test_shapes_key_is_optional(self) -> None: }, "Expected a list", ), + ( + {"smithy": "2.0", "shapes": {1: {"type": "string"}}}, + "Expected string object keys", + ), + ( + { + "smithy": "2.0", + "shapes": { + "example#Bad": { + "type": "string", + "traits": {"example#trait": object()}, + } + }, + }, + "Unsupported JSON value", + ), ], ) def test_invalid_documents_are_reported( @@ -246,9 +296,21 @@ def test_get_returns_none_for_unknown_shapes(self, model: Model) -> None: assert model.get("example.weather#Nope") is None assert model.get("smithy.api#Nope") is None + def test_prelude_shapes_are_shared_between_lookups(self, model: Model) -> None: + assert model.expect("smithy.api#String") is model.expect("smithy.api#String") + def test_member_id_resolves_to_container(self, model: Model) -> None: shape = model.expect("example.weather#Coordinates$latitude") assert shape.id == ShapeID.parse("example.weather#Coordinates") + assert model.expect("example.weather#Tags$member").type is ShapeType.LIST + + def test_member_id_of_an_undefined_member_does_not_resolve( + self, model: Model + ) -> None: + assert model.get("example.weather#Coordinates$altitude") is None + assert model.get("example.weather#Nope$latitude") is None + # The prelude resolves, but its shapes declare no members. + assert model.get("smithy.api#Unit$value") is None def test_expect_reports_missing_shapes(self, model: Model) -> None: with pytest.raises(ModelError, match="Shape not found"): @@ -489,6 +551,45 @@ def test_redefined_members_merge_traits_and_keep_position(self) -> None: assert a.has_trait("smithy.api#required") assert a.has_trait("smithy.api#documentation") + def test_list_and_map_members_are_inherited(self) -> None: + # Smithy omits member, key, and value from a shape that inherits them. + model = Model.from_dict( + self._document( + { + "example#AbstractList": { + "type": "list", + "traits": {"smithy.api#mixin": {}}, + "member": {"target": "smithy.api#String"}, + }, + "example#Names": { + "type": "list", + "mixins": [{"target": "example#AbstractList"}], + }, + "example#AbstractMap": { + "type": "map", + "traits": {"smithy.api#mixin": {}}, + "key": {"target": "smithy.api#String"}, + "value": {"target": "smithy.api#Integer"}, + }, + "example#Counts": { + "type": "map", + "mixins": [{"target": "example#AbstractMap"}], + }, + "example#Names$member": { + "type": "apply", + "traits": {"smithy.api#documentation": "A name"}, + }, + } + ) + ) + names = model.expect("example#Names") + assert names.member("member").target == ShapeID.parse("smithy.api#String") + assert names.member("member").trait("smithy.api#documentation") == "A name" + + counts = model.expect("example#Counts") + assert [member.name for member in counts.members] == ["key", "value"] + assert counts.member("value").target == ShapeID.parse("smithy.api#Integer") + def test_redefined_members_must_keep_their_target(self) -> None: with pytest.raises(ModelError, match="different target"): Model.from_dict( @@ -604,6 +705,20 @@ def test_operation_errors_are_inherited(self) -> None: }, "lacks the smithy.api#mixin trait", ), + ( + { + "example#M": { + "type": "list", + "traits": {"smithy.api#mixin": {}}, + "member": {"target": "smithy.api#String"}, + }, + "example#S": { + "type": "structure", + "mixins": [{"target": "example#M"}], + }, + }, + "is a structure but uses the list shape", + ), ( { "example#A": { From a1c80ebbcf6828168d52fee8677250814d4c07db Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sun, 13 Sep 2026 17:43:58 -0400 Subject: [PATCH 17/19] fix(codegen): Resolve member IDs to members, not containers Model.get and Model.expect returned the containing shape when given a member ID, so a caller resolving `ns#Shape$member` received a Shape whose type and traits belonged to the container rather than the member. Reject member IDs in the shape lookups and add Model.get_member and Model.expect_member, which return the Member itself and reject shape IDs symmetrically. --- .../smithy-python/src/smithy_python/model.py | 33 +++++++++++++++---- .../smithy-python/tests/unit/test_model.py | 27 +++++++++++---- 2 files changed, 46 insertions(+), 14 deletions(-) diff --git a/packages/smithy-python/src/smithy_python/model.py b/packages/smithy-python/src/smithy_python/model.py index 3f19e986f..70035acfc 100644 --- a/packages/smithy-python/src/smithy_python/model.py +++ b/packages/smithy-python/src/smithy_python/model.py @@ -306,14 +306,14 @@ def __len__(self) -> int: def get(self, shape_id: ShapeID | str) -> Shape | None: """Return a shape by ID, resolving prelude shapes even when omitted. - A member ID resolves to the shape containing it, or ``None`` when that - shape does not define the member. + Member IDs are rejected: a member is not a shape, so returning either the + member's container or its target would give callers the wrong traits and + type. Use :meth:`get_member` to resolve a member ID. """ - shape_id = ShapeID.parse(shape_id) if isinstance(shape_id, str) else shape_id - shape = self._index.get(shape_id.without_member()) - if shape is None or shape_id.member is None: - return shape - return shape if shape.get_member(shape_id.member) is not None else None + shape_id = _shape_id(shape_id) + if shape_id.member is not None: + raise ModelError(f"Expected a shape ID, found a member ID: {shape_id}") + return self._index.get(shape_id) def expect(self, shape_id: ShapeID | str) -> Shape: """Return a shape by ID or raise :class:`ModelError` if it is absent.""" @@ -321,6 +321,21 @@ def expect(self, shape_id: ShapeID | str) -> Shape: raise ModelError(f"Shape not found: {shape_id}") return shape + def get_member(self, member_id: ShapeID | str) -> Member | None: + """Return the member a member ID identifies, or ``None`` if it is absent.""" + member_id = _shape_id(member_id) + if member_id.member is None: + raise ModelError(f"Expected a member ID, found a shape ID: {member_id}") + if (shape := self._index.get(member_id.without_member())) is None: + return None + return shape.get_member(member_id.member) + + def expect_member(self, member_id: ShapeID | str) -> Member: + """Return a member by ID or raise :class:`ModelError` if it is absent.""" + if (member := self.get_member(member_id)) is None: + raise ModelError(f"Member not found: {member_id}") + return member + def services(self) -> tuple[Shape, ...]: """Return every service shape in modeled order.""" return tuple(shape for shape in self if shape.type is ShapeType.SERVICE) @@ -330,6 +345,10 @@ def replace_shapes(self, shapes: Iterable[Shape]) -> Self: return replace(self, shapes=tuple(shapes)) +def _shape_id(value: ShapeID | str) -> ShapeID: + return ShapeID.parse(value) if isinstance(value, str) else value + + def _parse_shape(shape_id: ShapeID, node: Mapping[str, object]) -> Shape: type_value = node.get("type") try: diff --git a/packages/smithy-python/tests/unit/test_model.py b/packages/smithy-python/tests/unit/test_model.py index bb9e7f68e..1d4f9e77d 100644 --- a/packages/smithy-python/tests/unit/test_model.py +++ b/packages/smithy-python/tests/unit/test_model.py @@ -299,24 +299,37 @@ def test_get_returns_none_for_unknown_shapes(self, model: Model) -> None: def test_prelude_shapes_are_shared_between_lookups(self, model: Model) -> None: assert model.expect("smithy.api#String") is model.expect("smithy.api#String") - def test_member_id_resolves_to_container(self, model: Model) -> None: - shape = model.expect("example.weather#Coordinates$latitude") - assert shape.id == ShapeID.parse("example.weather#Coordinates") - assert model.expect("example.weather#Tags$member").type is ShapeType.LIST + def test_member_ids_resolve_to_members(self, model: Model) -> None: + latitude = model.expect_member("example.weather#Coordinates$latitude") + assert latitude.name == "latitude" + assert latitude.target == ShapeID.parse("smithy.api#Float") + assert model.expect_member(ShapeID.parse("example.weather#Tags$member")) + + def test_shape_lookups_reject_member_ids(self, model: Model) -> None: + with pytest.raises(ModelError, match="found a member ID"): + model.get("example.weather#Coordinates$latitude") + with pytest.raises(ModelError, match="found a member ID"): + model.expect("example.weather#Coordinates$latitude") + + def test_member_lookups_reject_shape_ids(self, model: Model) -> None: + with pytest.raises(ModelError, match="found a shape ID"): + model.get_member("example.weather#Coordinates") def test_member_id_of_an_undefined_member_does_not_resolve( self, model: Model ) -> None: - assert model.get("example.weather#Coordinates$altitude") is None - assert model.get("example.weather#Nope$latitude") is None + assert model.get_member("example.weather#Coordinates$altitude") is None + assert model.get_member("example.weather#Nope$latitude") is None # The prelude resolves, but its shapes declare no members. - assert model.get("smithy.api#Unit$value") is None + assert model.get_member("smithy.api#Unit$value") is None def test_expect_reports_missing_shapes(self, model: Model) -> None: with pytest.raises(ModelError, match="Shape not found"): model.expect("example.weather#Nope") with pytest.raises(ModelError, match="Member not found"): model.expect("example.weather#Coordinates").member("altitude") + with pytest.raises(ModelError, match="Member not found"): + model.expect_member("example.weather#Coordinates$altitude") def test_services_are_listed_in_model_order(self, model: Model) -> None: assert [shape.id.name for shape in model.services()] == ["Weather"] From 78084df52e898af2774536cd66da475a15122e88 Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sun, 13 Sep 2026 17:44:11 -0400 Subject: [PATCH 18/19] fix(codegen): Reject JSON ASTs that are not Smithy 2.x Any string was accepted as the `smithy` version, so a Smithy 1.0 AST failed later with an unrelated error such as "Unsupported shape type 'set'". Check the major version while loading and report the version along with how to fix it. Document the 2.x requirement in the CLI design. --- designs/codegen/cli.md | 2 ++ packages/smithy-python/src/smithy_python/model.py | 8 ++++++++ packages/smithy-python/tests/unit/test_model.py | 3 +++ 3 files changed, 13 insertions(+) diff --git a/designs/codegen/cli.md b/designs/codegen/cli.md index 5ee56ea49..9e41a1475 100644 --- a/designs/codegen/cli.md +++ b/designs/codegen/cli.md @@ -102,6 +102,8 @@ Smithy process's `PATH`. Smithy passes no arguments other than those in When invoked by Smithy, the CLI reads one JSON AST document from standard input. The document represents the model after projection transforms have been applied. +Only Smithy 2.x JSON ASTs are supported; a document declaring another `smithy` +version is rejected with an error that names the version. Smithy serializes only what a shape introduces, so shapes that use mixins arrive without their inherited members, traits, and properties, and traits added to diff --git a/packages/smithy-python/src/smithy_python/model.py b/packages/smithy-python/src/smithy_python/model.py index 70035acfc..cec523248 100644 --- a/packages/smithy-python/src/smithy_python/model.py +++ b/packages/smithy-python/src/smithy_python/model.py @@ -28,6 +28,8 @@ PRELUDE_NAMESPACE = "smithy.api" MIXIN_TRAIT = "smithy.api#mixin" TRAIT_DEFINITION = "smithy.api#trait" +# Smithy 1.0 ASTs differ in shape types (`set`) and nullability semantics. +SUPPORTED_MAJOR_VERSION = "2" class ShapeType(StrEnum): @@ -276,6 +278,12 @@ def from_dict(cls, document: Mapping[str, object]) -> Self: version = document.get("smithy") if not isinstance(version, str): raise ModelError("The Smithy JSON AST is missing a string 'smithy' version") + if version.partition(".")[0] != SUPPORTED_MAJOR_VERSION: + raise ModelError( + f"Unsupported Smithy version {version!r}: only Smithy " + f"{SUPPORTED_MAJOR_VERSION}.x JSON ASTs are supported. Rebuild the " + f"model with a Smithy {SUPPORTED_MAJOR_VERSION}.x CLI." + ) shapes_node = _object_mapping(document.get("shapes", {}), "Smithy model shapes") metadata = _json_object(document.get("metadata", {}), "Smithy model metadata") diff --git a/packages/smithy-python/tests/unit/test_model.py b/packages/smithy-python/tests/unit/test_model.py index 1d4f9e77d..fd4e3c794 100644 --- a/packages/smithy-python/tests/unit/test_model.py +++ b/packages/smithy-python/tests/unit/test_model.py @@ -211,6 +211,9 @@ def test_shapes_key_is_optional(self) -> None: [ ({}, "missing a string 'smithy' version"), ({"smithy": 2}, "missing a string 'smithy' version"), + ({"smithy": "1.0"}, "Unsupported Smithy version '1.0'"), + ({"smithy": "3.0"}, "Unsupported Smithy version '3.0'"), + ({"smithy": ""}, "Unsupported Smithy version ''"), ({"smithy": "2.0", "shapes": []}, "Expected an object"), ( {"smithy": "2.0", "shapes": {"example#Bad": {"type": "nope"}}}, From 85d815192006d3e4ec674c23ea02584aa8b67dff Mon Sep 17 00:00:00 2001 From: jonathan343 Date: Sun, 13 Sep 2026 18:28:20 -0400 Subject: [PATCH 19/19] fix(codegen): Exit 1 when the model file cannot be read A missing --model path was pre-checked and reported as an invocation error with exit 2, while a file that existed but could not be read raised OSError and exited 1. The design classifies I/O failures as 1, and tools that distinguish usage errors from runtime failures, including the Smithy CLI, treat a missing input file as the latter. Drop the pre-check so every unreadable model exits 1 with a message that names the path and the cause. Also state in the design that model failures return 1, which the implementation already did but the text left implicit. --- designs/codegen/cli.md | 6 ++++-- packages/smithy-python/src/smithy_python/cli.py | 11 ++++++++--- packages/smithy-python/tests/unit/test_cli.py | 13 +++++++------ 3 files changed, 19 insertions(+), 11 deletions(-) diff --git a/designs/codegen/cli.md b/designs/codegen/cli.md index d84f5a8e9..a4322de82 100644 --- a/designs/codegen/cli.md +++ b/designs/codegen/cli.md @@ -61,8 +61,10 @@ added when there is a need for it. The command MUST return zero after successful generation and non-zero when arguments, settings, the model, or generation are invalid. Diagnostics are -written to standard error. Invalid command syntax and invocation inputs return -2, while I/O and generation failures return 1. +written to standard error. Invalid command syntax and invocation inputs, such as +options that cannot be combined or a service that cannot be selected, return 2. +Model, I/O, and generation failures, including a model file that cannot be read, +return 1. ## Smithy `run` Plugin diff --git a/packages/smithy-python/src/smithy_python/cli.py b/packages/smithy-python/src/smithy_python/cli.py index 79cf26a62..05a35b2c8 100644 --- a/packages/smithy-python/src/smithy_python/cli.py +++ b/packages/smithy-python/src/smithy_python/cli.py @@ -132,9 +132,14 @@ def _resolve_invocation( output_dir = output_path if model_path is not None: - if not model_path.is_file(): - raise InvalidInvocationError(f"Model path is not a file: {model_path}") - model_source = model_path.read_bytes() + # A model that cannot be read is an I/O failure like any other, whether + # the path is missing, a directory, or unreadable. + try: + model_source = model_path.read_bytes() + except OSError as error: + raise OSError( + f"Cannot read model {model_path}: {error.strerror or error}" + ) from error else: model_stream = sys.stdin.buffer if stdin is None else stdin if environment.plugin_dir is None and model_stream.isatty(): diff --git a/packages/smithy-python/tests/unit/test_cli.py b/packages/smithy-python/tests/unit/test_cli.py index 74a2418cb..033a7bda8 100644 --- a/packages/smithy-python/tests/unit/test_cli.py +++ b/packages/smithy-python/tests/unit/test_cli.py @@ -163,7 +163,7 @@ def test_direct_invocation_rejects_interactive_model_input( ) -def test_invocation_reports_unreadable_model( +def test_invocation_reports_missing_model( tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: missing = tmp_path / "missing.json" @@ -180,14 +180,15 @@ def test_invocation_reports_unreadable_model( ), environ={}, ) - == 2 + == 1 ) - assert f"Model path is not a file: {missing}" in capsys.readouterr().err + assert f"Cannot read model {missing}: No such file" in capsys.readouterr().err -def test_invocation_rejects_empty_model_path( +def test_invocation_reports_model_path_that_is_a_directory( tmp_path: Path, capsys: pytest.CaptureFixture[str] ) -> None: + # An empty path resolves to the current directory. assert ( main( ( @@ -200,9 +201,9 @@ def test_invocation_rejects_empty_model_path( ), environ={}, ) - == 2 + == 1 ) - assert "Model path is not a file: ." in capsys.readouterr().err + assert "Cannot read model .:" in capsys.readouterr().err def test_invocation_reports_model_io_error(