[DBAL-984] [GH-671] Fix quoted integers as default value. Created: 28/Aug/14  Updated: 29/Aug/14  Resolved: 29/Aug/14

Status: Resolved
Project: Doctrine DBAL
Component/s: Platforms
Affects Version/s: None
Fix Version/s: 2.5
Security Level: All

Type: Bug Priority: Minor
Reporter: Doctrine Bot Assignee: Steve Müller
Resolution: Fixed Votes: 0
Labels: None


 Description   

This issue is created automatically through a Github pull request on behalf of josemalonsom:

Url: https://github.com/doctrine/dbal/pull/671

Message:

The comparison names for the bigint and smallint types in the sql declaration for default values are misspelled and the values are returned quoted unlike the integer type that is returned unquoted.



 Comments   
Comment by Doctrine Bot [ 29/Aug/14 ]

A related Github Pull-Request [GH-671] was closed:
https://github.com/doctrine/dbal/pull/671

Comment by Steve Müller [ 29/Aug/14 ]

Fixed in commit: https://github.com/doctrine/dbal/commit/60a19586699e9e6ab734c810103ae4294f2ab77f





[DBAL-983] [GH-670] Handle default values for boolean, datetime, and datetimetz columns in PostgreSQL Created: 27/Aug/14  Updated: 28/Aug/14

Status: Open
Project: Doctrine DBAL
Component/s: None
Affects Version/s: None
Fix Version/s: None
Security Level: All

Type: Bug Priority: Major
Reporter: Doctrine Bot Assignee: Benjamin Eberlei
Resolution: Unresolved Votes: 0
Labels: None


 Description   

This issue is created automatically through a Github pull request on behalf of sarcher:

Url: https://github.com/doctrine/dbal/pull/670

Message:

When dealing with legacy schema it would be nice to be able to map default values and not have the schema spit out `ALTER` statements each time. This works correctly today for basic integer and string columns, but does not handle columns with special types such as `boolean` or `datetime`.

For example, the following statement:

`ALTER TABLE test_table ALTER test_column SET DEFAULT CURRENT_TIMESTAMP;`

Results in a column definition like:

`test_column | timestamp with time zone | not null default now()`

However, repeating the same schema generation will result in the same `ALTER` statement each time, because it will always detect that the default value has changed. The same is true for boolean columns.

This simple change prevents this situation from happening and correctly detects that the column default has not changed. It is specific to PostgreSQL.



 Comments   
Comment by Doctrine Bot [ 28/Aug/14 ]

A related Github Pull-Request [GH-670] was assigned:
https://github.com/doctrine/dbal/pull/670





[DBAL-982] [GH-669] Correct schema generation for altering PostgreSQL sequences Created: 25/Aug/14  Updated: 25/Aug/14

Status: Open
Project: Doctrine DBAL
Component/s: None
Affects Version/s: None
Fix Version/s: None
Security Level: All

Type: Bug Priority: Major
Reporter: Doctrine Bot Assignee: Benjamin Eberlei
Resolution: Unresolved Votes: 0
Labels: None


 Description   

This issue is created automatically through a Github pull request on behalf of sarcher:

Url: https://github.com/doctrine/dbal/pull/669

Message:

I wrote a detailed explanation here so it is hopefully easy to follow:

        1. Background

With PostgreSQL there are three states for a column to receive a sequence generator: no generator, an internal shortcut towards generating an auto incrementing sequence (SERIAL), and a manually-created sequence. In its current state, DBAL accepts only a boolean "autoincrement" to its `getAlterTableSql()` method (via the passed-in `TableDiff`).

This results in the following scenario:

  • ORM maps a column to "AUTO" which is treated as a sequence (autoincrement = false)
  • DBAL `PostgreSqlSchemaManager` inspects existing table and notices a sequence (autoincrement = true)
  • Diff logic in `getAlterTableSql()` will always detect that autoincrement has changed, and that the requested value is false, so it will always issue a `DROP DEFAULT` statement

This is clearly a bug, and can be proved via a unit test; run the `AbstractPostgreSqlPlatformTestCase::testAlterSchemaSequenceToSequence` test that I have committed against the current DBAL code and you will see it fail (based on what is currently passed in via the ORM; see Q&A below for detailed explanation).

        1. Solution

There already existed a few references to a not-yet-implemented `sequence` property of a column definition, which would store the name of the sequence being used on the column. This makes sense and allows us to support all three column states with regards to sequence, while also preserving backwards compatibility. The default is always null, so it will result in no changes to functionality on other platforms.

So, this PR:

  • Adds the `Column::_sequence` property
  • Correctly sets that property during the `PostgreSqlSchemaManager::_getPortableTableColumnDefinition` method (it actually was already there, just not being used)
  • Checks the value during the `Comparator::diffColumn` operation
  • Uses the value to more correctly determine when to create/drop a sequence during a schema alter
  • Adds the appropriate unit tests to verify the six different combinations here
        1. Q&A

Is this just an ORM bug? Could the ORM be changed to just set autoincrement = true for the AUTO and SEQUENCE strategies?

This would solve the immediate problem, yes. However, it would leave no possible distinction between the three ORM mapping strategies of AUTO, SEQUENCE, and IDENTITY. Further, it would cause a break in `getAlterTableSql()` because setting autoincrement to true implies that we are using the database-generated identity sequence which has a specific name, thereby removing the ability for a user to define a sequence manually with a custom name and have it be used here. This strategy opens the door for addressing those cases later, and does not cause a BC break today.

This seems incomplete; for example, this still doesn't handle the case where a sequence is requested to change from AUTO to a specifically-named sequence.

That is intentional. I think these other cases could and probably should be handled, but they would require changes to both the DBAL and ORM. For example, we pass a `Sequence` object to the create/alter/drop sequence functions, but we do not pass one to the alter function. As a result, the actual sequence name is not available here, so we would absolutely need to make a more thought-out ORM change to solve this. I think that should be separate work, or it could just be something we do not support.

Won't this still require an ORM change to set the `sequence` property?

Yes, the following needs to be added to the `SchemaTool::gatherColumn` method:

```php
if ($class->isIdGeneratorSequence() && $class->getIdentifierFieldNames() == array($mapping['fieldName']))

{ $options['sequence'] = $class->sequenceGeneratorDefinition['sequenceName']; }

```

However, even without that change, this solves the problem on the DBAL side and opens the door to fixing the ORM to behave correctly here.

How do things behave today and how do we want them to behave with respect to alters?

This is what should happen in each alter scenario:

Existing State Mapped to AUTO or SEQUENCE Mapped to IDENTITY (Auto Increment) Regular Column
-------------- ----------------------- ----------------------------------- --------------
*No Sequence* Add Sequence; Default to `nextval(seqeuence)` Add Serial (Identity Sequence) No Change
*Default `nextval(seqeuence)`* No Change No Change Drop Default

This is what does happen today:

Existing State Mapped to AUTO or SEQUENCE Mapped to IDENTITY (Auto Increment) Regular Column
-------------- ----------------------- ----------------------------------- --------------
*No Sequence* No Change (Bad) Add Serial (Identity Sequence) No Change
*Default `nextval(seqeuence)`* Drop Default (Bad) No Change Drop Default





Generated at Mon Sep 01 11:16:55 UTC 2014 using JIRA 6.2.3#6260-sha1:63ef1d6dac3f4f4d7db4c1effd405ba38ccdc558.