pkp/pkp-lib#2163 Fix PHP Notices for missing article columns

[NateWr]

@asmecher Ok here's a proper fix for the missing article columns that uses the query builder. All credit to @kaschioudi on this one as he had to lift me over all the SQL hurdles. :)

[asmecher]

Doesn't this defeat string escaping? (Also below)

[NateWr]

It does. Unfortunately, the bound params get compiled out of order otherwise, so we lose some of the benefits of the query builder here. But... (see below)

[asmecher]

I think this one would work by just changing out Capsule::raw("'{$locale}'") for $locale. (And then you wouldn't have to worry about malicious values of $locale breaking out of their cages using ' characters embedded within.)

[NateWr]

When I do that, I get the following error:

DB Error: Unknown column 'en_US' in 'on clause'

This comment walks through the debug process on that line and how we ended up there if that gives any clue to what's going on: #1483 (comment)

[asmecher]

It looks to me like heavy use of Capsule::raw essentially reverts us to gluing together SQL strings, negating the usefulness of the query builder, no?

[NateWr]

We do lose much of the expressiveness of the query builder. But it does allow us to piece together bits of a query out of order, which is really helpful for building extensible queries to share code between OJS/OMP, and to allow plugins to hook in.

This method of gluing SQL strings together seems easier and less fragile to me than direct string manipulations. But I'd be open to an alternate approach. I just don't think I'm the one to formulate that.

[asmecher]

Let's talk about this during the dev call. Everywhere where we need to split SQL construction between classes (most typically app/pkp-lib) it's currently spaghetti, so this is an improvement -- but it might be nice to find a way to establish this new pattern with less use of the raw construct.

[asmecher]

@NateWr, can you walk me through an example of where the ordering is important?

[NateWr]

@asmecher Ok I tried to come up with a reproducible case of the issue. I have to say I don't really understand what's going on here too well. But let's take the following code here:

		$q->leftJoin('section_settings as stpl', function($join) use($primaryLocale) {
			$join->on('s.section_id', '=', Capsule::raw("'stpl.section_id'"));
			$join->on('stpl.setting_name', '=', Capsule::raw("'section_title'"));
			$join->on('stpl.locale', '=', Capsule::raw("'{$primaryLocale}'"));
		});

My first attempt looked like this:

		$q->leftJoin('section_settings as stpl', function($join) use($primaryLocale) {
			$join->on('s.section_id', '=', 'stpl.section_id');
			$join->where('stpl.setting_name', '=', 'section_title');
			$join->where('stpl.locale', '=', $primaryLocale);
		});

If you look at the SQL query and the bound params, they look like this:

select `s`.*, COALESCE(stl.setting_value, stpl.setting_value) AS section_title, COALESCE(sal.setting_value, sapl.setting_value) AS section_abbrev from `submissions` as `s` left join `section_settings` as `stpl` on `s`.`section_id` = `stpl`.`section_id` and `stpl`.`setting_name` = ? and `stpl`.`locale` = ? left join `section_settings` as `stl` on `s`.`section_id` = 'stl.section_id' and `stl`.`setting_name` = 'section_title' and `stl`.`locale` = 'en_US' left join `section_settings` as `sapl` on `s`.`section_id` = 'sapl.section_id' and `sapl`.`setting_name` = 'section_abbrev' and `sapl`.`locale` = 'en_US' left join `section_settings` as `sal` on `s`.`section_id` = 'sal.section_id' and `sal`.`setting_name` = 'section_abbrev' and `sal`.`locale` = 'en_US' where `s`.`context_id` = ? and `s`.`stage_id` in (?, ?, ?) and `s`.`section_id` in (?) group by `s`.`submission_id`, COALESCE(stl.setting_value, stpl.setting_value), COALESCE(sal.setting_value, sapl.setting_value) order by `s`.`date_submitted` desc

Array
(
    [0] => 1
    [1] => 1
    [2] => 2
    [3] => 3
    [4] => section_title
    [5] => en_US
    [6] => 2
)

So the $join->where statements were getting set as bound parameters, but were out of order with the eventual compilation of the query string.

The fix was to use multiple $join->on statements, like this:

		$q->leftJoin('section_settings as stpl', function($join) use($primaryLocale) {
			$join->on('s.section_id', '=', 'stpl.section_id');
			$join->on('stpl.setting_name', '=', 'section_title');
			$join->on('stpl.locale', '=', $primaryLocale);
		});

For some reason, those on statements don't get set as bound parameters. They get rendered into the SQL statement. It's not clear to me whether the parameter binding/sanitizing is happening within the leftJoin, so it's still sanitizing the parameter. Of it it's just ignoring it and not doing any binding/sanitizing on it. That leads the the following SQL and params output:

