Fix Python namespace package imports

CHANGELOG.md

@@ -1,5 +1,14 @@
 # Changelog
 
+## Unreleased
+
+Python client generation improvements:
+
+- Generated Python clients with nested namespaces now import cleanly when models refer to parent or sibling namespaces, including cases like `offer_rules.InsurerCategory`.
+- Namespace names containing characters that are not valid in Python identifiers, such as dashes or leading digits, now generate valid Python classes.
+- Schemas with a root namespace named `sys` no longer conflict with Python's standard `sys` module.
+- Re-running `reflectapi codegen` into an existing output directory now removes generated files from older schemas while preserving hand-written files.
+
 ## 0.17.4
 
 Python client codegen fixes:

reflectapi-cli/src/main.rs

@@ -2,7 +2,10 @@ use anyhow::Context;
 use clap::{Parser, Subcommand, ValueEnum};
 use std::collections::BTreeSet;
 use std::io::Write;
-use std::path::PathBuf;
+use std::path::{Path, PathBuf};
+
+const GENERATED_MARKER: &str = "This file was generated by reflectapi-cli";
+const GENERATED_MANIFEST: &str = ".reflectapi-generated-files";
 
 #[derive(Parser)]
 #[command(version, about, long_about = None)]
@@ -252,12 +255,15 @@ fn main() -> anyhow::Result<()> {
                 std::fs::create_dir_all(&output_path).context(format!(
                     "Failed to create output directory: {output_path:?}"
                 ))?;
+                let expected_files = generated_file_set(files.keys())?;
+                cleanup_stale_generated_files(&output_path, &expected_files, &language)?;
                 for (filename, content) in &files {
                     write_file(
                         &output_path.join(generated_relative_path(filename)?),
                         content,
                     )?;
                 }
+                write_generated_manifest(&output_path, &expected_files)?;
             } else {
                 let parent = parent_or_dot(&output_path);
                 std::fs::create_dir_all(&parent)
@@ -276,6 +282,131 @@ fn main() -> anyhow::Result<()> {
     }
 }
 
