Development

Wiki Start
Wishlist
Timeline
Roadmap
Browse Source
New Ticket
View Tickets

Ticket #387 (assigned enhancement)

Opened 1 year ago

Last modified 3 months ago

wildcard select & num index for column

Reported by: joximu Assigned to: raphael (accepted)
Priority: blocker Milestone: ispCP ω 1.1.0
Component: Backend (Engine) Version: ispCP ω 1.0.0 - DEV
Severity: Hard Keywords: wildcard select and numeric column index
Cc:

Description

Having a lok at the problem from here http://www.isp-control.net/forum/errors-after-update-to-latest-trunk-t-766.html I realized that the handling of the mysql selects and results is not very friendly for extensions. Example: add a new column to the domain table, eg. "myflag" defaulting to "no". Now add a new domain/user - in (apache)ispcp.conf and the zonefile of the new domain there will be "no" instead of the IP address. The reason: in ispcp-dmn-mngr (function dmn_mngr_engine) there is a mysql select and the handling of the result (picked out some commands in order): 

$sql = "select t1.*, t2.ip_number from domain as t1, server_ips as t2 where ..."
($rs, $rows) = doSQL($sql);
my $entry = @$rows[0];
$rs = dmn_add_data($entry);
# in "dmn_add_data" the argument is given over to other functions
# also to dmn_add_named_db_data:
my ($dmn_data) = @_;
my $dmn_ip = @$dmn_data[21];

ok - the 21st field/column of the first result row is the IP address... not in our case with the new field....!

This really should be changed/improved. You never can rely on the order or the number of columns in a table (at least not if there is no check done if the order and numer still is as assumed) - a very basic database rule..

I put it to release 1.1 - for 1.0 we can write something about not extending the tables but as soon as there are people who want to extend ispcp this will be a bigger problem.

Attachments

ticket_387.diff (3.9 kB) - added by joximu on 06/18/2007 12:30:31 AM.
the diff file (diff -ur engine_654 engine)

Change History

06/11/2007 01:06:46 AM changed by rats

Remember it's perl and no one in the current team is that great in coding perl... If you are, feel free, to join us. I see no way to address mysql in another way than the numeric in perl.

06/11/2007 09:22:18 AM changed by joximu

Ok, I'll try this and hope to bring some light in the darkness :-)

(follow-up: ↓ 4 ) 06/11/2007 02:19:10 PM changed by joximu

Hi rats

ok, either the select should be changed so that all needed fields are written, eg: 

SELECT field1, field2, domain, domain_id, ...., field21 FROM...

or there should be a second "doSQL()" which creates hash-arrays: from "man dbi":

$ary_ref = $dbh->selectall_arrayref($statement); $hash_ref = $dbh->selectall_hashref($statement, $key_field);

at the moment we use the selectall_arrayref (ispcp_common_code.pl, line 255 and 259)...

Which solution do you favorite? I think the first one is faster to implement...

/Joximu

(in reply to: ↑ 3 ) 06/16/2007 08:47:36 PM changed by raphael

Replying to joximu:

ok, either the select should be changed so that all needed fields are written, eg: {{{ SELECT field1, field2, domain, domain_id, ...., field21 FROM... }}}

I prefer this one ;-)

06/18/2007 12:29:36 AM changed by joximu

I changed 4 files in engine from "SELECT t1.*, t2.*..." to "SELECT t1.domain_id, t1.domain_name...." so that a new filed not automatically is selected but at least the numerical result index stay the same...

A change to hash is not easy because the dpi:selectall_hashref needs a second parameter with the fields again... - a lot of work. I'd rather have a closer look at all of the select statements - I think that alot of the statements are not necessary or could be shortened...

06/18/2007 12:30:31 AM changed by joximu

  • attachment ticket_387.diff added.

the diff file (diff -ur engine_654 engine)

07/09/2007 01:04:25 AM changed by raphael

I completely forgot about this ticket.

As you can see in the DBD::mysql's documentation there's no need to use [0], [1], etc. See: 

my $sth = $dbh->prepare("SELECT * FROM foo");
  $sth->execute();
  while (my $ref = $sth->fetchrow_hashref()) {
    print "Found a row: id = $ref->{'id'}, name = $ref->{'name'}\n";
  }
  $sth->finish();

More info:
http://search.cpan.org/~timb/DBI/DBI.pm#selectall_arrayref[[BR]] http://search.cpan.org/~timb/DBI/DBI.pm#selectall_hashref[[BR]]

