From 189e88f1dbd3a5215ef35aeb9be443e21a97fcb8 Mon Sep 17 00:00:00 2001
From: Saugat Pachhai <suagatchhetri@outlook.com>
Date: Thu, 10 Dec 2020 16:05:45 +0545
Subject: [PATCH] Run graph checks on `collect`/`find_outs_by_path` (#5035)

We try to optimize `tree.exists` calls and probably few others
in that they either look directly into the workspace or,
to the cache without running graph checks. It does not seem
to be possible just to run graph checks on `find_outs_by_path`
due to those optimizations.

So, that's why, the `collect` also does a graph check for this
reason.

Fixes #5027
Fixes #4010
---
 dvc/repo/__init__.py              |  4 +--
 dvc/repo/collect.py               |  2 +-
 tests/func/metrics/test_common.py | 47 ++++++++++++++++++++++++++++++-
 tests/unit/repo/test_repo.py      | 11 ++++++++
 4 files changed, 60 insertions(+), 4 deletions(-)

diff --git a/dvc/repo/__init__.py b/dvc/repo/__init__.py
index a18cdf022..e2b9b2170 100644
--- a/dvc/repo/__init__.py
+++ b/dvc/repo/__init__.py
@@ -380,8 +380,8 @@ class Repo:
         return self.stage.collect_repo(onerror=error_handler)
 
     def find_outs_by_path(self, path, outs=None, recursive=False, strict=True):
-        if not outs:
-            outs = [out for stage in self.stages for out in stage.outs]
+        # using `outs_graph` to ensure graph checks are run
+        outs = outs or self.outs_graph
 
         abs_path = os.path.abspath(path)
         path_info = PathInfo(abs_path)
diff --git a/dvc/repo/collect.py b/dvc/repo/collect.py
index 78ee4e1aa..32dd92473 100644
--- a/dvc/repo/collect.py
+++ b/dvc/repo/collect.py
@@ -21,7 +21,7 @@ def _collect_outs(
 ) -> Outputs:
     outs = {
         out
-        for stage in repo.stages
+        for stage in repo.graph  # using `graph` to ensure graph checks run
         for out in (stage.deps if deps else stage.outs)
     }
     return set(filter(output_filter, outs)) if output_filter else outs
diff --git a/tests/func/metrics/test_common.py b/tests/func/metrics/test_common.py
index 2ba3b6e54..1a3d3735c 100644
--- a/tests/func/metrics/test_common.py
+++ b/tests/func/metrics/test_common.py
@@ -2,7 +2,14 @@ import logging
 
 import pytest
 
-from dvc.exceptions import NoMetricsFoundError, NoMetricsParsedError
+from dvc.exceptions import (
+    NoMetricsFoundError,
+    NoMetricsParsedError,
+    OverlappingOutputPathsError,
+)
+from dvc.path_info import PathInfo
+from dvc.utils.fs import remove
+from dvc.utils.serialize import dump_yaml, modify_yaml
 from tests.func.metrics.utils import _write_json
 
 
@@ -61,3 +68,41 @@ def test_show_malformed_metric(
 def test_show_no_metrics_files(tmp_dir, dvc, caplog, show):
     with pytest.raises(NoMetricsFoundError):
         show(dvc)
+
+
+@pytest.mark.parametrize("clear_before_run", [True, False])
+@pytest.mark.parametrize("typ", ["metrics", "plots"])
+def test_metrics_show_overlap(
+    tmp_dir, dvc, run_copy_metrics, clear_before_run, typ
+):
+    data_dir = PathInfo("data")
+    (tmp_dir / data_dir).mkdir()
+
+    outs = {typ: [str(data_dir / "m1.yaml")]}
+    dump_yaml(data_dir / "m1_temp.yaml", {"a": {"b": {"c": 2, "d": 1}}})
+    run_copy_metrics(
+        str(data_dir / "m1_temp.yaml"),
+        str(data_dir / "m1.yaml"),
+        single_stage=False,
+        commit=f"add m1 {typ}",
+        name="cp-m1",
+        **outs,
+    )
+    with modify_yaml("dvc.yaml") as d:
+        # trying to make an output overlaps error
+        d["stages"]["corrupted-stage"] = {
+            "cmd": "mkdir data",
+            "outs": ["data"],
+        }
+
+    # running by clearing and not clearing stuffs
+    # so as it works even for optimized cases
+    if clear_before_run:
+        remove(data_dir)
+        remove(dvc.cache.local.cache_dir)
+
+    dvc._reset()
+
+    show = dvc.metrics.show if typ == "metrics" else dvc.plots.show
+    with pytest.raises(OverlappingOutputPathsError):
+        show()
diff --git a/tests/unit/repo/test_repo.py b/tests/unit/repo/test_repo.py
index 272d5b78e..5b9b6f536 100644
--- a/tests/unit/repo/test_repo.py
+++ b/tests/unit/repo/test_repo.py
@@ -1,7 +1,9 @@
 import os
+import shutil
 
 import pytest
 
+from dvc.exceptions import OutputDuplicationError
 from dvc.repo import NotDvcRepoError, Repo, locked
 from dvc.utils.fs import remove
 
@@ -29,6 +31,15 @@ def test_find_outs_by_path(tmp_dir, dvc, path):
     assert outs[0].path_info == stage.outs[0].path_info
 
 
+def test_find_outs_by_path_does_graph_checks(tmp_dir, dvc):
+    tmp_dir.dvc_gen("foo", "foo")
+    shutil.copyfile("foo.dvc", "foo-2.dvc")
+
+    dvc._reset()
+    with pytest.raises(OutputDuplicationError):
+        dvc.find_outs_by_path("foo")
+
+
 @pytest.mark.parametrize(
     "path",
     [os.path.join("dir", "subdir", "file"), os.path.join("dir", "subdir")],