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

Backward-compatible sqlite autoincrement #47

Closed
wants to merge 6 commits into from

Conversation

basiliscos
Copy link

Hello,

Here is another attempt to introduce sqlite monotonic autoincrement which tries to be back-compatible.

Without extending SQL::Translator::Schema::Field I decided to use extra field to store type of autoincrement, which later could be re-used from Producer.

I hope this would be OK.

I tried to use it directly in DBIx::Class :

{ data_type => "integer", is_auto_increment => 1, is_nullable => 0, extra => { autoinc_method => 'sequence' } }
  • it works that way. But how to specify extra more correctly in DBIx::Class? i..e outside of auto-generated sources?

Thanks for the great project!

@ilmari
Copy link
Member

ilmari commented Dec 14, 2014

SQL::Translator::Parser::SQLite currently only sets is_auto_increment for AUTOINCREMENT columns. Arguably it should set that for INTEGER PRIMARY KEY columns as well, and add extra => { autoinc_method => 'sequence' } if the column is declared with AUTOINCREMENT.

I also think that, for consistency, the extra field should be auto_increment_method.

@basiliscos
Copy link
Author

OK, I have renamed auto_increment_method in extra field.

Should I enhance SQL::Translator::Parser::SQLite too, as described above? I ask that because it seems to be breaking compatibilty?

auto_increment_method => 'sequence',
},
);
my $expected = [ qq<CREATE TABLE "some_table" (\n "id" integer AUTOINCREMENT NOT NULL\n)>];
Copy link
Member

Choose a reason for hiding this comment

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

This is not actually valid in SQLite, AUTOINCREMENT can only be used immediately after INTEGER PRIMARY KEY, so field_autoinc() should be checking $self->_ipk($field) as well as $field->is_auto_increment and auto_increment_method.

@ilmari
Copy link
Member

ilmari commented Jan 7, 2015

Enhancing Parser::SQLite can be done separately. I don't consider fixing parser bugs/limitations that improve the fidelity of the parsed schema and fix round-tripping to be backwards-compatibility breakage.

@basiliscos
Copy link
Author

Indeed. Thanks for noting that.

@ribasushi
Copy link
Contributor

Hi, just looked over this in depth.

The patch itself is good, naming not so much. Would you be opposed to the following changes:
auto_increment_method => auto_increment_type
and sequence => monotonic

@ribasushi
Copy link
Contributor

Applied with proposed changes as 7f3f64d. Thank you!

@ribasushi ribasushi closed this Mar 5, 2015
@karupanerura
Copy link

Great 👍!
I need it. When it release to CPAN?

@ribasushi
Copy link
Contributor

On 10/12/2015 08:34 AM, karupanerura wrote:

Great 👍!
I need it. When it release to CPAN?

In about another week. Sorry for the delay with this :/

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.

None yet

4 participants