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

JSON fields support #25479

Merged
merged 2 commits into from Jan 3, 2020
Merged

JSON fields support #25479

merged 2 commits into from Jan 3, 2020

Conversation

@akaplya
Copy link
Contributor

akaplya commented Nov 5, 2019

Description (*)

Starting MySQL 5.7 supports native JSON data type. https://dev.mysql.com/doc/refman/5.7/en/json.html
This pull request enables the possibility to use JSON fields with Magento declarative schema.

Manual testing scenarios (*)

db_schema.xml
<?xml version="1.0"?>
<!--
/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */
-->
<schema xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
        xsi:noNamespaceSchemaLocation="urn:magento:framework:Setup/Declaration/Schema/etc/schema.xsd">
    <table name="test_table" resource="default" engine="innodb" comment="Data">
        <column xsi:type="bigint" name="id" padding="20" unsigned="true" nullable="false" identity="true" comment="ID"/>
        <column xsi:type="json" name="data" nullable="false" comment="data"/>
        <constraint xsi:type="primary" referenceId="PRIMARY">
            <column name="id"/>
        </constraint>
    </table>
</schema>
Structure of created table
mysql> show create table test_table\G
*************************** 1. row ***************************
       Table: test_table
Create Table: CREATE TABLE `test_table` (
  `id` bigint(20) unsigned NOT NULL AUTO_INCREMENT COMMENT 'ID',
  `data` json NOT NULL COMMENT 'data',
  PRIMARY KEY (`id`)
) ENGINE=InnoDB DEFAULT CHARSET=utf8 COMMENT='Data'
1 row in set (0.00 sec)

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds are green)
@akaplya akaplya requested a review from buskamuza as a code owner Nov 5, 2019
@m2-assistant

This comment has been minimized.

Copy link

m2-assistant bot commented Nov 5, 2019

Hi @akaplya. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento give me test instance - deploy test instance based on PR changes
  • @magento give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Guide documentation.

@skovalenk

This comment has been minimized.

Copy link
Contributor

skovalenk commented Nov 5, 2019

Looks good!

1 similar comment
@skovalenk

This comment has been minimized.

Copy link
Contributor

skovalenk commented Nov 5, 2019

Looks good!

@akaplya

This comment has been minimized.

Copy link
Contributor Author

akaplya commented Nov 6, 2019

SVC failed due to XSD change, but this can not affect existing extensions except the case when someone already declared JSON type locally, which is very unlikely.

Copy link
Contributor

ihor-sviziev left a comment

Hi @akaplya,

In general your changes looks good to me.

According to mysql docs:

As of MySQL 5.7.8, MySQL supports a native JSON data type

But in the magento requirements there is MySQL 5.7.x (it means version 5.7.0-5.7.7 also supported).

The Magento application requires MySQL 5.6.x. Magento versions 2.1.2 and later are compatible with MySQL 5.7.x. Magento is also compatible with MySQL NDB Cluster 7.4.x, MariaDB 10.0, 10.1, 10.2, Percona 5.7 and other binary compatible MySQL technologies.

Introducing this new functionality might brake some functionality for people who using mysql 5.7.0-5.7.7, I mean you're just adding support, but someone will start using it in the core and finally brake it.

I think together with this PR we need to define that requirements are MySQL 5.7.x, but not lower than 5.7.8.

What do you think?

@akaplya

This comment has been minimized.

Copy link
Contributor Author

akaplya commented Nov 11, 2019

@ihor-sviziev This PR has introduced a possibility to use the JSON field but did not add any real usages. Actually. This feature more for the community than for core. As an extension developer, you have significantly more freedom and control over the environment. So the developer can make a deciding on usage/not usage JSON fields.
Eventually, this feature will be used inside of core after Magento bumps the version of supported MySQL.

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

magento-engcom-team commented Nov 13, 2019

Hi @ihor-sviziev, thank you for the review.
ENGCOM-6276 has been created to process this Pull Request
✳️ @ihor-sviziev, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests
@akaplya

This comment has been minimized.

