(fix) Warn for duplicated inverseBy relationships #15217

Marc-Roig · 2022-12-20T09:25:35Z

What does it do?

Warn users to fix bidirectional relations where both sides use inversedBy.

Why is it needed?

There was this bug resolved, which was placing inversedBy in both sides of relations. Which resulted in creating two db tables for each relation.

This brought this problem for some users #14912.

Switching one of the sides to mapped by fixes the issue, but if you switch the wrong side the join table with data would be removed.

How to test it?

Create Cat content type:

{
  "kind": "collectionType",
  "collectionName": "cats",
  "info": {
    "singularName": "cat",
    "pluralName": "cats",
    "displayName": "Cat"
  },
  "options": {
    "draftAndPublish": true
  },
  "pluginOptions": {},
  "attributes": {
    "dogs": {
      "type": "relation",
      "relation": "manyToMany",
      "target": "api::dog.dog",
      "inversedBy": "cats"
    },
    "name": {
      "type": "string"
    }
  }
}

Create Dog content type:

{
  "kind": "collectionType",
  "collectionName": "dogs",
  "info": {
    "singularName": "dog",
    "pluralName": "dogs",
    "displayName": "Dog"
  },
  "options": {
    "draftAndPublish": true
  },
  "pluginOptions": {},
  "attributes": {
    "name": {
      "type": "string"
    },
    "cats": {
      "type": "relation",
      "relation": "manyToMany",
      "target": "api::cat.cat",
      "inversedBy": "dogs"
    }
  }
}

This should bring an error saying you should change one of the sides inversedBy.

To test it properly, you could try to add data in that relation and switching to this branch, to see if makes you switch the inversedBy wich removes the empty join table.

Related issue(s)/PR(s)

#14912

codecov · 2022-12-20T09:32:46Z

Codecov Report

Base: 61.25% // Head: 65.96% // Increases project coverage by +4.71% 🎉

Coverage data is based on head (d2636e5) compared to base (f5e87de).
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15217      +/-   ##
==========================================
+ Coverage   61.25%   65.96%   +4.71%     
==========================================
  Files        1353     1056     -297     
  Lines       33435    22981   -10454     
  Branches     6438     4120    -2318     
==========================================
- Hits        20481    15160    -5321     
+ Misses      11129     6889    -4240     
+ Partials     1825      932     -893

