Repository: frictionlessdata/frictionless-py

Issue:
Foreign key errors showing headers instead of row values
# Overview
* Version: **`v4.22.1`**


The foreign key errors returned in the report are the **header** values of the resource instead of the actual **row values**. Also, the message does not provide any hints about the problem: which is the foreign key violated? which fields? 
I would expect more details about the `foreign-key-error` alongside the other fields of the error found in the report. Does it makes sense?

![frictionless-foreign-key-error-bug](https://user-images.githubusercontent.com/632860/144231656-b55afd1a-acef-40f5-aa9b-44aa384c144a.png)

# How to reproduce
Here is ZIP with a minimal example to reproduce the problem: [frictionless-foreign-key-bug.zip](https://github.com/frictionlessdata/frictionless-py/files/7633635/frictionless-foreign-key-bug.zip)
Use the **awesome** `frictionless CLI` and `jq` just to get to the errors:
```
frictionless validate datapackage.json --type package --json | jq .tasks[].errors
```
Result:
```json
[
  {
    "cells": [
      "id",
      "first_name",
      "last_name",
      "dept_id"
    ],
    "rowNumber": 8,
    "rowPosition": 9,
    "code": "foreign-key-error",
    "name": "ForeignKey Error",
    "tags": [
      "#table",
      "#row"
    ],
    "note": "not found in the lookup table",
    "message": "Row at position \"9\" violates the foreign key: not found in the lookup table",
    "description": "Values in the foreign key fields should exist in the reference table"
  },
  {
    "cells": [
      "id",
      "first_name",
      "last_name",
      "dept_id"
    ],
    "rowNumber": 18,
    "rowPosition": 19,
    "code": "foreign-key-error",
    "name": "ForeignKey Error",
    "tags": [
      "#table",
      "#row"
    ],
    "note": "not found in the lookup table",
    "message": "Row at position \"19\" violates the foreign key: not found in the lookup table",
    "description": "Values in the foreign key fields should exist in the reference table"
  }
]
[]
```

Please replace this line with full information about your idea or problem. If it's a bug share as much as possible to reproduce it

---

Please preserve this line to notify @roll (lead of this repository)


Discussion:
roll: Thanks, @didiez, good catch!

Can you please add a simple test highlighting the bug? Also, are you interested in improving the error message at https://github.com/frictionlessdata/frictionless-py/blob/main/frictionless/resource.py#L978? We can do it in this PR to fully resolve #951
didiez: Sure, tests added :+1: 
However, while I was adding the `cells` attribute checking to the existing foreign key tests, I have noticed that the values found in the report for the `None` (empty) cells were the string `"None"`. Is there any reason to cast every cell to string?
Should we replace the current `cells=list(map(str, row.values()))` by `cells=row.cells` to preserve the cell types and the `None` values?
https://github.com/frictionlessdata/frictionless-py/blob/1b5a274bba5b736d1d7c4fc4a3eb62e64f56d1c0/frictionless/errors/row.py#L45
This also applies to `CellError.from_row(..)`
https://github.com/frictionlessdata/frictionless-py/blob/1b5a274bba5b736d1d7c4fc4a3eb62e64f56d1c0/frictionless/errors/cell.py#L78
roll: @didiez 
I would replace "None" by "". In general, the idea of cells being strings is that clients are able to show the row without handling data types but I need to review this approach. Maybe using `row.to_list(json=True)`... But I need to create a breaking release for changes like this
didiez: The `""` (empty) string sounds good to fix this without breaking things in a bugfix release. 
As of today, we didn't need cells with types in the report, but could be useful in some scenarios.
didiez: @roll Should we also fix the "None" from `CellError.from_row(..)` in this PR or better to open a new one?
roll: @didiez 
Using this one is fine I think. Can you please also add the change check for this to the tests?
roll: @didiez 
row.cells is needed to provide an original cells information. Sorry for being slightly out of the context. Can the fk bug be somewhere else?
didiez: > @didiez row.cells is needed to provide an original cells information. Sorry for being slightly out of the context.

I am not sure I'm following. Should we use `row.values()` or `row.cells` when creating the errors? The last revert was just to keep the old behaviour of CellError/RowError.from_row. However, in `fragments/row.py` the value passed to the error is `cells`.

> Can the fk bug be somewhere else?

I did a quick search and I think this bug (shows headers as row values) was only present for `RowError` when using `from_row` method (also in every subclass)


roll: @didiez 
I did a quick analysis and I think we need to use `cells=list(map(to_str, row.cells))` in the both `from_row` calls. The idea of the `errors.cells` is to provide original row cells while `row.values()` will return a cleaned version. Sorry for misleading you 
roll: So I think the problems we solve:
- https://github.com/frictionlessdata/frictionless-py/blob/main/frictionless/errors/row.py#L45
- https://github.com/frictionlessdata/frictionless-py/blob/main/frictionless/errors/cell.py#L78
- and inappropriate stringification for `None` (solved by your `to_str`)

Patch:
diff --git a/frictionless/errors/cell.py b/frictionless/errors/cell.py
index b6dad8f0..c8455e4d 100644
--- a/frictionless/errors/cell.py
+++ b/frictionless/errors/cell.py
@@ -73,9 +73,10 @@ class CellError(RowError):
             if field_name == name:
                 cell = row[field_name]
                 field_position = row.field_positions[field_number - 1]
+                to_str = lambda v: str(v) if v is not None else ""
                 return cls(
                     note=note,
-                    cells=list(map(str, row.values())),
+                    cells=list(map(to_str, row.cells)),
                     row_number=row.row_number,
                     row_position=row.row_position,
                     cell=str(cell),
diff --git a/frictionless/errors/row.py b/frictionless/errors/row.py
index b377d72c..cbb975f0 100644
--- a/frictionless/errors/row.py
+++ b/frictionless/errors/row.py
@@ -40,9 +40,10 @@ class RowError(TableError):
         Returns:
             RowError: error
         """
+        to_str = lambda v: str(v) if v is not None else ""
         return cls(
             note=note,
-            cells=list(map(str, row)),
+            cells=list(map(to_str, row.cells)),
             row_number=row.row_number,
             row_position=row.row_position,
         )
diff --git a/frictionless/row.py b/frictionless/row.py
index 4e5c64c4..ae2e6140 100644
--- a/frictionless/row.py
+++ b/frictionless/row.py
@@ -276,6 +276,7 @@ class Row(dict):
 
         # Prepare context
         cells = self.__cells
+        to_str = lambda v: str(v) if v is not None else ""
         fields = self.__field_info["objects"]
         field_mapping = self.__field_info["mapping"]
         field_positions = self.__field_info["positions"]
@@ -311,7 +312,7 @@ class Row(dict):
                 self.__errors.append(
                     errors.TypeError(
                         note=type_note,
-                        cells=list(map(str, cells)),
+                        cells=list(map(to_str, cells)),
                         row_number=self.__row_number,
                         row_position=self.__row_position,
                         cell=str(source),
@@ -327,7 +328,7 @@ class Row(dict):
                     self.__errors.append(
                         errors.ConstraintError(
                             note=note,
-                            cells=list(map(str, cells)),
+                            cells=list(map(to_str, cells)),
                             row_number=self.__row_number,
                             row_position=self.__row_position,
                             cell=str(source),
@@ -350,7 +351,7 @@ class Row(dict):
                 self.__errors.append(
                     errors.ExtraCellError(
                         note="",
-                        cells=list(map(str, cells)),
+                        cells=list(map(to_str, cells)),
                         row_number=self.__row_number,
                         row_position=self.__row_position,
                         cell=str(cell),
@@ -369,7 +370,7 @@ class Row(dict):
                     self.__errors.append(
                         errors.MissingCellError(
                             note="",
-                            cells=list(map(str, cells)),
+                            cells=list(map(to_str, cells)),
                             row_number=self.__row_number,
                             row_position=self.__row_position,
                             cell="",
@@ -385,7 +386,7 @@ class Row(dict):
             self.__errors = [
                 errors.BlankRowError(
                     note="",
-                    cells=list(map(str, cells)),
+                    cells=list(map(to_str, cells)),
                     row_number=self.__row_number,
                     row_position=self.__row_position,
                 )


Test patch:
diff --git a/tests/validate/test_package.py b/tests/validate/test_package.py
index 5aaae10b..b1c019b4 100644
--- a/tests/validate/test_package.py
+++ b/tests/validate/test_package.py
@@ -206,8 +206,8 @@ def test_validate_package_schema_foreign_key_self_referenced_resource_violation(
     descriptor = deepcopy(DESCRIPTOR_FK)
     del descriptor["resources"][0]["data"][4]
     report = validate(descriptor)
-    assert report.flatten(["rowPosition", "fieldPosition", "code"]) == [
-        [4, None, "foreign-key-error"],
+    assert report.flatten(["rowPosition", "fieldPosition", "code", "cells"]) == [
+        [4, None, "foreign-key-error", ["3", "rome", "4"]],
     ]
 
 
@@ -215,8 +215,8 @@ def test_validate_package_schema_foreign_key_internal_resource_violation():
     descriptor = deepcopy(DESCRIPTOR_FK)
     del descriptor["resources"][1]["data"][4]
     report = validate(descriptor)
-    assert report.flatten(["rowPosition", "fieldPosition", "code"]) == [
-        [5, None, "foreign-key-error"],
+    assert report.flatten(["rowPosition", "fieldPosition", "code", "cells"]) == [
+        [5, None, "foreign-key-error", ["4", "rio", ""]],
     ]
 
 
@@ -224,11 +224,11 @@ def test_validate_package_schema_foreign_key_internal_resource_violation_non_exi
     descriptor = deepcopy(DESCRIPTOR_FK)
     descriptor["resources"][1]["data"] = [["label", "population"], [10, 10]]
     report = validate(descriptor)
-    assert report.flatten(["rowPosition", "fieldPosition", "code"]) == [
-        [2, None, "foreign-key-error"],
-        [3, None, "foreign-key-error"],
-        [4, None, "foreign-key-error"],
-        [5, None, "foreign-key-error"],
+    assert report.flatten(["rowPosition", "fieldPosition", "code", "cells"]) == [
+        [2, None, "foreign-key-error", ["1", "london", "2"]],
+        [3, None, "foreign-key-error", ["2", "paris", "3"]],
+        [4, None, "foreign-key-error", ["3", "rome", "4"]],
+        [5, None, "foreign-key-error", ["4", "rio", ""]],
     ]
 
 
diff --git a/tests/validate/test_resource.py b/tests/validate/test_resource.py
index 918d5755..64eb0b90 100644
--- a/tests/validate/test_resource.py
+++ b/tests/validate/test_resource.py
@@ -587,8 +587,8 @@ def test_validate_schema_foreign_key_error_self_referencing_invalid():
         },
     }
     report = validate(source)
-    assert report.flatten(["rowPosition", "fieldPosition", "code"]) == [
-        [6, None, "foreign-key-error"],
+    assert report.flatten(["rowPosition", "fieldPosition", "code", "cells"]) == [
+        [6, None, "foreign-key-error", ["5", "6", "Rome"]],
     ]
 
 
@@ -609,6 +609,8 @@ def test_validate_schema_unique_error_and_type_error():
         ["a1", 100],
         ["a2", "bad"],
         ["a3", 100],
+        ["a4", 0],
+        ["a5", 0],
     ]
     schema = {
         "fields": [
@@ -617,9 +619,10 @@ def test_validate_schema_unique_error_and_type_error():
         ]
     }
     report = validate(source, schema=schema)
-    assert report.flatten(["rowPosition", "fieldPosition", "code"]) == [
-        [3, 2, "type-error"],
-        [4, 2, "unique-error"],
+    assert report.flatten(["rowPosition", "fieldPosition", "code", "cells"]) == [
+        [3, 2, "type-error", ["a2", "bad"]],
+        [4, 2, "unique-error", ["a3", "100"]],
+        [6, 2, "unique-error", ["a5", "0"]],
     ]
 
 
@@ -835,8 +838,8 @@ def test_validate_detector_headers_errors():
     }
     detector = Detector(schema_sync=True)
     report = validate(source, schema=schema, detector=detector)
-    assert report.flatten(["rowPosition", "fieldPosition", "code"]) == [
-        [4, 4, "constraint-error"],
+    assert report.flatten(["rowPosition", "fieldPosition", "code", "cells"]) == [
+        [4, 4, "constraint-error", ["3", "Smith", "Paul", ""]],
     ]