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

Use correct charset/collation from column/table defaults when creating relationships #9636

Merged
merged 1 commit into from
Apr 10, 2022

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Apr 8, 2022

Fixes #6823

When you have entities and columns with different charset/collation the SchemaTool should be smart and use the correct charset/collection for the column that references the other table.

In the tests I created the following entities:

User table Relation Default charset utf8mb4 Default collation utf8mb4_bin
id default default
group_id References Group table ascii ascii_general_ci
status_id References Status table latin1 latin1_bin

👉 As you can see, the group_id column uses the charset/collation from the group table default.
For the status_id, it uses the charset/collation from the id column because that was set explicitly.

Group table _Default charset ascii _Default collation ascii_general_ci
id default default

👉 As you can see, the id column of group uses the table default.

Status table _Default charset koi8r _Default collation koi8r_bin
id latin1 latin1_bin

👉 As you can see, the id column of status is different than the table default.

When I disable the fix in SchemaTool, the following errors would be produced:

// ALTER TABLE user ADD CONSTRAINT FK_8D93D649FE54D947 FOREIGN KEY (group_id) REFERENCES `group` (id)

An exception occurred while executing a query: SQLSTATE[HY000]: General error: 3780 Referencing column 'group_id' and referenced column 'id' in foreign key constraint 'FK_8D93D649FE54D947' are incompatible.

@ruudk ruudk requested a review from greg0ire April 9, 2022 10:10
@ruudk ruudk changed the base branch from 2.11.x to 2.12.x April 9, 2022 12:45
@ruudk ruudk force-pushed the failing-test branch 7 times, most recently from da30236 to 5b527ec Compare April 10, 2022 11:25
@ruudk ruudk requested a review from greg0ire April 10, 2022 11:35
@ruudk
Copy link
Contributor Author

ruudk commented Apr 10, 2022

@greg0ire Ok, the PR is green again 🎉

@ruudk ruudk force-pushed the failing-test branch 2 times, most recently from cdb8a92 to a822fb2 Compare April 10, 2022 11:41
@greg0ire greg0ire requested a review from derrabus April 10, 2022 11:42
@derrabus derrabus added the Bug label Apr 10, 2022
@derrabus derrabus merged commit 03f4468 into doctrine:2.12.x Apr 10, 2022
@derrabus derrabus added this to the 2.12.0 milestone Apr 10, 2022
@greg0ire
Copy link
Member

Thanks @ruudk !

@ruudk ruudk deleted the failing-test branch April 10, 2022 15:36
derrabus added a commit to derrabus/orm that referenced this pull request Apr 11, 2022
* 2.12.x:
  Leverage generic persistence event classes (doctrine#9633)
  Fix static analysis for Persistence 2.5 (doctrine#9648)
  Improve exception message (doctrine#9646)
  Deprecate console helper (doctrine#9641)
  Use charset/collation from column or table default when creating relations (doctrine#9636)
  Support Enum IDs and search by Enum fields (doctrine#9629)
  Fix composer install in contributing readme
@ruudk
Copy link
Contributor Author

ruudk commented Apr 11, 2022

@greg0ire @derrabus ⚠️ Heads up: I'm currently testing 2.12.x in my project and noticed that it produced invalid SQL. So I think we should not tag 2.12.0 yet, before we fix this.

The problem is as follows:
When the JoinColumn is an integer, it should never include charset/collation on the column, as integer columns don't require this.

For example, this SQL was produced:

CREATE TABLE table_name (
  // ..
  user_id INT CHARACTER SET utf8 NOT NULL COLLATE `utf8_unicode_ci`
  // ..
)

resulting in this error:

Query 1 ERROR: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'CHARACTER SET utf8 NOT NULL COLLATE `utf8_unicode_ci`, type VARCHAR(255) NOT NUL' at line 1

@ruudk
Copy link
Contributor Author

ruudk commented Apr 11, 2022

I think the solution can be simple, never include CHARACTER SET x on the columns. Because this works:

CREATE TABLE table_name (
  // ..
  user_id INT NOT NULL COLLATE `utf8_unicode_ci`,
  string_user_id VARCHAR(255) NOT NULL COLLATE `utf8_unicode_ci`,
  // ..
)

ruudk added a commit to ruudk/doctrine2 that referenced this pull request Apr 11, 2022
The fix introduced in doctrine#9636 now breaks when working with integer relations.

Putting `charset` and `collation` on a text join column works fine, but when the
column is an integer, MySQL won't accept it:

```sql
CREATE TABLE table_name (
  // ..
  user_id INT CHARACTER SET utf8 NOT NULL COLLATE `utf8_unicode_ci`
  // ..
)
```

produces this error:
```
Query 1 ERROR: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'CHARACTER SET utf8 NOT NULL COLLATE `utf8_unicode_ci`, type VARCHAR(255) NOT NUL' at line 1
```

Luckily the solution is simple. The charset is not needed as MySQL automatically derives that from
the `collation` when specified. It's also clever enough to ignore `collation` when the type is an integer.

Therefore the following works fine:
```sql
CREATE TABLE table_name (
  // ..
  user_id INT NOT NULL COLLATE `utf8_unicode_ci`,
  string_user_id VARCHAR(255) NOT NULL COLLATE `utf8_unicode_ci`,
  // ..
)
```
@ruudk
Copy link
Contributor Author

ruudk commented Apr 11, 2022

This should fix the problem: #9652

greg0ire added a commit to greg0ire/doctrine-orm that referenced this pull request Apr 11, 2022
…ing relations (doctrine#9636)"

This reverts commit 03f4468.
The inferring process seems fragile and MySQL-specific. The ORM might
not be the correct place to fix this issue (if it needs to be fixed at
all).
greg0ire added a commit that referenced this pull request Apr 12, 2022
Revert "Use charset/collation from column or table default when creatng relations (#9636)"
@greg0ire greg0ire removed this from the 2.12.0 milestone Apr 12, 2022
derrabus added a commit to derrabus/orm that referenced this pull request Apr 15, 2022
* 2.12.x:
  Indicate support for doctrine/persistence 3 (doctrine#9656)
  Fix tests for enum ID hydration (doctrine#9658)
  Revert "Use charset/collation from column or table default when creating relations (doctrine#9636)"
  Fix test file/class names (doctrine#9649)
derrabus added a commit to derrabus/orm that referenced this pull request Apr 15, 2022
* 2.12.x:
  Indicate support for doctrine/persistence 3 (doctrine#9656)
  Fix tests for enum ID hydration (doctrine#9658)
  Revert "Use charset/collation from column or table default when creating relations (doctrine#9636)"
  Fix test file/class names (doctrine#9649)
derrabus added a commit to derrabus/orm that referenced this pull request Apr 15, 2022
* 2.12.x:
  Indicate support for doctrine/persistence 3 (doctrine#9656)
  Fix tests for enum ID hydration (doctrine#9658)
  Revert "Use charset/collation from column or table default when creating relations (doctrine#9636)"
  Fix test file/class names (doctrine#9649)
derrabus added a commit to derrabus/orm that referenced this pull request Apr 15, 2022
* 2.12.x:
  Indicate support for doctrine/persistence 3 (doctrine#9656)
  Fix tests for enum ID hydration (doctrine#9658)
  Revert "Use charset/collation from column or table default when creating relations (doctrine#9636)"
  Fix test file/class names (doctrine#9649)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Joined columns not created with matching custom schema options
3 participants