[WIP] Feature schema management #1267

discordier · 2019-01-26T14:09:30Z

Description

As we have several problems with strict mode and therefore need to upgrade the database, here is a first throw at schema abstraction.

Companion PRs:

TODO:

integrate the schema updater in the backend
disable schema manipulation from legacy backend services for new attributes.
allow column dropping when no legacy attribute is in use (meaning unknown columns are really obsolete).

src/Attribute/ISimple.php

dmolineus

Looks good so far. I added some minor change requests.

composer.json

src/Information/MetaModelCollection.php

src/Schema/Doctrine/AbstractAttributeTypeSchemaGenerator.php

dmolineus · 2019-01-30T14:52:31Z

src/Schema/Doctrine/DoctrineSchemaGeneratorHelperTrait.php

+
+        $column = $tableSchema->getColumn($name);
+        $column->setType(Type::getType($typeName));
+        if ([] !== $options) {


Shouldn't the options not being always applied? What if I want to force having no options?

You are aware that this is an empty check and only calls the setter if any option is to be set on the doctrine column?

Yes. My questions relates to the case that the injected table schema already has the column defined with some options. What would happen if the option might have changed?

Maybe it does not happen in the intended use case of the helper trait but at least the behaviour should be documented.

I still do not get you on this one.
Where shall the options change?
As soon as the column is in schema, the schema is the master source, as the attribute is schema aware and therefore reflects the desired state.
If you (as third party developer) want to change the options in the schema, it is up to you to register an own schema generator that will override the column data again (registered with lower priority than the stock generator).

You can see the generator concept somewhat like compiler passes being sequentially run.
This trait merely is a helper that abstracts the whole "loop over all tables and all attributes and configure only the ones of type x" into a single "configure column y of type x"

src/Schema/Doctrine/DoctrineSchemaManipulator.php

discordier · 2019-02-01T16:34:00Z

Change requests so far adressed.

Next step is to integrate the schema updater in the backend and disable schema manipulation from legacy backend services for new attributes.

richardhj · 2019-02-01T19:09:25Z

src/CoreBundle/Command/SchemaValidatorCommand.php

+    protected function configure()
+    {
+        parent::configure();
+        $this->addOption('force', null, InputOption::VALUE_NONE, 'Perform the update');


why do you name the option "force" to perform the update.

Was "inherited" from doctrine:schema:upgrade, I am not yet confident on the naming and even functionality though.
Doctrine has the plain command which simply states: "Schema is not in sync" and then you cann optionally pass one of the following options:

--dump-sql Dumps SQL on the console to update - won't work for us, as not every schema is SQL.

--force Discouraged in Doctrine but executes the upgrades. This is the only way for our usecase as only the schema managers know how to manage their schema (especially if it is non SQL).

dmolineus · 2019-02-15T09:52:12Z

As discussed yesterday I made some tests to apply schema migration. I set up a simple test case (https://github.com/dmolineus/docker-migration-test). The test scenarios are far away from being complete, but right now there are several issues when migrating schemas if tables contain data

Issues

Change column type to a numeric type from string containing string data
Removing not null option if column contains null values
Add unique index for columns with nun unique values
Truncating data which is too long would results in an SQL warning - in my setup doctrine throws an exception for it

Thoughts

Without dropping columns we likely run into the issues described obove (Example: Change attribute type from text to decimal)
Applying schema changes in small steps would reduce risk of failing schema migrations

Test output

$ docker-compose run php vendor/bin/phpunit
Starting doctrine-migration-test_db_1 ... done
stty: standard input
PHPUnit 8.0.2 by Sebastian Bergmann and contributors.


SchemaMigrationTest
  ✓ change string column type [0.311s]
  x change string column type [0.227s]
  ✓ change string column type [0.283s]
  x change string column type [0.241s]
  x change string column type [0.189s]
  x null to not null [0.230s]
  x null to not null [0.250s]
  ✓ unique index [0.279s]
  x unique index [0.180s]
  ✓ unique index [0.292s]
  x unique index [0.192s]
  x unique index [0.207s]


Time: 2.95 seconds, Memory: 6.00MB

There were 8 failures:

1) SchemaMigrationTest::testChangeStringColumnType with data set #1 ('string', array(), 'decimal', array(0), array('foo', 123, 8, 12.3))
An exception occurred while executing 'ALTER TABLE test_table CHANGE test_column test_column NUMERIC(10, 0) DEFAULT '0' NOT NULL':

SQLSTATE[HY000]: General error: 1366 Incorrect DECIMAL value: '0' for column '' at row -1

/code/tests/SchemaMigrationTestCase.php:88
/code/tests/SchemaMigrationTest.php:17

2) SchemaMigrationTest::testChangeStringColumnType with data set #3 ('string', array(), 'smallint', array(0), array('foo', 123, 8, 12.3))
An exception occurred while executing 'ALTER TABLE test_table CHANGE test_column test_column SMALLINT DEFAULT 0 NOT NULL':

SQLSTATE[HY000]: General error: 1366 Incorrect integer value: 'foo' for column 'test_column' at row 1

/code/tests/SchemaMigrationTestCase.php:88
/code/tests/SchemaMigrationTest.php:17

3) SchemaMigrationTest::testChangeStringColumnType with data set #4 ('string', array(32), 'string', array(8), array('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa'))
An exception occurred while executing 'ALTER TABLE test_table CHANGE test_column test_column VARCHAR(8) NOT NULL':

