Open kiler129 opened 6 years ago
I think it makes sense. If you're going to file a pull request, please make sure there's a functional test which creates and uses a JSON column. This way, we'll make sure that all platforms which declare JSON as supported actually support it.
with
doctrine/dbal v2.7.1
on
Ver 10.3.8-MariaDB-log for Linux on x86_64 (Source distribution)
I'm seeing similar issues to those previously reported (& solved) for MDB 10.2x here
https://github.com/doctrine/dbal/pull/2825
Particularly, the 'infinite schema updates' and failing
doctrine schema:validate
after a (reported) 'successful'
doctrine schema:update
Other than the common issue of 'MDB 10.3 support', not clear that this is the right place for this.
If so, and it's useful, I can add the details here on request.
Atm, I've no specific functional test, other than a new DB init on a symfony4 project install.
@hal869 please provide the details.
@morozov
On a new SF4 + phpcr project, after package installs,
starting with new/clean DBs
a phpcr DB init reports successful completion
bin/console doctrine:phpcr:init:dbal --force
Jackalope Doctrine DBAL tables have been initialized successfully.
where
then a general migration diff
bin/console doctrine:migrations:diff
Generated new migration class to "/srv/www/test1/src/Migrations/Version20180704234248.php" from schema differences.
and the migrate
bin/console doctrine:migrations:migrate --no-interaction
Application Migrations
Migrating up to 20180704234248 from 0
++ migrating 20180704234248
-> CREATE TABLE media__gallery_media (id INT AUTO_INCREMENT NOT NULL, position INT NOT NULL, enabled TINYINT(1) NOT NULL, updated_at DATETIME NOT NULL, created_at DATETIME NOT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ENGINE = InnoDB
-> CREATE TABLE media__media (id INT AUTO_INCREMENT NOT NULL, name VARCHAR(255) NOT NULL, description TEXT DEFAULT NULL, enabled TINYINT(1) NOT NULL, provider_name VARCHAR(255) NOT NULL, provider_status INT NOT NULL, provider_reference VARCHAR(255) NOT NULL, provider_metadata LONGTEXT DEFAULT NULL COMMENT '(DC2Type:json)', width INT DEFAULT NULL, height INT DEFAULT NULL, length NUMERIC(10, 0) DEFAULT NULL, content_type VARCHAR(255) DEFAULT NULL, content_size INT DEFAULT NULL, copyright VARCHAR(255) DEFAULT NULL, author_name VARCHAR(255) DEFAULT NULL, context VARCHAR(64) DEFAULT NULL, cdn_is_flushable TINYINT(1) DEFAULT NULL, cdn_flush_identifier VARCHAR(64) DEFAULT NULL, cdn_flush_at DATETIME DEFAULT NULL, cdn_status INT DEFAULT NULL, updated_at DATETIME NOT NULL, created_at DATETIME NOT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ENGINE = InnoDB
-> CREATE TABLE media__gallery (id INT AUTO_INCREMENT NOT NULL, name VARCHAR(255) NOT NULL, context VARCHAR(64) NOT NULL, default_format VARCHAR(255) NOT NULL, enabled TINYINT(1) NOT NULL, updated_at DATETIME NOT NULL, created_at DATETIME NOT NULL, PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ENGINE = InnoDB
-> CREATE TABLE role (name VARCHAR(255) NOT NULL, PRIMARY KEY(name)) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ENGINE = InnoDB
-> CREATE TABLE user_role (user_id INT NOT NULL COMMENT '(DC2Type:msgphp_user_id)', role_name VARCHAR(255) NOT NULL, INDEX IDX_2DE8C6A3A76ED395 (user_id), INDEX IDX_2DE8C6A3E09C0C92 (role_name), PRIMARY KEY(user_id, role_name)) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ENGINE = InnoDB
-> CREATE TABLE user (id INT AUTO_INCREMENT NOT NULL COMMENT '(DC2Type:msgphp_user_id)', credential_email VARCHAR(255) NOT NULL, credential_password VARCHAR(255) NOT NULL, password_reset_token VARCHAR(255) DEFAULT NULL, password_requested_at DATETIME DEFAULT NULL, UNIQUE INDEX UNIQ_8D93D649A5D24B55 (credential_email), UNIQUE INDEX UNIQ_8D93D6496B7BA4B6 (password_reset_token), PRIMARY KEY(id)) DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_unicode_ci ENGINE = InnoDB
-> ALTER TABLE user_role ADD CONSTRAINT FK_2DE8C6A3A76ED395 FOREIGN KEY (user_id) REFERENCES user (id) ON DELETE CASCADE
-> ALTER TABLE user_role ADD CONSTRAINT FK_2DE8C6A3E09C0C92 FOREIGN KEY (role_name) REFERENCES role (name) ON DELETE CASCADE
++ migrated (1.63s)
------------------------
++ finished in 1.63s
++ 1 migrations executed
++ 8 sql queries
next, validate fails
bin/console doctrine:schema:validate
Mapping
-------
[OK] The mapping files are correct.
Database
--------
[ERROR] The database schema is not in sync with the current mapping file.
and checking for the issue
bin/console doctrine:schema:update --dump-sql
The following SQL statements will be executed:
ALTER TABLE media__media CHANGE provider_metadata provider_metadata LONGTEXT DEFAULT NULL COMMENT '(DC2Type:json)', CHANGE width width INT DEFAULT NULL, CHANGE height height INT DEFAULT NULL, CHANGE length length NUMERIC(10, 0) DEFAULT NULL, CHANGE content_type content_type VARCHAR(255) DEFAULT NULL, CHANGE content_size content_size INT DEFAULT NULL, CHANGE copyright copyright VARCHAR(255) DEFAULT NULL, CHANGE author_name author_name VARCHAR(255) DEFAULT NULL, CHANGE context context VARCHAR(64) DEFAULT NULL, CHANGE cdn_is_flushable cdn_is_flushable TINYINT(1) DEFAULT NULL, CHANGE cdn_flush_identifier cdn_flush_identifier VARCHAR(64) DEFAULT NULL, CHANGE cdn_flush_at cdn_flush_at DATETIME DEFAULT NULL, CHANGE cdn_status cdn_status INT DEFAULT NULL;
ALTER TABLE user CHANGE password_reset_token password_reset_token VARCHAR(255) DEFAULT NULL, CHANGE password_requested_at password_requested_at DATETIME DEFAULT NULL;
bin/console doctrine:schema:update --force
Updating database schema...
2 queries were executed
[OK] Database schema updated successfully!
bin/console doctrine:schema:update --dump-sql
The following SQL statements will be executed:
ALTER TABLE media__media CHANGE provider_metadata provider_metadata LONGTEXT DEFAULT NULL COMMENT '(DC2Type:json)', CHANGE width width INT DEFAULT NULL, CHANGE height height INT DEFAULT NULL, CHANGE length length NUMERIC(10, 0) DEFAULT NULL, CHANGE content_type content_type VARCHAR(255) DEFAULT NULL, CHANGE content_size content_size INT DEFAULT NULL, CHANGE copyright copyright VARCHAR(255) DEFAULT NULL, CHANGE author_name author_name VARCHAR(255) DEFAULT NULL, CHANGE context context VARCHAR(64) DEFAULT NULL, CHANGE cdn_is_flushable cdn_is_flushable TINYINT(1) DEFAULT NULL, CHANGE cdn_flush_identifier cdn_flush_identifier VARCHAR(64) DEFAULT NULL, CHANGE cdn_flush_at cdn_flush_at DATETIME DEFAULT NULL, CHANGE cdn_status cdn_status INT DEFAULT NULL;
ALTER TABLE user CHANGE password_reset_token password_reset_token VARCHAR(255) DEFAULT NULL, CHANGE password_requested_at password_requested_at DATETIME DEFAULT NULL;
bin/console doctrine:schema:validate
Mapping
-------
[OK] The mapping files are correct.
Database
--------
[ERROR] The database schema is not in sync with the current mapping file.
ad infinitum ...
@hal869 and what's the entity mapping? Also, as we always ask, could you try to isolate the issue in a test case?
@lcobucci After our offline chat, working on test case, came across this:
https://github.com/doctrine/dbal/issues/2985
and related ...
Updated to latest dbal/symfony/mariadb, and explicitly removed the
config/packages/doctrine.yaml
....
- server_version: 'mariadb-10.3.8'
....
letting the version returned from mariadb get used.
which seems to do the trick for the :validate,
bin/console doctrine:schema:validate
Mapping
-------
[OK] The mapping files are correct.
Database
--------
[OK] The database schema is in sync with the mapping files.
now, to look further downstream ...
@morozov as part of my PR I started to investigate how the MariaDB platform can be improved and faced that MariaDB >= 10.3 still doesn't have native JSON support and it convert this type into LONGTEXT by default.
So the previous PR (https://github.com/doctrine/dbal/pull/2825) should be compatible with 10.3 version as well.
What's the status of this feature? What has to be done in order to get native JSON support for MariaDB?
I've tried to test it by creating a platform class (by extending \Doctrine\DBAL\Platforms\MySqlPlatform
, but I see that MariaDb1027Platform
is final
, has no interface and is used \Doctrine\DBAL\Schema\MySqlSchemaManager::_getPortableTableColumnDefinition
using a private method. Doesn't seem like a good practice, isn't it? Or am I wrong?
I've never contributed to Doctrine before, but let me know if I can help.
What's the status of this feature? What has to be done in order to get native JSON support for MariaDB?
@stephanvierkant all you can see here is the status. There's nothing happening in this regard behind the curtain.
MariaDb1027Platform
isfinal
[…] Doesn't seem like a good practice, isn't it? Or am I wrong?
It looks like an exception. All other platform classes are not final
because they don't have an interface that developers could implement instead of extending the class. Although, in this case, it forces you to contribute the change upstream instead of keeping it to yourself which could be seen as a positive side effect for the community.
I've never contributed to Doctrine before, but let me know if I can help.
Please submit a pull request.
As stated in the MariaDB documentation, the JSON
-alias leads to an automatic JSON_VALID
-check from version 10.4.3 on: https://mariadb.com/kb/en/json-data-type/
So maybe that behavior could be kept for later MariaDB versions by using this alias? (I'm on 10.3 and setting the JSON
-alias always leads to LONGTEXT columns, how is that handled in 10.4.3 onwards?)
I did stumble over this today. As I think it could be a bc break changing:
from LONGTEXT
to JSON
I think maybe it would be possible to add an additional options to teh DSN e.g.: &nativeJson=true
or something and then use JSON
instead of LONGTEXT
.
Seems like this was changed in https://github.com/doctrine/dbal/pull/2825 but I'm not understanding why not let MariaDB handle this?
@alexander-schranz I think a platform option would be more appropriate. See for instance https://github.com/doctrine/dbal/blob/2aec3f8da5f601462162d9b90d013554b240292b/src/Platforms/PostgreSQLPlatform.php#L1217-L1224
Seems like this was changed in #2825 but I'm not understanding why not let MariaDB handle this?
Maybe it was simply overlooked?
Maybe it was simply overlooked?
Not sure 🙈. @lcobucci Do you remember why the JSON type was changed to TEXT again in: https://github.com/doctrine/dbal/pull/2825#issue-253126409?
Maybe it was simply overlooked?
Not sure 🙈. @lcobucci Do you remember why the JSON type was changed to TEXT again in: #2825 (comment)?
It's been a while but it was to reflect the reality (JSON is just an alias): https://mariadb.com/kb/en/json-data-type/ I think that tests were failing and we removed things from the PR to unblock things. But, again, it's been a while.
I'd advise to send a PR proposing a fix and see what happens on the suite now.
To @lcobucci's point, given the behavior documented in https://github.com/doctrine/dbal/pull/5100#discussion_r764557358, I'd say that MariaDB 10.3 does not natively support JSON. And assuming their reasoning:
as the JSON data type contradicts the SQL standard,
It won't support it in the near future.
There is a possibility to improve JSON support in MariaDB by adding CHECK (JSON_VALID())
to the column and introspecting it. This may work.
@morozov they support it in there way by having a JSON alias to LONGTEXT and JSON fields automatically gets a CHECK (JSON_VALID(j))
to them. So I think doctrine/dbal should use the JSON
here and not map it themselves to LONGTEXT which would not add the JSON_VALID
check. So I think we should define it just as JSON
here in dbal and let MariaDB do they work for us.
JSON fields automatically gets a CHECK (JSON_VALID(j)) to them.
It doesn't look like they do, nor that it's stated in the documentation:
Welcome to the MySQL monitor. Commands end with ; or \g.
Your MySQL connection id is 8
Server version: 5.5.5-10.3.32-MariaDB-1:10.3.32+maria~focal mariadb.org binary distribution
mysql> CREATE TABLE t (j JSON);
Query OK, 0 rows affected (0.03 sec)
mysql> insert into t VALUES('foo');
Query OK, 1 row affected (0.00 sec)
mysql> SELECT * FROM t;
+------+
| j |
+------+
| foo |
+------+
1 row in set (0.01 sec)
The DBAL would have to add this check automatically and then check if it exists during the introspection.
@morozov it says in the reasoning you linked that this behavior is supported since version 10.4.3 and it looks like you used 10.3 in the provided logs
Yeah, it looks like it adds the constraint automatically on the newer versions:
Welcome to the MySQL monitor. Commands end with ; or \g.
Your MySQL connection id is 3
Server version: 5.5.5-10.6.5-MariaDB-1:10.6.5+maria~focal mariadb.org binary distribution
mysql> CREATE TABLE t (j JSON);
Query OK, 0 rows affected (0.01 sec)
mysql> INSERT INTO t VALUES('foo');
ERROR 4025 (23000): CONSTRAINT `t.j` failed for `doctrine`.`t`
mysql> SHOW CREATE TABLE t;
+-------+----------------------------------------------------------------------------------------------------------------------------------------------------------+
| Table | Create Table |
+-------+----------------------------------------------------------------------------------------------------------------------------------------------------------+
| t | CREATE TABLE `t` (
`j` longtext CHARACTER SET utf8mb4 COLLATE utf8mb4_bin DEFAULT NULL CHECK (json_valid(`j`))
) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 |
+-------+----------------------------------------------------------------------------------------------------------------------------------------------------------+
1 row in set (0.00 sec)
Feature Request
Summary
https://github.com/doctrine/dbal/commit/52b4692ebefd5a25420238aa53176ef8d4dc2e83 disabled JSON support for MariaDB 10.2, since the engine is based on 5.6.
MariaDB 10.3 doesn't have its own platform file, but I think it needs one now to enable native JSON storage.
WDYT?