MySQL query to select messages related to a user

Question

Could someone please check my query and the table structures and let me know if there is any way to improve it.

I would like some tips for my own personal development

Query in PHP:

// Load all records so can be displayed in list
function load_all($page = NULL, $chat_type = NULL){
 if(!$page)$page = 1;
 $start = ($page - 1) * ITEMS_PER_PAGE_ADMIN;
 $query = "";
 $query .= "SELECT direct_message.*, IFNULL(direct_message_group.staff_id, 0) as staff_id FROM `direct_message`
 LEFT JOIN direct_message_group ON direct_message_group.chat_id = direct_message.id
 INNER JOIN direct_message_thread ON direct_message_thread.chat_id = direct_message.id
 WHERE";
 $query .= " (
 (
 direct_message.recipient_id = '".addslashes(session::getAdministratorStaff()->id)."' 
 OR direct_message.creator_id = '".addslashes(session::getAdministratorStaff()->id)."'
 OR (
 direct_message_group.staff_id = '".addslashes(session::getAdministratorStaff()->id)."'
 AND direct_message_group.active = '0'
 )
 )
 )";
 $query .= " AND direct_message.school_id = '".addslashes(session::getAdministratorStaffSchoolID())."'
 AND direct_message_thread.school_id = direct_message.school_id
 GROUP BY direct_message.id
 ORDER BY direct_message_thread.inserted DESC LIMIT $start, ".ITEMS_PER_PAGE_ADMIN;
 $rows = database::select($query);
 foreach($rows as $row){
 $holding = new direct_message();
 $holding->_populate($row);
 $this->direct_messages[] = $holding;
 }
}

Table structures:

enter image description here

---------------------------------------------------

enter image description here

---------------------------------------------------

enter image description here

• 200_success: It's not obvious what table each screenshot represents.
• I wrestled a bear once.: if your question is about an sql query then remove the php code and the php tag and just post the query.
• Conor Mancone: Some comments/questions before attempting an actual answer. First, are you aware that addslashes is not really the best way of preventing SQL injection? You will be much better off using prepared statements. There are some real world cases where addslashes will fail, see this: stackoverflow.com/questions/860954/…. Also, you should know that with MySQL you can't index an OR condition. Fixing this may involve reworking large parts of your application to really fix it. Are you ready for that?
• I wrestled a bear once.: @connor - if he's storing user input - or anything else that needs to be escaped (or prepared) in a column called "id" then he's got deeper problems. if the id is an int as it should be then all he has to do is make sure it's numeric and get on with his life - no escaping required.
• YaBCK: @conor-mancone Would you be able to give me an example of getting what I want without the OR conditions?

Answer 1

Some thoughts:

• You should consider using prepared statements.
• Remove $chat_type as parameter, as it is not used
• Consider stronger validation on $page_type() parameters(like that is it null or integer). Right now you can easily break the function
• Don't use * selects. They don not help the code reader understand what sort of data is being returned without having to look a database. They also tend to cause more bandwidth consumption than is necessary and make your queries fragile. What if you add a new audit field to the database table? That could then break your object instantiation later in the code if an unexpected field shows up or cause you to go modify that object's class for some reason that is unrelated to what that object does.
• Consider COALESCE instead of IFNULL for determining conditional values (this is more suitable function in that it does not have potentially hidden value typing behavior). This is probably more common as well amongst MySQL developer types.
• Write your query in a more readable form (not a series of concatenations, drop unnecessary parenthesis, ne consistent around using backticks, and capitalization of reserved words, etc.)

For example:

SELECT
 direct_message.{field} AS {field_alias},
 {other fields you need} AS {field_aliases}, 
 COALESCE (direct_message_group.staff_id, 0) AS staff_id
FROM direct_message
LEFT JOIN direct_message_group
 ON direct_message_group.chat_id = direct_message.id
INNER JOIN direct_message_thread
 ON direct_message_thread.chat_id = direct_message.id
WHERE
 (
 direct_message.recipient_id = ?
 OR direct_message.creator_id = ? 
 OR (
 direct_message_group.staff_id = ?
 AND direct_message_group.active = '0'
 )
 )
 AND direct_message.school_id = ?
 AND direct_message_thread.school_id = direct_message.school_id
GROUP BY direct_message.id
ORDER BY direct_message_thread.inserted DESC
LIMIT ?, ?

Here I have applied named parameters instead of you string concatenation.

• You query makes you relationship between tables seem unclear. On one hand you join direct_message to direct_message_thread on some sort of chat id value, yet you also put in a WHERE condition that school id's for these records on these tables must be equal. Should this just be a part of the join condition as opposed to WHERE clause filter?
• addslashes() may not be appropriate for escape purposes and it is not being used to escape $start (offset value). I would suggest you either use prepared statements or an appropriate DB-connector-specific escape functionality. Also, if you know the values of your class methods calls are going to be integers, for example, do you really need to escape these?
• You object instantiation for direct_message class seems weird. Why not pass the row data upon instantiation rather than having to call separate _populate() method with the data? Why start that method name with an underscore? Consider UpperFirstCamelCase for all class/interface names instead of snake_case.

• Conor Mancone: It's worth noting that your example uses bound parameters with the same name. This can cause errors in some backends: stackoverflow.com/questions/18511645/… Also, bound parameters on the LIMIT clause only works when using emulated queries (which isn't usually the recommendation), and can cause SQL errors in some edge cases: stackoverflow.com/questions/2269840/…
• Mike Brant: @ConorMancone With regard to emulated prepares and LIMIT, I believe it is actually the opposite of what you state, that when emulating prepared statements with PDO you have to explicitly bind integer value to parameter being used in limit clause. Either way, I have just gone ahead and edited my answer to use ? to remove ambiguity. Thanks for comment.
• Conor Mancone: Regarding the limit, you're right, I got those backwards. Thanks!