select `s`.*, COALESCE(stl.setting_value, stpl.setting_value) AS section_title, COALESCE(sal.setting_value, sapl.setting_value) AS section_abbrev from `submissions` as `s` left join `section_settings` as `stpl` on `s`.`section_id` = `stpl`.`section_id` and `stpl`.`setting_name` = `section_title` and `stpl`.`locale` = `en_US` left join `section_settings` as `stl` on `s`.`section_id` = 'stl.section_id' and `stl`.`setting_name` = 'section_title' and `stl`.`locale` = 'en_US' left join `section_settings` as `sapl` on `s`.`section_id` = 'sapl.section_id' and `sapl`.`setting_name` = 'section_abbrev' and `sapl`.`locale` = 'en_US' left join `section_settings` as `sal` on `s`.`section_id` = 'sal.section_id' and `sal`.`setting_name` = 'section_abbrev' and `sal`.`locale` = 'en_US' where `s`.`context_id` = ? and `s`.`stage_id` in (?, ?, ?) and `s`.`section_id` in (?) group by `s`.`submission_id`, COALESCE(stl.setting_value, stpl.setting_value), COALESCE(sal.setting_value, sapl.setting_value) order by `s`.`date_submitted` desc

Array
(
    [0] => 1
    [1] => 1
    [2] => 2
    [3] => 3
    [4] => 2
)

However, that leads to an OJS database error:

<h1>DB Error: Unknown column 'section_title' in 'on clause'</h1>

Finally, Kassim recommended the use of Capsule::raw, which seems to fix that problem:

		$q->leftJoin('section_settings as stpl', function($join) use($primaryLocale) {
			$join->on('s.section_id', '=', Capsule::raw("'stpl.section_id'"));
			$join->on('stpl.setting_name', '=', Capsule::raw("'section_title'"));
			$join->on('stpl.locale', '=', Capsule::raw("'{$primaryLocale}'"));
		});

So it's not clear to me if this is really a case where order is causing problems. Maybe where methods are to be avoided inside leftJoin closures. It's not the only place I've run into this issue, but it's not exactly clear to me when the params get bound of order or why.

Maybe @kaschioudi has a better idea of a more specific example or use-case to illustrate the issue?

[asmecher]

Wrapping column names in Capsule::raw is OK when it's just a column name -- it's more when whole clauses of SQL get wrapped in it that we lose the benefits of the query builder, I think.

[NateWr]

Yeah, so I think this is the largest chunk that gets passed through Capsule::raw:

$this->columns[] = Capsule::raw('COALESCE(stl.setting_value, stpl.setting_value) AS section_title');

When I take that out of the Capsule::raw it tells me:

<h1>DB Error: Unknown column 'section_title' in 'on clause'</h1>

Maybe you or @kaschioudi have an idea for getting around this, but it's not entirely clear to me what the issue is, or why wrapping it in Capsule::raw solves the problem.

[asmecher]

I think that's OK to wrap Capsule::raw around this example, as I suspect it'll try to interpret it as a string value or something without the wrapper.

[asmecher]

Thinking broadly about the division of SQL between the DAOs and the service layer classes, what is the end-game for the DAOs? For example, would they retain insert/delete/fetch, leaving the more business-logic-intensive joins to service classes? (That's as much for @kaschioudi as anyone)

[NateWr]

I'll defer to @kaschioudi on this one. :)

[asmecher]

@kaschioudi, pinging you on the above comment!

[kaschioudi]

Thanks Alec for the reminder. I totally forgot about this. So here's my point of view.

Generally services are helper objects which handle complex business logic usually by controlling multiple domain objects. For OJS that means Services should orchestrate interactions between group of DAO and/or query builders. CRUD and simple operations remain on DAOs and complex queries go into entity query builders. Once we replace ADODB (pkp/pkp-lib#2493), we will probably introduce model (ActiveRecord implementation) objects which will centralize CRUD and complex queries under a single class.

[asmecher]

Thanks, that makes sense -- and provides a good place for cross-object queries to live, which we've never had before.

[NateWr]

@kaschioudi and @asmecher I'm running into some problems with this line and 63, 69 and 75 below. When written like the following, it passes mysql tests but fails postgres tests:

'`stpl`.`section_id`'

Same with:

"`stpl`.`section_id`"

I've tried other things which pass postgres but cause problems with mysql. I've tried:

'"stpl"."section_id"'

And

"'stpl'.'section_id'"

And

'stpl.section_id'

Do either of you have an idea for what the syntax should be for this to pass muster with postgres and mysql?

[asmecher]

Looking into this now...

[asmecher]

I don't think any quoting is needed here. This works for me with PostgreSQL: asmecher@9ca1c8b