Skip to content

DB Information Schema - COLUMNS #306

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

Conversation

buskamuza
Copy link
Contributor

Problem

This is a subset of #264 , covering only COLUMNS and TABLES interfaces.

Different RDBMS return Information Schema (metadata) information in different format. Magento framework should allow to switch between different implementations in order to support those different versions of DBs, as well as differences between versions.

Solution

Introduce granular interfaces, based in Information Schema standard (looking into MySQL and MariaDB definitions in this scope, links are in the document). Application can switch between different implementation based on the profile.

@larsroettig
Copy link
Member

larsroettig commented Oct 4, 2019

@buskamuza can we add a section about backward compatibly? This is planned for the 2.3.x branch correct?

@buskamuza buskamuza marked this pull request as ready for review October 4, 2019 16:28
public function getIsNullable(): bool {}
public function getExtra(): Extra {}
public function getComment() {}
public function getDefaultValue(): string {}

Choose a reason for hiding this comment

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

?string should be used

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated


`\Magento\Framework\DB\Adapter\AdapterInterface` has methods like `describeTable` which return raw values from database engine. This is leaky interface. We should have interfaces that return normalized data and are compatible with all databases. Methods that don't return normalized data need to be deprecated on `\Magento\Framework\DB\Adapter\AdapterInterface`.

`engine` field in declarative schema need to be deprecated.

Choose a reason for hiding this comment

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

Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed. It's part of original document.

@buskamuza buskamuza mentioned this pull request Oct 9, 2019
@buskamuza buskamuza changed the title Declarative schema database compatibility columns DB Information Schema - COLUMNS Oct 9, 2019
@buskamuza
Copy link
Contributor Author

Can we make $connectionName optional?

public function getIsNullable(): bool {}
public function getExtra(): Extra {}
public function getComment() {}
public function getDefaultValue(): ?string {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this one be more generic and then strongly typed on the type specific implementations?
Examples:

  • \InformationSchema\Table\Column::getDefaultValue()
  • \InformationSchema\Table\VarcharColumn::getDefaultValue(): ?string
  • \InformationSchema\Table\IntColumn::getDefaultValue(): ?int
  • \InformationSchema\Table\FloatColumn::getDefaultValue(): ?float
  • \InformationSchema\Table\DateColumn(): ?string

Copy link
Member

Choose a reason for hiding this comment

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

Should we remove table_name field? I also think that default_value might be better to move to specific types

{
public function getSchema(): string {}
public function getName(): string {}
public function getEngine(): string {}
Copy link
Member

Choose a reason for hiding this comment

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

Propose to either to remove it or introduce enum for engine. I think we need to introduce enums for collation and charset. What is schema, definition of the table?

Copy link
Contributor

Choose a reason for hiding this comment

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

I would not use an enum for the engine. Technically most RDBMS support having storage engines other than their default but this is mostly used in MySQL / MariaDB. Introducing an enum for engines could become a maintenance issue as MySQL / MariaDB evolve (e.g. as of MariaDB 10.4 the default storage engine is AriaDB which is an evolution of MyISAM).

Schema is (more or less) the database in which the table resides. SQL defines that a table resides in the schema of a catalog, hence the columns information_schema.tables.catalog and information_schema.tables.schema. MySQL / MariaDB do not support catalogs thus the schema reflects the database (as opposed to PostgreSQL supporting multiple schemas per catalog).

```
class InformationSchema\Extra
{
public function getOnUpdate(): string {}
Copy link
Member

Choose a reason for hiding this comment

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

I think it might differ between databases, I would propose to introduce enums.

```
class InformationSchema\Table\VarcharColumn extends Column
{
public const TYPE = 'VARCHAR';
Copy link
Member

Choose a reason for hiding this comment

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

I would remove it from here because it differs between database engines.


public function isUnsigned(): bool {}
public function getPadding(): int {}
public function getType(): IntEnum {}
Copy link
Member

Choose a reason for hiding this comment

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

Propose to have different types instead.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed

The profiles are declared in `di.xml` or as a separate configuration, with the following structure:

```
information_schema_profiles:
Copy link
Member

Choose a reason for hiding this comment

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

Should it be databse_information_schema_profiles?

```
class InformationSchema\Table
{
public function getSchema(): string {}
Copy link
Member

Choose a reason for hiding this comment

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

I suggest to remove it as it comes from connection and changing this field will not rename database.

@buskamuza
Copy link
Contributor Author

Closing in favor of #318

@buskamuza buskamuza closed this Oct 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants