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

SQLite - TableSet::update_table(...) always throwing (tries to drop primary key) #8

Closed
rkubik opened this issue Oct 7, 2015 · 2 comments
Assignees

Comments

@rkubik
Copy link

rkubik commented Oct 7, 2015

Reference:

table_set.cpp:68 
void TableSet::update_table( Table &required ) 

required.fields() and existing.fields() will always be different because of the automatic primary key assignment. Using the example table below:

td.fields() = fields
        ("code",        ActiveRecord::text)
        ("title",       ActiveRecord::text);

required.fields() will contain: [code, title]
existing.fields() will contain: [id, code, title]

Thus, the set difference required.fields() - existing.fields() will always return the primary key id and try to drop it.

Proposed solution:

void TableSet::update_table( Table &required ) {
  log( "TableSet::update_table" );
  log( required.table_name() );
  Table existing = table_data( required.connection(), required.table_name() );
  Fields missing = required.fields() - existing.fields();
  Fields remove  = existing.fields() - required.fields();
  for( Fields::iterator it = missing.begin(); it != missing.end(); ++it )
    existing.add_field( *it );
  for( Fields::iterator it = remove.begin(); it != remove.end(); ++it ) {
    if ((*it).name() == existing.primary_key()) /* fix */
      continue;
    throw ActiveRecordException( "Table::remove_field not yet implemented", __FILE__, __LINE__ );
    //existing.remove_field( *it );
  }
}

Better solution:

The fields() method should always return the same Fields no matter the context (ie. database fetch or class). The TableSet::table_data function should not load the primary key as a Field but instead only set the id_ variable of the Table. Of course I say this without fully understanding the impact, but hopefully you agree with keeping everything deterministic.

@joeyates
Copy link
Owner

joeyates commented Oct 7, 2015

I think we can adopt the proposed solution in the first instance, and open an issue for the better solution.
I'm writing a (failing) test for the proposal.

@joeyates
Copy link
Owner

joeyates commented Oct 7, 2015

Thanks for the input. I've used your proposed temporary fix.

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

No branches or pull requests

2 participants