+fn generated_file_set<'a>(
+    filenames: impl Iterator<Item = &'a String>,
+) -> anyhow::Result<BTreeSet<PathBuf>> {
+    filenames
+        .map(|filename| generated_relative_path(filename).map(Path::to_path_buf))
+        .collect()
+}
+
+fn cleanup_stale_generated_files(
+    output_dir: &Path,
+    expected_files: &BTreeSet<PathBuf>,
+    language: &Language,
+) -> anyhow::Result<()> {
+    let manifest_path = output_dir.join(GENERATED_MANIFEST);
+    let stale_candidates = if manifest_path.is_file() {
+        read_generated_manifest(&manifest_path)?
+    } else {
+        let mut candidates = BTreeSet::new();
+        collect_legacy_generated_files(output_dir, output_dir, language, &mut candidates)?;
+        candidates
+    };
+
+    for relative_path in stale_candidates {
+        if expected_files.contains(&relative_path) {
+            continue;
+        }
+        let path = output_dir.join(&relative_path);
+        if path.is_file() && is_generated_file(&path, language) {
+            std::fs::remove_file(&path)
+                .context(format!("Failed to remove stale generated file: {path:?}"))?;
+            prune_empty_generated_dirs(output_dir, path.parent());
+        }
+    }
+
+    Ok(())
+}
+
+fn read_generated_manifest(path: &Path) -> anyhow::Result<BTreeSet<PathBuf>> {
+    let content = std::fs::read_to_string(path)
+        .context(format!("Failed to read generated file manifest: {path:?}"))?;
+    let mut files = BTreeSet::new();
+    for line in content.lines() {
+        let line = line.trim();
+        if line.is_empty() || line.starts_with('#') {
+            continue;
+        }
+        files.insert(generated_relative_path(line)?.to_path_buf());
+    }
+    Ok(files)
+}
+
+fn collect_legacy_generated_files(
+    output_dir: &Path,
+    dir: &Path,
+    language: &Language,
+    files: &mut BTreeSet<PathBuf>,
+) -> anyhow::Result<()> {
+    for entry in std::fs::read_dir(dir).context(format!("Failed to read directory: {dir:?}"))? {
+        let entry = entry?;
+        let path = entry.path();
+        let file_type = entry
+            .file_type()
+            .context(format!("Failed to inspect directory entry: {path:?}"))?;
+        if file_type.is_symlink() {
+            continue;
+        }
+        if file_type.is_dir() {
+            collect_legacy_generated_files(output_dir, &path, language, files)?;
+            continue;
+        }
+        if path.file_name().and_then(|name| name.to_str()) == Some(GENERATED_MANIFEST) {
+            continue;
+        }
+        if is_generated_file(&path, language) {
+            let relative = path
+                .strip_prefix(output_dir)
+                .context(format!("Generated path escaped output directory: {path:?}"))?;
+            files.insert(generated_relative_path(&relative.to_string_lossy())?.to_path_buf());
+        }
+    }
+    Ok(())
+}
+
+fn is_generated_file(path: &Path, language: &Language) -> bool {
+    if !matches_language_extension(path, language) {
+        return false;
+    }
+    let Ok(content) = std::fs::read_to_string(path) else {
+        return false;
+    };
+    content.contains(GENERATED_MARKER)
+}
+
+fn matches_language_extension(path: &Path, language: &Language) -> bool {
+    let extension = path.extension().and_then(|extension| extension.to_str());
+    match language {
+        Language::Typescript => extension == Some("ts"),
+        Language::Rust => extension == Some("rs"),
+        Language::Python => extension == Some("py"),
+        Language::Openapi => extension == Some("json"),
+    }
+}
+
+fn prune_empty_generated_dirs(output_dir: &Path, mut dir: Option<&Path>) {
+    while let Some(current) = dir {
+        if current == output_dir || !current.starts_with(output_dir) {
+            break;
+        }
+        match std::fs::remove_dir(current) {
+            Ok(()) => dir = current.parent(),
+            Err(_) => break,
+        }
+    }
+}
+
+fn write_generated_manifest(output_dir: &Path, files: &BTreeSet<PathBuf>) -> anyhow::Result<()> {
+    let mut content = format!("# {GENERATED_MARKER}\n");
+    content.push_str("# Relative files generated during the last reflectapi codegen run.\n");
+    for file in files {
+        content.push_str(&file.to_string_lossy());
+        content.push('\n');
+    }
+    write_file(&output_dir.join(GENERATED_MANIFEST), &content)
+}
+
 fn parent_or_dot(path: &std::path::Path) -> std::path::PathBuf {
     path.parent()
         .filter(|p| !p.as_os_str().is_empty())

reflectapi-cli/tests/output_paths.rs

@@ -8,7 +8,7 @@
 //!   - stdout via `--output -`, which must print the language's *primary*
 //!     file rather than the alphabetically-first one.
 
-use std::process::Command;
+use std::{fs, process::Command};
 
 fn cargo_bin() -> std::path::PathBuf {
     let bin = env!("CARGO_BIN_EXE_reflectapi");
@@ -30,6 +30,49 @@ fn run(args: &[&str]) -> std::process::Output {
         .expect("spawn reflectapi")
 }
 
+fn write_minimal_python_schema(path: &std::path::Path, type_name: &str) {
+    let schema = format!(
+        r#"{{
+  "name": "CLI stale cleanup test",
+  "description": "",
+  "functions": [
+    {{
+      "name": "items.get",
+      "path": "",
+      "output_kind": "complete",
+      "output_type": {{ "name": "{type_name}" }},
+      "serialization": ["json"],
+      "readonly": true
+    }}
+  ],
+  "input_types": {{ "types": [] }},
+  "output_types": {{
+    "types": [
+      {{
+        "kind": "primitive",
+        "name": "std::string::String",
+        "description": "String"
+      }},
+      {{
+        "kind": "struct",
+        "name": "{type_name}",
+        "fields": {{
+          "named": [
+            {{
+              "name": "value",
+              "type": {{ "name": "std::string::String" }},
+              "required": true
+            }}
+          ]
+        }}
+      }}
+    ]
+  }}
+}}"#
+    );
+    fs::write(path, schema).unwrap();
+}
+
 #[test]
 fn ts_output_into_fresh_directory() {
     let tmp = tempfile::tempdir().unwrap();
@@ -80,6 +123,112 @@ fn python_output_into_fresh_directory() {
     assert!(target.join("__init__.py").is_file());
 }
 
+#[test]
+fn python_directory_output_removes_stale_generated_files() {
+    let tmp = tempfile::tempdir().unwrap();
+    let target = tmp.path().join("python-client");
+    fs::create_dir_all(target.join("legacy")).unwrap();
+    fs::write(
+        target.join("legacy/__init__.py"),
+        "\"\"\"\nDO NOT MODIFY THIS FILE MANUALLY\nThis file was generated by reflectapi-cli\n\"\"\"\n",
+    )
+    .unwrap();
+    fs::write(target.join("user_notes.py"), "# hand written\n").unwrap();
+
+    let schema_one = tmp.path().join("schema-one.json");
+    let schema_two = tmp.path().join("schema-two.json");
+    write_minimal_python_schema(&schema_one, "first::child::Thing");
+    write_minimal_python_schema(&schema_two, "second::Thing");
+
+    let out = run(&[
+        "codegen",
+        "--language",
+        "python",
+        "--schema",
+        schema_one.to_str().unwrap(),
+        "--output",
+        target.to_str().unwrap(),
+    ]);
+    assert!(
+        out.status.success(),
+        "stderr:\n{}",
+        String::from_utf8_lossy(&out.stderr)
+    );
+    assert!(
+        !target.join("legacy/__init__.py").exists(),
+        "pre-manifest generated files should be removed by header scan"
+    );
+    assert!(target.join("first/child/__init__.py").is_file());
+    assert!(target.join("user_notes.py").is_file());
+
+    let out = run(&[
+        "codegen",
+        "--language",
+        "python",
+        "--schema",
+        schema_two.to_str().unwrap(),
+        "--output",
+        target.to_str().unwrap(),
+    ]);
+    assert!(
+        out.status.success(),
+        "stderr:\n{}",
+        String::from_utf8_lossy(&out.stderr)
+    );
+    assert!(
+        !target.join("first/child/__init__.py").exists(),
+        "manifest-listed files absent from the new generation should be removed"
+    );
+    assert!(target.join("second/__init__.py").is_file());
+    assert!(target.join("user_notes.py").is_file());
+
+    let manifest = fs::read_to_string(target.join(".reflectapi-generated-files")).unwrap();
+    assert!(manifest.contains("second/__init__.py"));
+    assert!(!manifest.contains("first/child/__init__.py"));
+}
+
+#[cfg(unix)]
+#[test]
+fn python_legacy_cleanup_skips_symlinked_directories() {
+    use std::os::unix::fs::symlink;
+
+    let tmp = tempfile::tempdir().unwrap();
+    let target = tmp.path().join("python-client");
+    let external = tmp.path().join("external");
+    fs::create_dir_all(&target).unwrap();
+    fs::create_dir_all(&external).unwrap();
+    fs::write(
+        external.join("__init__.py"),
+        "\"\"\"\nDO NOT MODIFY THIS FILE MANUALLY\nThis file was generated by reflectapi-cli\n\"\"\"\n",
+    )
+    .unwrap();
+    symlink(&external, target.join("linked")).unwrap();
+
+    let schema = tmp.path().join("schema.json");
+    write_minimal_python_schema(&schema, "current::Thing");
+
+    let out = run(&[
+        "codegen",
+        "--language",
+        "python",
+        "--schema",
+        schema.to_str().unwrap(),
+        "--output",
+        target.to_str().unwrap(),
+    ]);
+    assert!(
+        out.status.success(),
+        "stderr:\n{}",
+        String::from_utf8_lossy(&out.stderr)
+    );
+    assert!(
+        external.join("__init__.py").is_file(),
+        "legacy cleanup should not follow directory symlinks out of the output tree"
+    );
+    assert!(target.join("linked").is_symlink());
+    assert!(target.join("current/__init__.py").is_file());
+}
+
 #[test]
 fn ts_output_to_file_path_writes_siblings() {
     // --output …/generated.ts should still work; transport file lands

reflectapi-demo/clients/python/api_client/.reflectapi-generated-files

@@ -0,0 +1,11 @@
+# This file was generated by reflectapi-cli
+# Relative files generated during the last reflectapi codegen run.
+__init__.py
+_client.py
+_rebuild.py
+generated.py
+myapi/__init__.py
+myapi/model/__init__.py
+myapi/model/input/__init__.py
+myapi/model/output/__init__.py
+myapi/proto/__init__.py