Postgres numeric check needs to happen after precision check in schema generation
stormsweeper - July 17, 2009 - 19:28
| Project: | Drupal |
| Version: | 7.x-dev |
| Component: | postgresql database |
| Category: | bug report |
| Priority: | critical |
| Assigned: | Unassigned |
| Status: | needs review |
| Issue tags: | PostgreSQL Surge |
Description
The current function:
<?php
protected function createFieldSql($name, $spec) {
...
if (!empty($spec['unsigned'])) {
$sql .= " CHECK ($name >= 0)";
}
if (!empty($spec['length'])) {
$sql .= '(' . $spec['length'] . ')';
}
elseif (isset($spec['precision']) && isset($spec['scale'])) {
$sql .= '(' . $spec['precision'] . ', ' . $spec['scale'] . ')';
}
...
}
?>So what happens is if you define a schema element of type numeric with a precision and scale specified, and unsigned, you get this :
colname numeric CHECK (colname >= 0)(10, 2)The patch switches these two so you get the expected result of
colname numeric(10, 2) CHECK (colname >= 0)| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| pgsql-unsigned.D7.patch | 1.09 KB | Idle | Failed: Failed to install HEAD. | View details | Re-test |
#1
Marking as postgres specific
#2
This type of things scream for a test. Would it be possible to extend the Schema API tests to include this case?
#3
Let me see what tests are there now, although I don't think testbot uses Postgres.
#4
Added a new test that creates a table with unsigned columns of each number type, and then tries to insert a negative value in all of them. The test is db agnostic.
#5
Now let's see if the test bot chokes on this.
#6
Not sure what I did, but it looks like the new test broke the Schema API test class so bad that it didn't load (see the detailed results). Will chew through this.
#7
forgot to call function as a method. fixed, this patch works, Thanks.
#8
Couple of minor things with the test:
+ * Tests creating unsigned columns and that those columns reject+ * negative values on insert.
Please rephrase so this fits within 80 chars.
+ // set up unsigned schema with just serial columnAll comments should be written "With sentence capitalization, ending in a period." There are a few lines like this.
+ 'fields' => array('serial_col' => array('type' => 'serial', 'unsigned' => TRUE, 'not null' => TRUE)),...
+ $col_spec = array('type' => $type, 'unsigned'=> TRUE);
...
+ * @param $col
etc.
We don't abbreviate variable, etc. names. Please spell it out as "column".
#9
Working on it now. Is there a compiled addendum anywhere for test coding standards over and above the standard Drupal ones?
#10
Lightly refactored the patch, which included catching a small bug with the inserts.
#11
Also, there's already a D6 version of this patch here: #256001: pgsql driver does not handle unsigned numeric fields
#12
I missed the inline comments before.
#13
The last submitted patch failed testing.
#14
I'm going to wait a bit and set this back to CNR. The current HEAD of Drupal is broken, it seems.
#15
OK, sorted out the update, but the same three asserts fail on a clean checkout of HEAD. They're all string comparisons, but as far as I can tell the text has not changed:
In modules/system/system.test, lines 118-122:
<?php
$this->assertText(t('The configuration options have been saved.'), t('Modules status has been updated.'));
// Check that hook_modules_installed and hook_modules_enabled hooks were invoked and check tables.
$this->assertText(t('hook_modules_installed fired for aggregator'), t('hook_modules_installed fired.'));
$this->assertText(t('hook_modules_enabled fired for aggregator'), t('hook_modules_enabled fired.'));
?>
The module is enabled just fine, and every other assert passes. I'll check back tomorrow to set this to CNR, as I am certain the patch is not at fault.
#16
is testbot fixed now?
#17
Oh, and I think this was the cause of the last fails: #554106: Module enable/disable tests failing on testing servers
#18
Just marked #256001: pgsql driver does not handle unsigned numeric fields postponed on this one, since that is the same patch for Drupal 6.
#19
I like it. Lets get this in.
#20
The last submitted patch failed testing.
#21
It looks like another test failed before it got to the ones in here.
#22
Josh Waihi requested that failed test be re-tested.