Flag	Coverage Δ
back	`?`
front	`65.96% <ø> (-0.05%)`	⬇️
unit_back	`?`
unit_front	`65.96% <ø> (-0.05%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
packages/core/admin/admin/src/hooks/index.js	`100.00% <0.00%> (ø)`
packages/core/admin/admin/src/pages/App/index.js	`66.66% <0.00%> (ø)`
packages/core/database/lib/index.js
packages/core/database/lib/metadata/relations.js
packages/core/utils/lib/print-value.js
packages/core/permissions/lib/index.js
packages/core/database/lib/query/index.js
.../plugins/i18n/server/content-types/locale/index.js
packages/core/utils/lib/template-configuration.js
packages/plugins/i18n/server/validation/locales.js
... and 289 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

Marc-Roig · 2023-01-02T11:03:36Z

packages/core/database/lib/validations/relations/bidirectional.js

+            ` instead of inversedBy in the relation "${invRelation.inversedBy}".`
+        );
+      } else {
+        // Both sides have data in the join table


I would like to discuss what should we do if this happens. It should be extremely rare and would only happen if useres are tweaking with the tables manually.

Why don't we have both directions of the relationship in the relationship table? 🤔

Bad wording sorry, that comment should be:
Both sides of the relation have a different join table, and both have data

Only one join table should exist, which would contain all necessary info of the relation.

In the other two cases, only one of the join tables holds data, so it's safe to remove the other. But in this case both have data, which leaves the question of what do we communicate to the user.

Marc-Roig · 2023-01-02T11:04:50Z

packages/core/database/lib/validations/relations/bidirectional.js

+        errorList.push(
+          `Error on attribute "${relation.inversedBy}" in model "${contentType.tableName}"(${contentType.uid}):` +
+            ` One of the sides of the relationship must be the owning side. You should use mappedBy` +
+            ` instead of inversedBy in the relation "${relation.inversedBy}".`
+        );


Pending for a discussion.

Should this be an error?
It could be a little bit intrusive for the users updating to the new version , when everything was working for them before, and now getting this error.

What do you think to make this a warning?

Do the relations work if they didn't do this correctly?
If so, I guess it should be a warning.

I prefer warning too. At the same time we can add some sense of urgency in the message:

"Please modify your schema by renaming the key inversedBy by mappedBy. Ex: { inverseBy: 'api::dog.dog' } -> { mappedBy: 'api::dog.dog' }"

Changed error to a warning and also the message inside, let me know what you think. @petersg83
Also, do we have a better way to log a warning than process.emitWarning? I don't feel it's quite visible when starting the application.

I do this way as apparently it's the official way and allows some features (choosing what to log depending on server config etc.). I would say it's enough, no?

alright :) asking just in case

petersg83

Nice job, I find the code structure clean!
I have put some comments.

packages/core/database/lib/validations/relations/bidirectional.js

petersg83 · 2023-01-09T17:52:03Z

packages/core/database/lib/validations/relations/bidirectional.js

+      const linkTableEmpty = await isLinkTableEmpty(db, joinTableName);
+      const inverseLinkTableEmpty = await isLinkTableEmpty(db, inverseJoinTableName);
+
+      if (linkTableEmpty) {


I'm wondering if it's not the opposite? Based on what the error message says I would put if (inverseLinkTableEmpty) here and else if (linkTableEmpty) {.
(or exchange the error message)

I just tried and I think it's just the message that as to be rewritten. I did a relation between country and categories and the error message is :
Error on attribute "categories" in model "categories"(api::category.category): One of the sides of the relationship must be the owning side. You should use mappedBy instead of inversedBy in the relation "categories".
It doesn't mention country at all and it feels it should :p

You are right, rephrased it , let me know if it sounds better now.

packages/core/database/lib/validations/relations/bidirectional.js

petersg83 · 2023-01-09T17:58:11Z

packages/core/database/lib/validations/relations/bidirectional.js

+        errorList.push(
+          `Error on attribute "${relation.inversedBy}" in model "${contentType.tableName}"(${contentType.uid}):` +
+            ` One of the sides of the relationship must be the owning side. You should use mappedBy` +
+            ` instead of inversedBy in the relation "${relation.inversedBy}".`
+        );


I prefer warning too. At the same time we can add some sense of urgency in the message:

"Please modify your schema by renaming the key inversedBy by mappedBy. Ex: { inverseBy: 'api::dog.dog' } -> { mappedBy: 'api::dog.dog' }"

petersg83

It works very well! It's awesome 😊 I've put a suggestion for the message. What do you think?

petersg83 · 2023-01-18T13:26:57Z

packages/core/database/lib/validations/relations/bidirectional.js

+      // Its safe to delete the inverse join table
+      process.emitWarning(
+        `Error on attribute "${relation.inversedBy}" in model "${invContentType.singularName}" (${invContentType.uid}).` +
+          ` Please modify your schema by renaming the key inversedBy by mappedBy.` +


Suggested change

` Please modify your schema by renaming the key inversedBy by mappedBy.` +

` Please modify your ${invContentType.singularName} schema by renaming the key "inversedBy" by "mappedBy".` +

(Same for the other message)

petersg83

LGTM! Well done :)

look for duplicated inverseBy relationships

e273f7f

Marc-Roig requested a review from petersg83 December 20, 2022 09:25

Marc-Roig changed the title ~~look for duplicated inverseBy relationships~~ (fix) Warn for duplicated inverseBy relationships Dec 20, 2022

remove unnecessary comments

189a67b

Marc-Roig marked this pull request as ready for review January 2, 2023 10:28

add validation folder in database

74c6ec1

Marc-Roig requested a review from nathan-pichon January 2, 2023 11:02

Marc-Roig commented Jan 2, 2023

View reviewed changes

Marc-Roig added source: core:database Source is core/database package pr: fix This PR is fixing a bug labels Jan 2, 2023

petersg83 suggested changes Jan 9, 2023

View reviewed changes

change error to warning

37ff580

Marc-Roig requested a review from petersg83 January 11, 2023 11:00

Marc-Roig added 3 commits January 11, 2023 13:55

change attribute name displayed in warning

1ef6230

Merge branch 'main' into fix/duplicate-join-tables

cf231a2

Merge branch 'main' into fix/duplicate-join-tables

167aba4

petersg83 suggested changes Jan 18, 2023

View reviewed changes

specify content type schema name

003b009

Marc-Roig requested a review from petersg83 January 18, 2023 15:54

Marc-Roig added 3 commits January 18, 2023 17:11

add quotes in inversedBy and mappedBy

52b780b

fix typo

bc53362

fix typo

d2636e5

petersg83 approved these changes Jan 18, 2023

View reviewed changes

Marc-Roig merged commit 07f5c0a into main Jan 23, 2023

Marc-Roig deleted the fix/duplicate-join-tables branch January 23, 2023 08:48

Marc-Roig added this to the 4.6.0 milestone Jan 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

(fix) Warn for duplicated inverseBy relationships #15217

(fix) Warn for duplicated inverseBy relationships #15217

Marc-Roig commented Dec 20, 2022 •

edited

codecov bot commented Dec 20, 2022 •

edited

Marc-Roig Jan 2, 2023

nathan-pichon Jan 3, 2023

Marc-Roig Jan 3, 2023 •

edited

Marc-Roig Jan 2, 2023

nathan-pichon Jan 5, 2023

petersg83 Jan 9, 2023

Marc-Roig Jan 11, 2023

petersg83 Jan 16, 2023

Marc-Roig Jan 17, 2023

petersg83 left a comment •

edited

petersg83 Jan 9, 2023

petersg83 Jan 9, 2023

Marc-Roig Jan 11, 2023

petersg83 Jan 9, 2023

petersg83 left a comment

petersg83 Jan 18, 2023

petersg83 Jan 18, 2023

Marc-Roig Jan 18, 2023

petersg83 left a comment

	` Please modify your schema by renaming the key inversedBy by mappedBy.` +
	` Please modify your ${invContentType.singularName} schema by renaming the key "inversedBy" by "mappedBy".` +

(fix) Warn for duplicated inverseBy relationships #15217

(fix) Warn for duplicated inverseBy relationships #15217

Conversation

Marc-Roig commented Dec 20, 2022 • edited

What does it do?

Why is it needed?

How to test it?

Related issue(s)/PR(s)

codecov bot commented Dec 20, 2022 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marc-Roig Jan 3, 2023 • edited

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

petersg83 left a comment • edited

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

petersg83 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

petersg83 left a comment

Choose a reason for hiding this comment

Marc-Roig commented Dec 20, 2022 •

edited

codecov bot commented Dec 20, 2022 •

edited

Marc-Roig Jan 3, 2023 •

edited

petersg83 left a comment •

edited