SQLSTATE[01000]: Warning: 1265 Data truncated for column 'test_column' at row 1

/code/tests/SchemaMigrationTestCase.php:88
/code/tests/SchemaMigrationTest.php:17

4) SchemaMigrationTest::testNullToNotNull with data set #0 ('string', array(false), 'string', array(), array('abs', 123, 12.3, null))
An exception occurred while executing 'ALTER TABLE test_table CHANGE test_column test_column VARCHAR(255) NOT NULL':

SQLSTATE[22004]: Null value not allowed: 1138 Invalid use of NULL value

/code/tests/SchemaMigrationTestCase.php:88
/code/tests/SchemaMigrationTest.php:43

5) SchemaMigrationTest::testNullToNotNull with data set #1 ('string', array(false), 'decimal', array(0), array(123, 12.3, null))
An exception occurred while executing 'ALTER TABLE test_table CHANGE test_column test_column NUMERIC(10, 0) DEFAULT '0' NOT NULL':

SQLSTATE[01000]: Warning: 1265 Data truncated for column 'test_column' at row 3

/code/tests/SchemaMigrationTestCase.php:88
/code/tests/SchemaMigrationTest.php:43

6) SchemaMigrationTest::testUniqueIndex with data set #1 ('string', array(), 'string', array(), array('abc', 'foo', 'foo'))
An exception occurred while executing 'CREATE UNIQUE INDEX UNIQ_FDF7294DEA8A8050 ON test_table (test_column)':

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry 'foo' for key 'UNIQ_FDF7294DEA8A8050'

/code/tests/SchemaMigrationTestCase.php:88
/code/tests/SchemaMigrationTest.php:68

7) SchemaMigrationTest::testUniqueIndex with data set #3 ('decimal', array(), 'decimal', array(), array(1, 2, 1.0))
An exception occurred while executing 'CREATE UNIQUE INDEX UNIQ_FDF7294DEA8A8050 ON test_table (test_column)':

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '1' for key 'UNIQ_FDF7294DEA8A8050'

/code/tests/SchemaMigrationTestCase.php:88
/code/tests/SchemaMigrationTest.php:68

8) SchemaMigrationTest::testUniqueIndex with data set #4 ('decimal', array(), 'decimal', array(), array(1, 2, 1.0, 1))
An exception occurred while executing 'CREATE UNIQUE INDEX UNIQ_FDF7294DEA8A8050 ON test_table (test_column)':

SQLSTATE[23000]: Integrity constraint violation: 1062 Duplicate entry '1' for key 'UNIQ_FDF7294DEA8A8050'

/code/tests/SchemaMigrationTestCase.php:88
/code/tests/SchemaMigrationTest.php:68

FAILURES!
Tests: 12, Assertions: 48, Failures: 8.

dmolineus · 2019-02-15T09:57:14Z

My considerations about the open todo how to intergrate the schema updater in the backend:

For user experience it would be great if the schama update is applied automatically every time the schema changes
This would decrease risk of failing migrations
Automatic schema updating could be deactivated in the bundle configuration if any projects require a custom way

zonky2 · 2019-02-15T10:05:21Z

in additional to #1267 (comment)

what do we do if a schema update fails?

Before only slugs were read

