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.

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

Fix hasTable is undefined for schema postgres users #15702

Merged
merged 1 commit into from
Feb 6, 2023

Conversation

petersg83
Copy link
Contributor

What does it do?

Fix #15603

Why is it needed?

Users using postgres with a schema and having the double inverseBy bug (#14428) cannot start Strapi.

How to test it?

  1. Create a schema in your postgres DB:
# SQL
CREATE SCHEMA myschema;
  1. Set the schema in the database config ./config/database.js:
module.exports = ({ env }) => ({
  client: 'postgres',
  connection: {
    // ...
    schema: 'myschema',
    // ...
  },
});
  1. Create a many-many relation between 2 content-types through the content-type builder
  2. Identify the content-type schema where the previously created relation has the key mappedBy
  3. Modify this schema file by replacing the key mappedBy by inversedBy
  4. Restart the app
  5. See that there is no error anymore

Related issue(s)/PR(s)

#15603

@petersg83 petersg83 added source: core:database Source is core/database package pr: fix This PR is fixing a bug labels Feb 3, 2023
@petersg83 petersg83 added this to the 4.6.1 milestone Feb 3, 2023
@codecov
Copy link

codecov bot commented Feb 3, 2023

Codecov Report

Base: 59.11% // Head: 59.11% // No change to project coverage 👍

Coverage data is based on head (3707434) compared to base (7cb5e9e).
Patch coverage: 0.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #15702   +/-   ##
=======================================
  Coverage   59.11%   59.11%           
=======================================
  Files        1502     1502           
  Lines       38360    38360           
  Branches     7385     7385           
=======================================
  Hits        22677    22677           
  Misses      13411    13411           
  Partials     2272     2272           
Flag Coverage Δ
back 47.38% <0.00%> (ø)
front 67.15% <ø> (ø)
unit_back 47.38% <0.00%> (ø)
unit_front 67.15% <ø> (ø)

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

Impacted Files Coverage Δ
...atabase/lib/validations/relations/bidirectional.js 45.71% <0.00%> (ø)

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.

Copy link
Contributor

@Marc-Roig Marc-Roig left a comment

Choose a reason for hiding this comment

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

LGTM!

@@ -32,7 +32,7 @@ const getLinksWithoutMappedBy = (db) => {

const isLinkTableEmpty = async (db, linkTableName) => {
// If the table doesn't exist, it's empty
const exists = await db.getConnection().schema.hasTable(linkTableName);
const exists = await db.getSchemaConnection().hasTable(linkTableName);
Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot we had getSchemaConnection. Good catch!

Copy link
Contributor

Choose a reason for hiding this comment

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

Just a random question: why is this not catched by any test? :)

Copy link
Contributor

@Marc-Roig Marc-Roig Feb 3, 2023

Choose a reason for hiding this comment

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

That is a very good question and a thing we could discuss better.

This is caused by a custom configuration on postgres we didnt account for. For the default config is working ok.

We have faced multiple issues caused by db configs

GTID issues
Mariadb clusters
Postgress custom schemas

We could extend this list and periodically test for it, or see if there is a possibility to add it in the CI.

It is quite complicated to account for things like this in our current tests today.

Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting. I imagine it would be quite hard to run all tests against every combination of configuration. Do you see a way to create tests, e.g. to mimic a specific configuration? E.g. to mock something in a way to simulate such a configuration?

@petersg83 petersg83 merged commit dd1cfed into main Feb 6, 2023
@petersg83 petersg83 deleted the fix/warning-inversedBy branch February 6, 2023 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix This PR is fixing a bug source: core:database Source is core/database package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Upgrading 4.6.0 hasTable undefined error
3 participants