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

DiffGenerator: fix applying regex pattern filter to mappings schema #1490

Open
wants to merge 1 commit into
base: 3.8.x
Choose a base branch
from

Conversation

cristi-contiu
Copy link
Contributor

@cristi-contiu cristi-contiu commented Feb 14, 2025

Q A
Type improvement
BC Break no/minor
Fixed issues #1487

Summary

DiffGenerator applies regex pattern filtering to the metadata-based schema ($toSchema) using DBAL's schemaAssetsFilter (and DoctrineBundle's doctrine.dbal.schema_filter, if applicable), which was meant for filtering the introspected schema ($fromSchema ) to prevent dropping existing tables from the database (DBAL and DoctrineBundle already cover this). This is also explained in the docs : "custom tables which are not managed by Doctrine" = not in the Doctrine mappings, so if an asset is in the metadata ("managed by Doctrine"), it should be included in both $toSchema and $fromSchema (if it exists), regardless of doctrine.dbal.schema_filter config.

This extra filtering also generates differences in the diff created by Migrations vs the ones created by ORM's SchemaTool, as explained in #1487 .

The logic not to exclude tables in the introspected schema that exist in the mappings was ported from doctrine/orm#7875 and prevents adding unwanted "CREATE TABLE" queries.


$schemaAssetsFilter = $this->dbalConfiguration->getSchemaAssetsFilter();
if ($previousFilter === null) {
return $this->schemaManager->introspectSchema();
Copy link
Contributor Author

@cristi-contiu cristi-contiu Feb 14, 2025

Choose a reason for hiding this comment

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

This is kept for compatibility with DBAL v3. In DBAL v4, it can no longer be null. Might trigger a codecov warning

@cristi-contiu cristi-contiu changed the title Update generated diff to match ORM schema diff DiffGenerator: align generated diff with ORM SchemaTool Feb 14, 2025
@@ -53,16 +51,16 @@ static function ($assetName) use ($filterExpression) {
$assetName = $assetName->getName();
}

return preg_match($filterExpression, $assetName);
return (bool) preg_match($filterExpression, $assetName);
Copy link
Contributor Author

@cristi-contiu cristi-contiu Feb 14, 2025

Choose a reason for hiding this comment

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

Casting to bool for docs and DoctrineBundle alignment

@cristi-contiu cristi-contiu force-pushed the align-diff-with-orm branch 6 times, most recently from 0ae9dc5 to 92b5f6c Compare February 14, 2025 20:48
@cristi-contiu cristi-contiu changed the title DiffGenerator: align generated diff with ORM SchemaTool DiffGenerator: fix applying regex filter to mappings schema Feb 18, 2025
@cristi-contiu cristi-contiu changed the title DiffGenerator: fix applying regex filter to mappings schema DiffGenerator: fix applying regex pattern filter to mappings schema Feb 18, 2025
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

Can you please add a failing test?

@cristi-contiu
Copy link
Contributor Author

cristi-contiu commented Feb 27, 2025

@greg0ire My use case is the DiffCommand being called without an explicit --filter-expression , but the DBAL already having a SchemaAssetsFilter callback configured (by DoctrineBundle and the doctrine.dbal.schema_filter config).

Failing test and pipeline

There are differences in the way Migrations and DBAL apply the filter: DBAL applies it during schema introspection on what it thinks it's the most accurate table name using "portableTableDefinition" which has platform-specific checks like schemas support or current / default search-path schemas (for example some_schema.my_table on PostgreSQL), while Migrations removes the schema and applies the filter only on the unqualified table name (for the same table as earlier, it would be only my_table).

The Migrations library should at least apply the regex filtering on the exact table name provided by the user in the mappings, it should not assume that some_schema.my_table and my_table reference the same table . I suggested a fix that keeps the schema name for platforms that support schemas.

@cristi-contiu
Copy link
Contributor Author

cristi-contiu commented Feb 28, 2025

Then there is the broader issue (addressed in this PR): if the DiffGenerator should manipulate/filter the mapped $toSchema at all (the ORM SchemaTool does not).

The metadata schema does not have the advanced platform-specific checks that the introspected schema has, and any apparent differences are addressed by the Comparator during diff.

For example, a mapped table with name public.my_table OR a table with both name my_table and explicit schema/namespace public OR a table just with name my_table can reference the same table in PostgreSQL, but filtering just by the provided name as string without platform-specific knowledge would produce mixed results. A safer approach would be to remove that table from the mappings instead on relying on Migrations library to filter it out.

@greg0ire
Copy link
Member

I don't know why there is a filter at the migrations level. @goetas , maybe you know?

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.

2 participants