Skip to content

Commit

Permalink
Enforce that paths used for BundleMap and NativeAppCompiler are alway…
Browse files Browse the repository at this point in the history
…s absolute (#1678)

Subtle cwd-dependent bugs can creep in if we work with relative paths too much. Let's enforce that all paths used to initialize `BundleMap` and `NativeAppCompiler` should be absolute so there's no doubt about where files will be read from or written to. Paths to individual files within these roots are still allowed to be relative.
  • Loading branch information
sfc-gh-fcampbell authored and sfc-gh-mraba committed Oct 14, 2024
1 parent 977a022 commit 1b8fe83
Show file tree
Hide file tree
Showing 8 changed files with 87 additions and 23 deletions.
1 change: 1 addition & 0 deletions RELEASE-NOTES.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@


## Fixes and improvements
* Fixed a bug that would cause the `deploy_root`, `bundle_root`, and `generated_root` directories to be created in the current working directory instead of the project root when invoking commands with the `--project` flag from a different directory.


# v3.0.0
Expand Down
12 changes: 12 additions & 0 deletions src/snowflake/cli/_plugins/nativeapp/artifacts.py
Original file line number Diff line number Diff line change
Expand Up @@ -240,9 +240,21 @@ class BundleMap:
"""
Computes the mapping between project directory artifacts (aka source artifacts) to their deploy root location
(aka destination artifact). This information is primarily used when bundling a native applications project.
:param project_root: The root directory of the project and base for all relative paths. Must be an absolute path.
:param deploy_root: The directory where artifacts should be copied to. Must be an absolute path.
"""

def __init__(self, *, project_root: Path, deploy_root: Path):
# If a relative path ends up here, it's a bug in the app and can lead to other
# subtle bugs as paths would be resolved relative to the current working directory.
assert (
project_root.is_absolute()
), f"Project root {project_root} must be an absolute path."
assert (
deploy_root.is_absolute()
), f"Deploy root {deploy_root} must be an absolute path."

self._project_root: Path = resolve_without_follow(project_root)
self._deploy_root: Path = resolve_without_follow(deploy_root)
self._artifact_map = _ArtifactPathMap(project_root=self._project_root)
Expand Down
7 changes: 7 additions & 0 deletions src/snowflake/cli/_plugins/nativeapp/codegen/compiler.py
Original file line number Diff line number Diff line change
Expand Up @@ -65,10 +65,17 @@ def __init__(
self,
bundle_ctx: BundleContext,
):
self._assert_absolute_paths(bundle_ctx)
self._bundle_ctx = bundle_ctx
# dictionary of all processors created and shared between different artifact objects.
self.cached_processors: Dict[str, ArtifactProcessor] = {}

@staticmethod
def _assert_absolute_paths(bundle_ctx: BundleContext):
for name in ["Project", "Deploy", "Bundle", "Generated"]:
path = getattr(bundle_ctx, f"{name.lower()}_root")
assert path.is_absolute(), f"{name} root {path} must be an absolute path."

def compile_artifacts(self):
"""
Go through every artifact object in the project definition of a native app, and execute processors in order of specification for each of the artifact object.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,11 +185,13 @@ def action_deploy(
policy = DenyAlwaysPolicy()

return self.deploy(
console=ctx.console,
project_root=ctx.project_root,
deploy_root=Path(model.deploy_root),
bundle_root=Path(model.bundle_root),
generated_root=Path(model.generated_root),
console=workspace_ctx.console,
project_root=workspace_ctx.project_root,
deploy_root=workspace_ctx.project_root / model.deploy_root,
bundle_root=workspace_ctx.project_root / model.bundle_root,
generated_root=(
workspace_ctx.project_root / model.deploy_root / model.generated_root
),
artifacts=model.artifacts,
bundle_map=None,
package_name=package_name,
Expand Down Expand Up @@ -237,11 +239,13 @@ def action_validate(
policy = DenyAlwaysPolicy()

self.validate_setup_script(
console=ctx.console,
project_root=ctx.project_root,
deploy_root=Path(model.deploy_root),
bundle_root=Path(model.bundle_root),
generated_root=Path(model.generated_root),
console=workspace_ctx.console,
project_root=workspace_ctx.project_root,
deploy_root=workspace_ctx.project_root / model.deploy_root,
bundle_root=workspace_ctx.project_root / model.bundle_root,
generated_root=(
workspace_ctx.project_root / model.deploy_root / model.generated_root
),
artifacts=model.artifacts,
package_name=package_name,
package_role=(model.meta and model.meta.role) or ctx.default_role,
Expand Down Expand Up @@ -284,11 +288,13 @@ def action_version_create(
model = self._entity_model
package_name = model.fqn.identifier
return self.version_create(
console=ctx.console,
project_root=ctx.project_root,
deploy_root=Path(model.deploy_root),
bundle_root=Path(model.bundle_root),
generated_root=Path(model.generated_root),
console=workspace_ctx.console,
project_root=workspace_ctx.project_root,
deploy_root=workspace_ctx.project_root / model.deploy_root,
bundle_root=workspace_ctx.project_root / model.bundle_root,
generated_root=(
workspace_ctx.project_root / model.deploy_root / model.generated_root
),
artifacts=model.artifacts,
package_name=package_name,
package_role=(model.meta and model.meta.role) or ctx.default_role,
Expand Down Expand Up @@ -323,11 +329,13 @@ def action_version_drop(
model = self._entity_model
package_name = model.fqn.identifier
return self.version_drop(
console=ctx.console,
project_root=ctx.project_root,
deploy_root=Path(model.deploy_root),
bundle_root=Path(model.bundle_root),
generated_root=Path(model.generated_root),
console=workspace_ctx.console,
project_root=workspace_ctx.project_root,
deploy_root=workspace_ctx.project_root / model.deploy_root,
bundle_root=workspace_ctx.project_root / model.bundle_root,
generated_root=(
workspace_ctx.project_root / model.deploy_root / model.generated_root
),
artifacts=model.artifacts,
package_name=package_name,
package_role=(model.meta and model.meta.role) or ctx.default_role,
Expand Down
2 changes: 1 addition & 1 deletion src/snowflake/cli/api/project/definition_conversion.py
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ def _find_manifest():
# manifest file from the resultant BundleMap, since the bundle process ensures
# that only a single source path can map to the corresponding destination path
bundle_map = BundleMap(
project_root=project_root, deploy_root=Path(native_app.deploy_root)
project_root=project_root, deploy_root=project_root / native_app.deploy_root
)
for artifact in native_app.artifacts:
bundle_map.add(artifact)
Expand Down
17 changes: 16 additions & 1 deletion tests/nativeapp/codegen/test_compiler.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,8 @@
# 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.

import re
from pathlib import Path

import pytest
from snowflake.cli._plugins.nativeapp.codegen.artifact_processor import (
Expand Down Expand Up @@ -62,6 +63,20 @@ def test_compiler(test_proj_def):
return NativeAppCompiler(na_project.get_bundle_context())


@pytest.mark.parametrize("name", ["Project", "Deploy", "Bundle", "Generated"])
def test_compiler_requires_absolute_paths(test_proj_def, name):
na_project = create_native_app_project_model(test_proj_def.native_app)
bundle_context = na_project.get_bundle_context()

path = Path()
setattr(bundle_context, f"{name.lower()}_root", path)
with pytest.raises(
AssertionError,
match=re.escape(rf"{name} root {path} must be an absolute path."),
):
NativeAppCompiler(bundle_context)


def test_try_create_processor_returns_none(test_proj_def, test_compiler):
artifact_to_process = test_proj_def.native_app.artifacts[2]
result = test_compiler._try_create_processor( # noqa: SLF001
Expand Down
19 changes: 19 additions & 0 deletions tests/nativeapp/test_artifacts.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
from __future__ import annotations

import os
import re
from pathlib import Path
from typing import Dict, Iterable, List, Optional, Union

Expand Down Expand Up @@ -166,6 +167,24 @@ def test_empty_bundle_map(bundle_map):
)


def test_bundle_map_requires_absolute_project_root():
project_root = Path()
with pytest.raises(
AssertionError,
match=re.escape(rf"Project root {project_root} must be an absolute path."),
):
BundleMap(project_root=project_root, deploy_root=Path("output/deploy"))


def test_bundle_map_requires_absolute_deploy_root():
deploy_root = Path("output/deploy")
with pytest.raises(
AssertionError,
match=re.escape(rf"Deploy root {deploy_root} must be an absolute path."),
):
BundleMap(project_root=Path().resolve(), deploy_root=deploy_root)


def test_bundle_map_handles_file_to_file_mappings(bundle_map):
bundle_map.add(PathMapping(src="README.md", dest="deployed_readme.md"))
bundle_map.add(PathMapping(src="app/setup.sql", dest="app_setup.sql"))
Expand Down
4 changes: 3 additions & 1 deletion tests/workspace/test_application_package_entity.py
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,9 @@ def test_deploy(

mock_sync.assert_called_once_with(
console=mock_console,
deploy_root=Path("output/deploy"),
deploy_root=(
app_pkg._workspace_ctx.project_root / Path("output/deploy") # noqa SLF001
),
package_name="pkg",
stage_schema="app_src",
bundle_map=mock.ANY,
Expand Down

0 comments on commit 1b8fe83

Please sign in to comment.