Hotfix release 2.1.4 - Read parameters from the get section before ths slug parameters are read - Ignore rendering of internal attributes in show

This will add the remaining attributes (all non `ISchemaManagedAttribute` or `IInternal`) to the legacy schema.

discordier mentioned this pull request Jan 26, 2019

[WIP] Feature schema management MetaModels/attribute_alias#26

Open

discordier assigned discordier, stefanheimes, zonky2 and richardhj Jan 26, 2019

discordier added the enhancement label Jan 26, 2019

discordier added this to the 2.1.0 milestone Jan 26, 2019

zonky2 mentioned this pull request Jan 26, 2019

Error messages by "strict mode" #1268

Closed

richardhj reviewed Jan 26, 2019

View changes

src/Attribute/ISimple.php Outdated Show resolved Hide resolved

This was referenced Jan 28, 2019

[2.2] error at rename attribute column #1266

Open

[2.2] rename attribute column without delete+new #1265

Open

dmolineus requested changes Jan 30, 2019

View changes

dmolineus reviewed Jan 30, 2019

View changes

src/Schema/Doctrine/DoctrineSchemaManipulator.php Show resolved Hide resolved

richardhj reviewed Feb 1, 2019

View changes

zonky2 mentioned this pull request Feb 7, 2019

Error messages by "strict mode" #1232

Closed

zonky2 requested a review from stefanheimes Feb 14, 2019

zonky2 modified the milestones: 2.1.0, Future Mar 1, 2019

discordier closed this Mar 11, 2019

discordier changed the base branch from release/2.1.0 to hotfix/2.1.1 Mar 11, 2019

discordier changed the base branch from hotfix/2.1.1 to master Mar 11, 2019

discordier reopened this Mar 11, 2019

discordier and others added 4 commits Jun 6, 2019

Also read parameters from "get" section

Loading status checks…

d612edd

Before only slugs were read

Ignore rendering of internal attributes in show

Loading status checks…

8f5a455

Initial draft of schema generator handling

19d9d1f

discordier added 11 commits Jan 26, 2019

Correctly return void

e3dfe2b

Add PR number in deprecation hint

deef81c

Initialize array

8cfe3e0

Priorize tagged services

7fd231f

Add legacy schema generator

73381fb

This will add the remaining attributes (all non `ISchemaManagedAttribute` or `IInternal`) to the legacy schema.

Fix class name in deprecations

8450c04

Add legacy schema manager

6fc63b0

Add SchemaManagerInterface::validate()

db386d3

Add console command for updating the schema

7aac224

Lower doctrine constraint

560ddca

Fix some return types

Loading status checks…

1599dbb

discordier force-pushed the release/2.1.0-schema-management branch from a87324b to 1599dbb Aug 2, 2019

MetaModels / core

[WIP] Feature schema management #1267

[WIP] Feature schema management #1267

discordier commented Jan 26, 2019 •

edited by zonky2

dmolineus left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

discordier commented Feb 1, 2019

This comment has been minimized.

This comment has been minimized.

dmolineus commented Feb 15, 2019 •

edited

dmolineus commented Feb 15, 2019

zonky2 commented Feb 15, 2019

MetaModels / core

[WIP] Feature schema management #1267

[WIP] Feature schema management #1267

Conversation

discordier commented Jan 26, 2019 • edited by zonky2

Description

dmolineus left a comment

This comment has been minimized.

dmolineus Jan 30, 2019 Contributor

This comment has been minimized.

discordier Feb 1, 2019 Author Member

This comment has been minimized.

dmolineus Feb 1, 2019 Contributor

This comment has been minimized.

discordier Feb 5, 2019 Author Member

discordier commented Feb 1, 2019

This comment has been minimized.

richardhj Feb 1, 2019 Member

This comment has been minimized.

discordier Feb 1, 2019 Author Member

dmolineus commented Feb 15, 2019 • edited

dmolineus commented Feb 15, 2019

zonky2 commented Feb 15, 2019

discordier commented Jan 26, 2019 •

edited by zonky2

dmolineus Jan 30, 2019
Contributor

discordier Feb 1, 2019
Author Member

dmolineus Feb 1, 2019
Contributor

discordier Feb 5, 2019
Author Member

richardhj Feb 1, 2019
Member

discordier Feb 1, 2019
Author Member

dmolineus commented Feb 15, 2019 •

edited