fix(schema-engine): recovered "chosen data" / "original data" formatting in introspection #3924

jkomyno · 2023-04-26T11:25:24Z

This PR:

normalizes name of entries in introspection warnings
organizes introspection warnings that can result in long repetitions of different field values for the same fixed key into "groups". This results in something like: Model: "person", field(s): ["0", "1", ..., "100"]
adds a mysql test introspection_tests::views::mysql::invalid_field_names_trigger_warnings inspired by prisma/introspection-ci

See Slack thread.

…ting in introspection

codspeed-hq · 2023-04-26T11:36:43Z

CodSpeed Performance Report

Merging #3924 fix/introspection-formatting-and-grouping (075b453) will not alter performances.

Summary

🔥 0 improvements
❌ 0 regressions
✅ 6 untouched benchmarks

🆕 0 new benchmarks
⁉️ 0 dropped benchmarks

…rnings with several repetitions in a group

jkomyno · 2023-04-26T15:31:23Z

schema-engine/connectors/schema-connector/src/warnings.rs

+        write!(f, r#"Model: "{}""#, &model)?;
+        write!(f, ", field(s): [")?;
+
+        let (last, vec_but_last) = vec.split_last().unwrap();


.unwrap() is safe, as vec is at least a singleton

Iterator::peekable() is a good way to do this that without unwraps/panics, but I really don't mind doing it this way either.

I haven't quite understood how we could apply Iterator::peekable() in this scenario, but I'm happy to see that in action (if not for this PR, maybe for future ones)

There's an example here:

prisma-engines/schema-engine/connectors/sql-schema-connector/src/introspection_map/relation_names.rs

Line 154 in e9d4b38

let mut cols = fk.constrained_columns().peekable();

no need to change here though

schema-engine/connectors/schema-connector/src/warnings.rs

Jolg42

Note: I skipped the Rust code

The output looks good!

tomhoule

Functionally this looks good, but introducing two new traits for this is more complexity than is warranted. I'm open to discussion (I may have missed something) / pairing.

tomhoule · 2023-04-27T08:52:53Z

schema-engine/connectors/schema-connector/src/warnings.rs

+
+    /// Formats the grouped items in a compact, human-readable way.
+    fn fmt_group(&self, f: &mut fmt::Formatter<'_>, key: &K, value: &[&V]) -> fmt::Result;
+}


I think this could be an impl Display for GroupBy<ModelAndField> { ... } (etc.) instead of a new trait. The formatting done in render_warnings_grouped could be a helper function.

Thanks, I have implemented this.

tomhoule · 2023-04-27T08:53:46Z

schema-engine/connectors/schema-connector/src/warnings.rs

    /// Fields having an empty name.
-    pub fields_with_empty_names_in_type: Vec<TypeAndField>,
+    pub fields_with_empty_names_in_type: GroupBy<TypeAndField>,


These can stay Vec<ModelAndField> and we don't need the new WriteOnlyVec trait if we go with the previous suggestion (we'd just wrap in GroupBy at the last moment in render_warnings_grouped).

Thank you, I have applied this.

tomhoule · 2023-04-27T08:56:03Z

schema-engine/connectors/schema-connector/src/warnings.rs

+        write!(f, r#"Model: "{}""#, &model)?;
+        write!(f, ", field(s): [")?;
+
+        let (last, vec_but_last) = vec.split_last().unwrap();


Iterator::peekable() is a good way to do this that without unwraps/panics, but I really don't mind doing it this way either.

…gs, removed WriteOnlyVec trait

tomhoule · 2023-04-27T14:42:50Z

schema-engine/connectors/schema-connector/src/warnings.rs

+
+        result
+    }
+}


I meant we could remove the whole trait, and replace the lines that just call group_by() with something like let grouped: BTreeMap<_, _> = self.iter().map(|mf| (&mf.model, mf)).collect();

Druue · 2023-05-01T10:51:05Z

schema-engine/sql-introspection-tests/tests/views/mysql.rs

