$indexes 'Unique' does not work for versioned DO #2403

Closed
g4b0 opened this Issue Sep 12, 2013 · 7 comments

Comments

Projects
None yet
4 participants
@g4b0
Contributor

g4b0 commented Sep 12, 2013

private static $indexes = array( 
        'Number' => 'Unique', //make it a unique index 
);

ALTER TABLE is also applied to DO_versions, where the column can't be UNIQUE due to the nature of the table. Versions table should be excluded from UNIQUE indexes

@oddnoc

This comment has been minimized.

Show comment Hide comment
@oddnoc

oddnoc Nov 26, 2013

Contributor

This appears to be because Versioned does not detect unique indexes properly. There is code to change them from unique to ordinary indexes, but it never executes. Using xdebug to trace Versioned.php (3.0), the failure to change an index from unique to non-unique for the _versions table is at line 414. In that line, $index is tested with is_array(). But $index is never an array. It is either a boolean 1 or a string containing "unique".

                    //Unique indexes will not work on versioned tables, so we'll convert them to standard indexes:
                    foreach($indexes as $key=>$index){
                        if(is_array($index) && strtolower($index['type'])=='unique'){ //line 414
                            $indexes[$key]['type']='index';
                        }

I don't understand this deeply enough to attempt a fix. I hope @chillu can have a look! This is a showstopper for a project I'm currently working on.

Contributor

oddnoc commented Nov 26, 2013

This appears to be because Versioned does not detect unique indexes properly. There is code to change them from unique to ordinary indexes, but it never executes. Using xdebug to trace Versioned.php (3.0), the failure to change an index from unique to non-unique for the _versions table is at line 414. In that line, $index is tested with is_array(). But $index is never an array. It is either a boolean 1 or a string containing "unique".

                    //Unique indexes will not work on versioned tables, so we'll convert them to standard indexes:
                    foreach($indexes as $key=>$index){
                        if(is_array($index) && strtolower($index['type'])=='unique'){ //line 414
                            $indexes[$key]['type']='index';
                        }

I don't understand this deeply enough to attempt a fix. I hope @chillu can have a look! This is a showstopper for a project I'm currently working on.

@oddnoc

This comment has been minimized.

Show comment Hide comment
@oddnoc

oddnoc Nov 26, 2013

Contributor

After further investigation, this appears to be due to a weakness in the documentation of how to set up unique indexes. If you set a unique index with code like this, you get the failure:

static $indexes = array('SEOName' => 'unique (SEOName)', );

The correct way is like this:

static $indexes = array('SEOName' => array('type' => 'unique', 'value' => 'SEOName',));

I'll look for a place to document this, and submit a pull request.

Contributor

oddnoc commented Nov 26, 2013

After further investigation, this appears to be due to a weakness in the documentation of how to set up unique indexes. If you set a unique index with code like this, you get the failure:

static $indexes = array('SEOName' => 'unique (SEOName)', );

The correct way is like this:

static $indexes = array('SEOName' => array('type' => 'unique', 'value' => 'SEOName',));

I'll look for a place to document this, and submit a pull request.

@tractorcow

This comment has been minimized.

Show comment Hide comment
@tractorcow

tractorcow Nov 26, 2013

Contributor

Hi @oddnoc , another correct way to setup a unique index is as below:

private static $indexs = array(
    'NameIndex' => 'unique ("FirstName","LastName")',
    'OtherIndex' => 'unique ("Code")'
)

I have written some code in upcoming pull requests to elegantly parse indexes in a consistent manner, but in the absense of such code you'll need to consider both cases.

The versioned code above should also consider the is_string($index) case as well, and handle it in a similar way.

It wouldn't hurt to consider a documentation update to include the above, if you feel like taking this on. :)

Contributor

tractorcow commented Nov 26, 2013

Hi @oddnoc , another correct way to setup a unique index is as below:

private static $indexs = array(
    'NameIndex' => 'unique ("FirstName","LastName")',
    'OtherIndex' => 'unique ("Code")'
)

I have written some code in upcoming pull requests to elegantly parse indexes in a consistent manner, but in the absense of such code you'll need to consider both cases.

The versioned code above should also consider the is_string($index) case as well, and handle it in a similar way.

It wouldn't hurt to consider a documentation update to include the above, if you feel like taking this on. :)

@tractorcow

This comment has been minimized.

Show comment Hide comment
@tractorcow

tractorcow Nov 26, 2013

Contributor

Should have read your commit above.. looks like you're on top of it already, mate. :)

Refer to my notes at oddnoc/silverstripe-framework@9f49bd5

Contributor

tractorcow commented Nov 26, 2013

Should have read your commit above.. looks like you're on top of it already, mate. :)

Refer to my notes at oddnoc/silverstripe-framework@9f49bd5

@oddnoc

This comment has been minimized.

Show comment Hide comment
@oddnoc

oddnoc Nov 26, 2013

Contributor

Yeh, I'm wary of trying to parse the string version of things, because it's too easy to get a regex wrong in the face of the ingenuity :) of web developers. So I took the approach of documenting the one known robust way of doing indexes. Actual pull request in a moment. Cheers!

Contributor

oddnoc commented Nov 26, 2013

Yeh, I'm wary of trying to parse the string version of things, because it's too easy to get a regex wrong in the face of the ingenuity :) of web developers. So I took the approach of documenting the one known robust way of doing indexes. Actual pull request in a moment. Cheers!

oddnoc pushed a commit to oddnoc/silverstripe-framework that referenced this issue Dec 6, 2013

Fred Condo
API: Support string descriptors for unique indexes in Versioned
- Document the format for descriptor arrays
- Implement the behaviour that developers have come to expect for
  string descriptors of indexes
- Add test for handling of unique indexes (MySQL & sqlite3)
- Resolve #2403

Versioned needs to convert unique indexes to non-unique for its suffixed
tables, such as Foo_Live and Foo_versions. Because DataObject accepts
string descriptors such as array('UniqIDX' => 'unique (Uniq)') as well
as array-based descriptors, Versioned needs to recognize string
descriptors. This patch accomplishes that. Before, Versioned would fail
to convert string-described indexes to non-unique, resulting in run-time
errors when creating a new version of an object.

@simonwelsh simonwelsh added 3.1 labels Mar 15, 2014

@tractorcow

This comment has been minimized.

Show comment Hide comment
@tractorcow

tractorcow May 18, 2017

Contributor

This has been fixed for a while.

Contributor

tractorcow commented May 18, 2017

This has been fixed for a while.

@tractorcow tractorcow closed this May 18, 2017

@tractorcow

This comment has been minimized.

Show comment Hide comment
@tractorcow

tractorcow May 18, 2017

Contributor

Fixed with #2686

Contributor

tractorcow commented May 18, 2017

Fixed with #2686

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment