db.mysql `query` and `real_query` can return null objects but there is no function to check for them.

[einar-hjortdal]

Describe the bug

query and real_query return a !Result type correctly: an error if there is a database error or a Result object when there are no errors.
The Result object can contain no data, when the statement return no result sets, or an empty set:

mysql.Result{
    result: &nil
}

There currently is no way of checking whether the returned object is null or not. Attempting to use methods on the result, such as Result.rows() causes a runtime memory access error.

Expected Behavior

Some options:

1. A function such as Result.is_null() should be available
2. A boolean field Result.is_null should exist
3. Existing functions should return empty objects: for example Result.rows() should return an empty []Row.

Current Behavior

mysql.Result.rows() can cause memory access errors

Reproduction Steps

use real_query to select something, provide a where clause that will not match anything. Then use rows() on the result.

res := mysql.real_query('SELECT column FROM table WHERE id = 69')
runtime_memory_access_error := res.rows()

Possible Solution

No response

Additional Information/Context

No response

V version

Current V version: V 0.4.0 3211a65, timestamp: 2023-08-09 15:05:17 +0300

Environment details (OS name and version, etc.)

V full version: V 0.4.0 ee4150f.3211a65
OS: linux, Linux version 5.14.0-284.25.1.el9_2.x86_64 (mockbuild@pp-el9-x86) (gcc (GCC) 11.3.1 20221121 (Red Hat 11.3.1-4), GNU ld version 2.35.2-37.el9) #1 SMP PREEMPT_DYNAMIC Wed Aug 2 09:19:04 UTC 2023
Processor: 4 cpus, 64bit, little endian, Intel(R) Core(TM) i3-5005U CPU @ 2.00GHz

getwd: /home/coachonko/Documents/projects/vlang/peony
vexe: /home/coachonko/.local/lib64/v/v
vexe mtime: 2023-08-09 19:18:34

vroot: OK, value: /home/coachonko/.local/lib64/v
VMODULES: OK, value: /home/coachonko/.vmodules
VTMP: OK, value: /tmp/v_1000

Git version: git version 2.39.3
Git vroot status: weekly.2023.30-103-g3211a653
.git/config present: true

CC version: cc (GCC) 11.3.1 20221121 (Red Hat 11.3.1-4)
thirdparty/tcc status: thirdparty-linux-amd64 12f392c3

[JalonSolov]

The simple workaround is to compare the return to an empty instance, like so:

if res == mysql.Result{} {
    // no results
} else {
    // some results
}

However, these routines should probably be changed to return an Option or a Result (whichever makes the most sense).

[jacksonmowry]

From the docs for db.mysql there appears to be a method of result called n_rows() which returns the number of rows in a result as a u64 (note that the term result here is refering to the db.mysql struct called Result, not the result used in V.

Currently, as stated above, calling any methods on an empty Result (including n_rows() causes a runtime memory access error. I see one of two solutions working here:

1. A query that returns 0 rows should return 1 empty row. This is probably harder to implement just because this is a wrapper around a C struct, but as @Coachonko suggested above we could move this step into the .rows() method to avoid messing with C structs.
2. n_rows() should have a special case where it checks for an empty Result (as @JalonSolov showed above) and returns 0 if this is the case.

This is hard because db.pg and db.sqlite return []Row directly without the result wrapper.

[JalonSolov]

Your PR #19093 fixes that for sqlite. We just need it fixed for the others.

[jacksonmowry]

I think it is a similar fix but not exactly the same. I am unfamiliar with mysql but it appears that the C API returns a Result struct which contains all of the data within, whereas sqlite and postgres return []Row directly. If we are trying to keep the usage similar we can just make it so that the .rows() method returns an empty row, which would be the same as sqlite and postgres.

This is how I check for an empty query result in sqlite

module main

import db.sqlite

fn main() {
	db := sqlite.connect(':memory:')!
	db.exec('create table users (
		    id integer primary key,
		    username text
		)')!

	users := db.exec('select * from users')!
	if users.len == 0 {
		println('empty')
	} else {
		println(users)
	}
}

So if we just rows() return []Row{} for an empty result in db.mysql we can use the same pattern. I'm thinking something like:

module main

import db.mysql

fn main() {
	mut connection := mysql.Connection{
		username: 'root'
		dbname: 'mysql'
	}
	connection.connect()!
	connection.select_db('db_users')!
	users := connection.query('SELECT * FROM users')!
	rows := users.rows()
	if rows.len == 0 {
		println('empty')
	} else {
		println(rows)
	}
}

All we would have to change in this case is .rows()

// rows returns array of rows, each containing an array of values,
// one for each column.
pub fn (r Result) rows() []Row {
	// NEW CODE HERE
	if r == mysql.Result{} {
		return []Row{}
	}
	// NEW CODE HERE

	mut rows := []Row{}
	nr_cols := r.n_fields()
	for rr := r.fetch_row(); rr; rr = r.fetch_row() {
		mut row := Row{}
		for i in 0 .. nr_cols {
			if unsafe { rr[i] == 0 } {
				row.vals << ''
			} else {
				row.vals << mystring(unsafe { &u8(rr[i]) })
			}
		}
		rows << row
	}
	return rows
}

Of course we'd have to change the entire API around a mysql.Result but this is just a basic example to see if this is the right direction. If we think this is a good idea I can start working on a PR to implement this.

[jacksonmowry]

Or would we even be better off bringing this up to a higher level abstraction with a method called exec that would return ![]Row? That way it can handle all of the MySQL stuff under the hood for those who don't care about it and just want []Row back after executing a query. Once again I don't know MySQL so maybe this doesn't make sense. Just trying to think of approachable API's for V.

This way we can leave query in place for those who want it, and we can introduce exec as a consistent approachable method across different databases.

[JalonSolov]

I would vote for making things consistent across all the db.* modules. That way the user can switch which database they use, without having to rewrite their code other than changing the db.* names where used. They wouldn't have to change the logic, or which functions are called, etc.

[spytheman]

When the mysql was written initially, the goal was just a quick and thin wrapper around what libmysqlclient had, thus the:

pub struct Result {
    result &C.MYSQL_RES = unsafe { nil }
}

A higher level API, similar to what db.pg and db.sqlite have, will be very nice to have, and easier to use for most people:

vlib/db/sqlite/sqlite.v:212:pub fn (db &DB) exec(query string) ([]Row, int) {
vlib/db/sqlite/sqlite.v:245:pub fn (db &DB) exec_one(query string) !Row {
vlib/db/sqlite/sqlite.v:280:pub fn (db &DB) exec_none(query string) int {
vlib/db/sqlite/sqlite.v:290:pub fn (db &DB) exec_param_many(query string, params []string) ![]Row {
vlib/db/sqlite/sqlite.v:341:pub fn (db &DB) exec_param(query string, param string) ![]Row {

[spytheman]

we'd have to change the entire API around a mysql.Result

imho, leave the current API as it is when possible, just implement the more convenient set of new functions.

We can just mark all the stuff, that returns mysql.Result with [unsafe], to nudge people to use the more convenient set.

[spytheman]

Also at the time, the mut and pub modifiers were buggy, so there was no problem of accessing x.result in other modules too.

I see no problem with changing the declaration to:

pub struct Result {
pub:
    result &C.MYSQL_RES = unsafe { nil }
}

which will solve this concrete issue.

[einar-hjortdal]

I also agree that there could be a higher-level api that is consistent across mysql, postgres and sqlite, similar to the way Go handles it.