Copy link
Contributor Author

akaplya commented Nov 14, 2019

Unfortunately, this feature cannot be covered with integration tests :( because Magento CI/CD, which is still using MySQL 5.6 under the hood. Because Magento still officially supports 5.6.

@engcom-Alfa

This comment has been minimized.

Copy link

engcom-Alfa commented Nov 15, 2019

✔️ QA Passed

Before:
before

After:
after

@engcom-Alfa engcom-Alfa moved this from Testing in Progress to Extended Testing (optional) in Pull Requests Dashboard Nov 15, 2019
@jonashrem

This comment has been minimized.

Copy link
Contributor

jonashrem commented Nov 18, 2019

This will also break compatibility with MariaDB.

@sshymko

This comment has been minimized.

Copy link

sshymko commented Nov 18, 2019

@jonashrem,
Does MariaDB not have the same JSON type as MySQL?

@jonashrem

This comment has been minimized.

Copy link
Contributor

jonashrem commented Nov 18, 2019

@sshymko see https://mariadb.com/kb/en/library/incompatibilities-and-feature-differences-between-mariadb-103-and-mysql-57/ there are some differences.

Depending on how JSON is used, it might still work through.

MariaDB stores JSON as true text, not in binary format as MySQL. MariaDB's JSON functions are much faster than MySQL's so there is no need to store in binary format, which would add complexity when manipulating JSON objects.
For the same reason, MariaDB's JSON data type is an alias for LONGTEXT. If you want to replicate JSON columns from MySQL to MariaDB, you should store JSON objects in MySQL in a TEXT or LONGTEXT column or use statement based replication. If you are using JSON columns and want to upgrade to MariaDB, you need to either convert them to TEXT or use mysqldump to copy these tables to MariaDB.
In MySQL, JSON is compared according to json values. In MariaDB JSON strings are normal strings and compared as strings.
MariaDB 10.3 does not support MySQL's JSON operators (-> and ->>).

@akaplya

This comment has been minimized.

Copy link
Contributor Author

akaplya commented Nov 19, 2019

This PR does not introduce any JSON fields into the core product. As a result, it is 100% backward compatible. It presents a possibility to add the JSON field in extension in case a developer is confident enough about the environment where the extension will be deployed.
This PR does not say how to operate with JSON fields, it says how to create a table with JSON fields through the declarative schema.
As a result, it is compatible with MariaDB starting 10.2.7
https://mariadb.com/kb/en/library/json-data-type/

@slavvka

This comment has been minimized.

Copy link
Member

slavvka commented Dec 6, 2019

Guys, this PR is currently under the architecture review

@slavvka

This comment has been minimized.

Copy link
Member

slavvka commented Dec 10, 2019

The PR is approved by architects

@magento-engcom-team

This comment has been minimized.

Copy link
Contributor

magento-engcom-team commented Dec 10, 2019

Hi @slavvka, thank you for the review.
ENGCOM-6276 has been created to process this Pull Request

magento-engcom-team pushed a commit that referenced this pull request Jan 3, 2020
@magento-engcom-team magento-engcom-team merged commit ba5ee6e into magento:2.4-develop Jan 3, 2020
9 of 10 checks passed
9 of 10 checks passed
Semantic Version Checker Build reports
Details
Adobe CLA Signed? ✓ Adobe (Magento) Employee
Details
Database Compare Build reports
Details
Functional Tests Build reports
Details
Integration Tests Build reports
Details
Magento Health Index Build reports
Details
Sample Data Tests Build reports
Details
Static Tests Build reports
Details
Unit Tests Build reports
Details
WebAPI Tests Build reports
Details
@m2-assistant

This comment has been minimized.

Copy link

m2-assistant bot commented Jan 3, 2020

Hi @akaplya, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

@pull-request-dashboard-manager pull-request-dashboard-manager bot moved this from Merge in Progress to Recently Merged in Pull Requests Dashboard Jan 3, 2020
@magento-engcom-team magento-engcom-team removed this from Recently Merged in Pull Requests Dashboard Jan 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.