Right now doSQL uses selectall_arrayref when the query is a SELECT or SHOW.

07/09/2007 09:13:30 PM changed by joximu

Ok, so we could do the "better" solution...

Now, our favorite is the other one (compared to comment 4)?

Did you have a look at my patch? It's the solution where all fields are written explicitly in the SELECT statement.

I'll do a try with the "while (my $ref = $sth->fetchrow_hashref())", this is a "nicer" way :-)

Right?

07/09/2007 09:52:09 PM changed by raphael

I couldn't take a good look at selectall_arrayref/selectall_hashref's documentation... but maybe using selectall_hashref in doSQL instead of selectall_arrayref should be enough to be able to use $rows{'domain_name'} instead of $rows[7] and so on.

I did take a look at your patch but I decided to go a little further and find a real, good, solution for this problem instead of the current way things are done.

07/09/2007 09:52:38 PM changed by raphael

  • milestone changed from ispCP ω 1.1.0 to ispCP ω 1.0.0 - RC3.

07/10/2007 12:35:12 AM changed by joximu

Hi Well, I support this. It's only... there are *many* sql statements and use of doSQL - so the change will take a while because the results are used widespread...

That's why I rather would make a new sub/function for doSQL, doHashSQL or something like this. Then a move to hash based queries and results is not that tricky. When it's finished we can kick the old doSQL function...

(The way from rc2 to rc3 is one of the biggest step for this project... :-)

07/18/2007 09:29:35 PM changed by anonymous

Ok, just added doHashSQL.

07/31/2007 05:02:51 PM changed by BeNe

What about it ?
Looks like joximu is busy and has not enough time :-(

07/31/2007 07:45:48 PM changed by raphael

I'll try to test switching from doSQL to doHashSQL. But collaboration is always very welcomed ;)

08/03/2007 01:41:33 PM changed by malte

i think it would be enough to just remove the * and replace it with the used fields in the right order.

I know this is not a smart solution - but it helps all those people who want to add a new field somewhere in the domains table. And since we've to rewrite most parts of the daemon for 1.1 i think to provide a stable release its enough to do it that way.

08/03/2007 06:46:04 PM changed by raphael

  • owner set to raphael.
  • status changed from new to assigned.

I will use the patch Joximu contributed to make the engine use hash-results. (collaboration is well appreciated)

08/16/2007 03:18:14 AM changed by raphael

Joximu: I just applied your patch

08/17/2007 02:04:04 PM changed by rats

is it completed or not? If not, which files are missing?

09/01/2007 05:38:56 PM changed by BeNe

Maybe someone should contact Joximu via PM
he is busy the last time. So that we have enough infos about it.
Or was this reply for raphael ?

09/02/2007 09:04:59 PM changed by rats

  • priority changed from major to minor.

if it's not possible to realize it in time, we have to move it to a date after 1.0 release; It's not critical for release!

09/03/2007 01:26:03 AM changed by joximu

I try to spend some time on this - but it's not a 2-minute thing :-)

09/07/2007 02:23:02 PM changed by BeNe

Move it to another Milestone!
If joximu is is ready it can be updated in the trunk - if not - it is to fix in 1.0 or later.

09/20/2007 01:26:59 AM changed by Joximu

My Perl complains:

Usage: $h->selectall_hashref($statement, $keyfield [, \%attr [, @bind_params ] ]) at /var/www/ispcp/engine//ispcp_common_methods.pl line 334.

with selectall_hashref you need at least 2 parameters: the sql statement and a key field or key array...

This key filed has to be one of the selected fields. Since we don't know this - we need a second parameter for doHashSQL. Right?

Anyone knowing more about this?

Joximu

09/23/2007 11:22:43 PM changed by joximu

Move this ticket to > 1.0

It's not absolutely needed for now - it'd bee very cool but not needed for now. It'd require a retest of all engine functions...

This is my opinion.

10/01/2007 09:53:13 AM changed by rats

  • milestone changed from ispCP ω 1.0.0 - RC3 to ispCP ω 1.1.0.

finally moved to 1.1.0; engine has to be rewritten!

02/17/2008 02:44:24 PM changed by rats

  • severity set to Hard.
  • milestone changed from ispCP ω 1.1.0 to Working.

02/18/2008 12:15:05 AM changed by rats

  • priority changed from minor to blocker.
  • version changed from ispCP ω 1.0.0 - RC2 to ispCP ω 1.0.0 - DEV.
  • milestone changed from Working to ispCP ω 1.1.0.

Add/Change #387 (wildcard select & num index for column)