sadpandajoe commented on code in PR #39724:
URL: https://github.com/apache/superset/pull/39724#discussion_r4128720146
##########
superset-frontend/package.json:
##########
@@ -43,7 +43,7 @@
"build-dev": "cross-env NODE_OPTIONS=--max_old_space_size=8192
NODE_ENV=development webpack --mode=development --color",
"build-instrumented": "cross-env NODE_ENV=production
BABEL_ENV=instrumented webpack --mode=production --color",
"build-storybook": "storybook build",
- "build-translation": "scripts/po2json.sh",
+ "build-translation": "python3 ../scripts/translations/compile_po.py",
Review Comment:
On Windows using the standard python.org installer, there's typically no
`python3` command on PATH (only `python`/`py`) unless the user manually enabled
the alias, so `npm run build-translation` fails to find this script and no
translation JSON gets generated -- the opposite of this port's cross-platform
goal. Could this invoke `python` (or `py -3`) instead of hardcoding `python3`?
##########
scripts/change_detector.py:
##########
@@ -56,6 +56,9 @@
"frontend": [
r"^\.github/workflows/.*(bashlib|frontend|e2e)",
r"^superset-frontend/",
+ # `npm run build-translation` shells out to this script; a change
+ # limited to it should still exercise that frontend build step.
+ r"^scripts/translations/compile_po\.py$",
Review Comment:
tests/unit_tests/scripts/change_detector_test.py has no case asserting that
a change limited to this file alone maps to the `frontend` category -- a typo
or wrong category here would silently skip the frontend CI job for changes to
this script. Could this get a test case?
##########
scripts/translations/compile_po.py:
##########
@@ -0,0 +1,175 @@
+#!/usr/bin/env python3
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Cross-platform Python port of ``po2json.sh``.
Review Comment:
.gitignore:115 still says these generated files come from
`./scripts/po2json.sh`, which this PR deletes -- a contributor tracing why
`messages.json` is ignored would land on a dead path. Could that comment be
updated to point at this script instead?
##########
tests/unit_tests/scripts/translations/compile_po_test.py:
##########
@@ -0,0 +1,323 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+"""Tests for ``scripts/translations/compile_po.py``.
+
+The script is not installed as a package, so it is loaded via importlib from
+its filesystem path.
+"""
+
+from __future__ import annotations
+
+import importlib.util
+import json # noqa: TID251 - testing a standalone script that uses stdlib json
+from pathlib import Path
+from unittest.mock import MagicMock, patch
+
+_SCRIPT_PATH = (
+ Path(__file__).resolve().parents[4] / "scripts" / "translations" /
"compile_po.py"
+)
+_spec = importlib.util.spec_from_file_location("compile_po", _SCRIPT_PATH)
+assert _spec is not None, f"Could not load {_SCRIPT_PATH}"
+assert _spec.loader is not None, f"No loader on spec for {_SCRIPT_PATH}"
+compile_po = importlib.util.module_from_spec(_spec)
+_spec.loader.exec_module(compile_po)
+
+
+# ---------------------------------------------------------------------------
+# resolve_node_entry
+# ---------------------------------------------------------------------------
+
+
+def test_resolve_node_entry_dict_bin(tmp_path: Path) -> None:
+ """A dict "bin" field is resolved by package name, not the first key."""
+ pkg_dir = tmp_path / "node_modules" / "po2json"
+ pkg_dir.mkdir(parents=True)
+ (pkg_dir / "package.json").write_text(
+ json.dumps({"bin": {"po2json": "bin/po2json"}})
+ )
+ (pkg_dir / "bin").mkdir()
+ entry = pkg_dir / "bin" / "po2json"
+ entry.touch()
+
+ with patch.object(compile_po, "FRONTEND_DIR", str(tmp_path)):
+ resolved = compile_po.resolve_node_entry("po2json")
+ assert resolved == str(entry)
+
+
+def test_resolve_node_entry_string_bin(tmp_path: Path) -> None:
+ """A plain string "bin" field (single-command package shorthand) resolves
too."""
+ pkg_dir = tmp_path / "node_modules" / "oxfmt"
+ pkg_dir.mkdir(parents=True)
+ (pkg_dir / "package.json").write_text(json.dumps({"bin": "bin/oxfmt"}))
+ (pkg_dir / "bin").mkdir()
+ entry = pkg_dir / "bin" / "oxfmt"
+ entry.touch()
+
+ with patch.object(compile_po, "FRONTEND_DIR", str(tmp_path)):
+ resolved = compile_po.resolve_node_entry("oxfmt")
+ assert resolved == str(entry)
+
+
+def test_resolve_node_entry_missing_package(tmp_path: Path) -> None:
+ """Returns None when the package isn't installed at all."""
+ with patch.object(compile_po, "FRONTEND_DIR", str(tmp_path)):
+ assert compile_po.resolve_node_entry("po2json") is None
+
+
+def test_resolve_node_entry_missing_bin_field(tmp_path: Path) -> None:
+ """Returns None when package.json has no "bin" field."""
+ pkg_dir = tmp_path / "node_modules" / "po2json"
+ pkg_dir.mkdir(parents=True)
+ (pkg_dir / "package.json").write_text(json.dumps({"name": "po2json"}))
+
+ with patch.object(compile_po, "FRONTEND_DIR", str(tmp_path)):
+ assert compile_po.resolve_node_entry("po2json") is None
+
+
+def test_resolve_node_entry_entry_file_missing(tmp_path: Path) -> None:
+ """Returns None when package.json declares a bin entry that doesn't exist
+ on disk (a partially-installed / corrupted node_modules)."""
+ pkg_dir = tmp_path / "node_modules" / "po2json"
+ pkg_dir.mkdir(parents=True)
+ (pkg_dir / "package.json").write_text(
+ json.dumps({"bin": {"po2json": "bin/po2json"}})
+ )
+
+ with patch.object(compile_po, "FRONTEND_DIR", str(tmp_path)):
+ assert compile_po.resolve_node_entry("po2json") is None
+
+
+# ---------------------------------------------------------------------------
+# run
+# ---------------------------------------------------------------------------
+
+
+def test_run_returns_process_returncode() -> None:
+ """run() returns the child process's exit code and never shells out."""
+ with patch.object(compile_po.subprocess, "run") as mock_run:
+ mock_run.return_value = MagicMock(returncode=3)
+ rc = compile_po.run(["node", "script.js"])
+ assert rc == 3
+ args, kwargs = mock_run.call_args
+ assert args[0] == ["node", "script.js"]
+ assert "shell" not in kwargs
+
+
+# ---------------------------------------------------------------------------
+# convert_po_to_json
+# ---------------------------------------------------------------------------
+
+
+def test_convert_po_to_json_success(tmp_path: Path) -> None:
+ """Builds the po2json argv and writes to the .po file's .json sibling."""
+ po_file = tmp_path / "fr" / "LC_MESSAGES" / "messages.po"
+ po_file.parent.mkdir(parents=True)
+ po_file.write_text('msgid ""\nmsgstr ""\n')
+
+ with patch.object(compile_po, "run", return_value=0) as mock_run:
+ json_dest = compile_po.convert_po_to_json(
+ "/usr/bin/node", "/pkg/bin/po2json", str(po_file)
+ )
+
+ assert json_dest == str(po_file.with_suffix(".json"))
+ mock_run.assert_called_once_with(
+ [
+ "/usr/bin/node",
+ "/pkg/bin/po2json",
+ "--domain",
+ "superset",
+ "--format",
+ "jed1.x",
+ "--fuzzy",
+ str(po_file),
+ str(po_file.with_suffix(".json")),
+ ]
+ )
+
+
+def test_convert_po_to_json_failure() -> None:
+ """Reports failure when po2json returns non-zero."""
+ with patch.object(compile_po, "run", return_value=1):
+ json_dest = compile_po.convert_po_to_json(
+ "/usr/bin/node", "/pkg/bin/po2json", "x.po"
+ )
+ assert json_dest is None
+
+
+def test_convert_po_to_json_preserves_locale_in_path(tmp_path: Path) -> None:
+ """Regression: two locales' messages.po must not collide on one .json
+ output -- the destination is derived from the full glob path (which
+ includes the locale and LC_MESSAGES components), not a locale-stripped
+ relative path."""
+ fr_po = tmp_path / "fr" / "LC_MESSAGES" / "messages.po"
+ de_po = tmp_path / "de" / "LC_MESSAGES" / "messages.po"
+ for f in (fr_po, de_po):
+ f.parent.mkdir(parents=True)
+ f.touch()
+
+ destinations = []
+ with patch.object(compile_po, "run", return_value=0) as mock_run:
+ for po_file in (fr_po, de_po):
+ compile_po.convert_po_to_json(
+ "/usr/bin/node", "/pkg/bin/po2json", str(po_file)
+ )
+ destinations.append(mock_run.call_args.args[0][-1])
+
+ assert destinations[0] != destinations[1]
+ assert destinations[0] == str(fr_po.with_suffix(".json"))
+ assert destinations[1] == str(de_po.with_suffix(".json"))
+
+
+# ---------------------------------------------------------------------------
+# main
+# ---------------------------------------------------------------------------
+
+
+def test_main_missing_node() -> None:
+ """Returns 1 when node isn't on PATH."""
+ with patch.object(compile_po.shutil, "which", return_value=None):
+ assert compile_po.main() == 1
+
+
+def test_main_missing_npm_packages() -> None:
+ """Returns 1 when po2json or oxfmt aren't installed."""
+ with (
+ patch.object(compile_po.shutil, "which", return_value="/usr/bin/node"),
+ patch.object(compile_po, "resolve_node_entry", return_value=None),
+ ):
+ assert compile_po.main() == 1
+
+
+def test_main_missing_translations_dir(tmp_path: Path) -> None:
+ """Returns 1 when the translations directory doesn't exist."""
+ with (
+ patch.object(compile_po.shutil, "which", return_value="/usr/bin/node"),
+ patch.object(compile_po, "resolve_node_entry",
return_value="/pkg/bin/x"),
+ patch.object(compile_po, "TRANSLATIONS_DIR", str(tmp_path / "nope")),
+ ):
+ assert compile_po.main() == 1
+
+
+def test_main_reports_conversion_failures(tmp_path: Path) -> None:
Review Comment:
A few scenarios in main()'s flow aren't exercised: every test here mocks
`resolve_node_entry` to return the identical path for both `po2json` and
`oxfmt` (e.g. line 243), so a swapped entry between the conversion and
formatting calls would still pass; this failure case only covers a single,
all-failing `.po` file, not a mix of some succeeding and some failing; and an
existing-but-empty `TRANSLATIONS_DIR` (zero `.po` files) has no test, so a
regression that still invokes oxfmt with no file operands wouldn't be caught.
Could these get their own assertions?
##########
scripts/translations/compile_po.py:
##########
@@ -0,0 +1,175 @@
+#!/usr/bin/env python3
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+"""Cross-platform Python port of ``po2json.sh``.
+
+Compiles ``superset/translations/**/*.po`` into sibling ``.json`` files for
+the frontend (``po2json``), then formats the generated JSON with ``oxfmt`` --
+the same two steps ``po2json.sh`` performed, in the same order.
+
+Each tool is invoked as ``node <package's own bin entry point>``, resolved
+from the package's own ``package.json`` "bin" field, rather than through the
+platform-specific ``node_modules/.bin`` wrapper (a symlink on POSIX, a
+``.cmd``/``.ps1`` shim on Windows). ``node`` is a real executable on every
+platform, so this never goes through ``cmd.exe`` the way invoking a ``.cmd``
+wrapper does (even with ``subprocess``'s ``shell=False``) -- so there is no
+shell-metacharacter or ``%VAR%``-expansion surface to defend against here.
+
+Usage:
+ python scripts/translations/compile_po.py
+"""
+
+from __future__ import annotations
+
+import glob
+import json # noqa: TID251 - standalone script, not the Flask app
+import os
+import shutil
+import subprocess
+import sys
+
+ROOT_DIR = os.path.abspath(
+ os.path.join(os.path.dirname(os.path.abspath(__file__)), "..", "..")
+)
+FRONTEND_DIR = os.path.join(ROOT_DIR, "superset-frontend")
+TRANSLATIONS_DIR = os.path.join(ROOT_DIR, "superset", "translations")
+
+
+def resolve_node_entry(package: str) -> str | None:
+ """Resolve an installed npm package's CLI entry point.
+
+ Reads the entry path out of the package's own ``package.json`` "bin"
+ field instead of guessing at the ``node_modules/.bin`` wrapper's shape,
+ so the result can always be run via ``node <entry>`` directly.
+ """
+ pkg_dir = os.path.join(FRONTEND_DIR, "node_modules", package)
+ manifest_path = os.path.join(pkg_dir, "package.json")
+ if not os.path.isfile(manifest_path):
+ return None
+ with open(manifest_path, encoding="utf-8") as f:
+ manifest = json.load(f)
+ bin_field = manifest.get("bin")
+ rel_entry = bin_field.get(package) if isinstance(bin_field, dict) else
bin_field
+ if not rel_entry:
+ return None
+ entry_path = os.path.normpath(os.path.join(pkg_dir, rel_entry))
+ return entry_path if os.path.isfile(entry_path) else None
+
+
+def run(command: list[str]) -> int:
+ """Run a command directly, with no shell involved, and return its exit
+ code."""
+ return subprocess.run(command, check=False).returncode # noqa: S603
+
+
+def convert_po_to_json(node_bin: str, po2json_entry: str, po_file: str) -> str
| None:
+ """Convert one ``.po`` file to its sibling ``.json`` via ``po2json``.
+
+ Returns the generated ``.json`` path on success, ``None`` on failure.
+ """
+ json_dest = f"{os.path.splitext(po_file)[0]}.json"
+ rc = run(
+ [
+ node_bin,
+ po2json_entry,
+ "--domain",
+ "superset",
+ "--format",
+ "jed1.x",
+ "--fuzzy",
+ po_file,
+ json_dest,
+ ]
+ )
+ return json_dest if rc == 0 else None
+
+
+def main() -> int:
+ """Convert every ``.po`` file under ``superset/translations`` to
+ ``.json``, then format the generated JSON with ``oxfmt``."""
+ node_bin = shutil.which("node")
Review Comment:
`shutil.which("node")` isn't guaranteed to return `node.exe` -- on Windows,
if a `node.cmd` shim resolves first on PATH, `subprocess.run` here still routes
through `cmd.exe` for that batch file even with the default `shell=False`,
reopening metacharacter injection via an attacker-controlled `.po` filename
(e.g. `messages&whoami&.po`) for that one case. Could this restrict resolution
to a real Windows executable rather than accepting any PATH match for `node`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