+    let expected = expect![[r#"
+        *** WARNING ***
+
+        These fields were commented out because their names are currently not supported by Prisma. Please provide valid ones that match [a-zA-Z][a-zA-Z0-9_]* using the `@map` attribute:


Is this something we want to separate out? rather than having one heading for commented out fields and then ordering / sorting them from there, so e.g. still keeping the two models, and then the two views etc

Druue · 2023-05-01T10:59:34Z

schema-engine/connectors/schema-connector/src/warnings.rs

@@ -134,19 +276,19 @@ impl fmt::Display for Warnings {
            f,
        )?;

-        render_warnings(
+        render_warnings_grouped(


It's not immediately clear to me how the scaffolding for render_warnings_grouped works. I.e. the workflow for adding new warnings - or editing previous warnings - that we want grouped. The change that @tomhoule called out does seem a lot simpler to me

Druue · 2023-05-01T11:00:59Z

schema-engine/connectors/sql-schema-connector/src/warnings/view.rs

@@ -1,5 +1,8 @@
 use crate::introspection_pair::ViewPair;
-use schema_connector::{warnings as generators, Warnings};
+use schema_connector::{


a little confused by this change, does this have any meaning outside semantics? and on that, what are the semantic benefits here

fix(schema-engine): recovered "chosen data" / "original data" formatt…

1932f3d

…ting in introspection

jkomyno and others added 5 commits April 26, 2023 14:17

test(schema-engine): update snapshots for mongodb

a1350f5

feat(schema-engine): add "Groupable<K, V>" trait for introspection wa…

0b41e4c

…rnings with several repetitions in a group

test(schema-engine): update test snapshots

ff3e4bb

Merge branch 'main' into fix/introspection-formatting-and-grouping

82c0123

chore: minor fixes

e9f5102

jkomyno commented Apr 26, 2023

View reviewed changes

schema-engine/connectors/schema-connector/src/warnings.rs Outdated Show resolved Hide resolved

jkomyno added this to the 4.14.0 milestone Apr 26, 2023

chore: minor fixes

7317511

Jolg42 approved these changes Apr 26, 2023

View reviewed changes

tomhoule reviewed Apr 27, 2023

View reviewed changes

Jolg42 mentioned this pull request Apr 27, 2023

Introspection warnings: move rendering from DbPull.ts to the engine prisma/prisma#17582

Closed

jkomyno added 3 commits April 27, 2023 14:10

Merge branch 'main' into fix/introspection-formatting-and-grouping

80f7956

feat(schema-engine): simplify traits for grouped introspection warnin…

84c1261

…gs, removed WriteOnlyVec trait

feat(schema-engine): add Display trait for GroupBy<T>

075b453

jkomyno marked this pull request as ready for review April 27, 2023 13:56

jkomyno requested a review from a team as a code owner April 27, 2023 13:56

janpio requested a review from tomhoule April 27, 2023 14:16

tomhoule reviewed Apr 27, 2023

View reviewed changes

tomhoule approved these changes Apr 27, 2023

View reviewed changes

jkomyno merged commit 15a4f29 into main Apr 27, 2023
52 checks passed

jkomyno deleted the fix/introspection-formatting-and-grouping branch April 27, 2023 15:37

tomhoule mentioned this pull request Apr 28, 2023

se: simplify rendering of introspection warnings #3934

Merged

Druue reviewed May 1, 2023

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(schema-engine): recovered "chosen data" / "original data" formatting in introspection #3924

fix(schema-engine): recovered "chosen data" / "original data" formatting in introspection #3924

jkomyno commented Apr 26, 2023 •

edited

codspeed-hq bot commented Apr 26, 2023 •

edited

jkomyno Apr 26, 2023

tomhoule Apr 27, 2023

jkomyno Apr 27, 2023

tomhoule Apr 27, 2023

Jolg42 left a comment

tomhoule left a comment

tomhoule Apr 27, 2023

jkomyno Apr 27, 2023

tomhoule Apr 27, 2023

jkomyno Apr 27, 2023

tomhoule Apr 27, 2023

tomhoule Apr 27, 2023

Druue May 1, 2023

Druue May 1, 2023

Druue May 1, 2023

fix(schema-engine): recovered "chosen data" / "original data" formatting in introspection #3924

fix(schema-engine): recovered "chosen data" / "original data" formatting in introspection #3924

Conversation

jkomyno commented Apr 26, 2023 • edited

codspeed-hq bot commented Apr 26, 2023 • edited

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Jolg42 left a comment

Choose a reason for hiding this comment

tomhoule left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkomyno commented Apr 26, 2023 •

edited

codspeed-hq bot commented Apr 26, 2023 •

edited