Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

introspection: merge index+relation introspection and reintrospection #3346

Merged
merged 1 commit into from
Nov 2, 2022
Merged

introspection: merge index+relation introspection and reintrospection #3346

merged 1 commit into from
Nov 2, 2022

Conversation

tomhoule
Copy link
Contributor

@tomhoule tomhoule commented Oct 27, 2022 •
edited

introspection: merge indexes+relations introspection and reintrospection

The path to multiSchema introspection we started on begins with a
refactoring aiming at merging introspection and reintrospection into one
step, and removing all DML usage from the introspection engine. This is
a part of it. See the following past PRs for more context:
#3338 and
#3333.

@tomhoule tomhoule force-pushed the ie/model-reintrospection branch 4 times, most recently from 6c60e3c to 90255a4 Compare November 1, 2022 13:37
@tomhoule tomhoule force-pushed the ie/model-reintrospection branch 3 times, most recently from 6642372 to bb84f7a Compare November 1, 2022 14:49
@tomhoule tomhoule added this to the 4.6.0 milestone Nov 1, 2022
@tomhoule tomhoule changed the title wip: multiSchema prep: models reintrospection introspection: merge indexes+relations introspection and reintrospection Nov 1, 2022
@tomhoule tomhoule changed the title introspection: merge indexes+relations introspection and reintrospection introspection: merge index+relation introspection and reintrospection Nov 1, 2022
@tomhoule tomhoule force-pushed the ie/model-reintrospection branch 5 times, most recently from 6dee8ca to b543507 Compare November 2, 2022 08:43
@tomhoule tomhoule marked this pull request as ready for review November 2, 2022 09:08
@tomhoule tomhoule requested a review from a team as a code owner November 2, 2022 09:08
tomhoule
tomhoule commented Nov 2, 2022
View reviewed changes
introspection-engine/connectors/sql-introspection-connector/src/introspection.rs
.filter(|table| is_prisma_1_point_1_or_2_join_table(*table) || is_prisma_1_point_0_join_table(*table))
.map(|table| table.name()[1..].to_string())
.collect();
fn introspect_models(datamodel: &mut Datamodel, ctx: &mut Context<'_>) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function should get its own module. I didn't do that to avoid a larger diff, but it is coming.

eviefp
eviefp suggested changes Nov 2, 2022
View reviewed changes
Copy link
Contributor

@eviefp eviefp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and I love we removed a lot of code. Left in a few comments, let's have a chat if you disagree with any of them.

introspection-engine/connectors/sql-introspection-connector/src/calculate_datamodel.rs Outdated Show resolved Hide resolved
introspection-engine/connectors/sql-introspection-connector/src/calculate_datamodel.rs Outdated Show resolved Hide resolved
introspection-engine/connectors/sql-introspection-connector/src/calculate_datamodel.rs Outdated Show resolved Hide resolved
introspection-engine/connectors/sql-introspection-connector/src/introspection.rs Outdated Show resolved Hide resolved
...spection-engine/connectors/sql-introspection-connector/src/introspection/inline_relations.rs
use sql_schema_describer as sql;
use std::collections::{HashMap, HashSet};

pub(super) fn introspect_inline_relations(datamodel: &mut psl::dml::Datamodel, ctx: &mut super::Context<'_>) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is quite verbose. I think it would be nice to split, but if that's impractical, perhaps an overview comment here explaining what's going on in this function might be enough.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wrote a doc comment. Beyond porting the existing code, we'll probably want to do something a bit more principled with relation and field names at some point, as that is what causes a lot of the verbosity in this function.

...spection-engine/connectors/sql-introspection-connector/src/introspection/inline_relations.rs Show resolved Hide resolved
...spection-engine/connectors/sql-introspection-connector/src/introspection/inline_relations.rs Outdated Show resolved Hide resolved
introspection-engine/introspection-engine-tests/tests/re_introspection/mod.rs Show resolved Hide resolved
eviefp
eviefp approved these changes Nov 2, 2022
View reviewed changes
Copy link
Contributor

@eviefp eviefp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@tomhoule
introspection: merge index+relation introspection and reintrospection
bffd935 
The path to multiSchema introspection we started on begins with a
refactoring aiming at merging introspection and reintrospection into one
step, and removing all DML usage from the introspection engine. This is
a part of it. See the following past PRs for more context:
#3338 and
#3333.
@tomhoule tomhoule merged commit 6719e58 into main Nov 2, 2022
@tomhoule tomhoule deleted the ie/model-reintrospection branch November 2, 2022 14:44
tomhoule added a commit that referenced this pull request Nov 10, 2022
@tomhoule
Relation (re)introspection and name sanitization
05b2bac 
Changes:

- Prisma 1 join tables did not get an `@relation("...")` attribute with
  an explicit name if they were not ambiguous, before this PR. This
  means migrate and client would have missed them.
- The criteria for considering a relation ambiguous got more agressive.
  Every relation between two models that have more than one relation
  between them is now considered ambiguous and gets named accordingly.

This should fix the relation introspection regressions introduced in #3346

closes https://github.com/prisma/schema-team/issues/354
tomhoule added a commit that referenced this pull request Nov 10, 2022
@tomhoule
Relation (re)introspection and name sanitization
5cff27a 
Changes:

- Prisma 1 join tables did not get an `@relation("...")` attribute with
  an explicit name if they were not ambiguous, before this PR. This
  means migrate and client would have missed them.
- The criteria for considering a relation ambiguous got more agressive.
  Every relation between two models that have more than one relation
  between them is now considered ambiguous and gets named accordingly.

This should fix the relation introspection regressions introduced in #3346

closes https://github.com/prisma/schema-team/issues/354
tomhoule added a commit that referenced this pull request Nov 10, 2022
@tomhoule
Relation (re)introspection and name sanitization
a55ee36 
Changes:

- Prisma 1 join tables did not get an `@relation("...")` attribute with
  an explicit name if they were not ambiguous, before this PR. This
  means migrate and client would have missed them.
- The criteria for considering a relation ambiguous got more agressive.
  Every relation between two models that have more than one relation
  between them is now considered ambiguous and gets named accordingly.

This should fix the relation introspection regressions introduced in #3346

closes https://github.com/prisma/schema-team/issues/354
tomhoule added a commit that referenced this pull request Nov 11, 2022
@tomhoule
Relation (re)introspection and name sanitization
7b6152d 
Changes:

- Prisma 1 join tables did not get an `@relation("...")` attribute with
  an explicit name if they were not ambiguous, before this PR. This
  means migrate and client would have missed them.
- The criteria for considering a relation ambiguous got more agressive.
  Every relation between two models that have more than one relation
  between them is now considered ambiguous and gets named accordingly.

This should fix the relation introspection regressions introduced in #3346

closes https://github.com/prisma/schema-team/issues/354
tomhoule added a commit that referenced this pull request Nov 11, 2022
@tomhoule
Relation (re)introspection and name sanitization
43ad36e 
Changes:

- Prisma 1 join tables did not get an `@relation("...")` attribute with
  an explicit name if they were not ambiguous, before this PR. This
  means migrate and client would have missed them.
- The criteria for considering a relation ambiguous got more agressive.
  Every relation between two models that have more than one relation
  between them is now considered ambiguous and gets named accordingly.

This should fix the relation introspection regressions introduced in #3346

closes https://github.com/prisma/schema-team/issues/354
tomhoule added a commit that referenced this pull request Nov 15, 2022
@tomhoule
Relation (re)introspection and name sanitization (#3381)
59141a4 
Changes:

- Prisma 1 join tables did not get an `@relation("...")` attribute with
  an explicit name if they were not ambiguous, before this PR. This
  means migrate and client would have missed them.
- The criteria for considering a relation ambiguous got more agressive.
  Every relation between two models that have more than one relation
  between them is now considered ambiguous and gets named accordingly.

This should fix the relation introspection regressions introduced in #3346

closes https://github.com/prisma/schema-team/issues/354
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eviefp eviefp eviefp approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
4.6.0
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@tomhoule @eviefp