Potential Issue - Returning Too many results

[CodeRed-Alpha]

Hello,

I have a select statement that seems to only work 'mo st' of the time.  I am trying to correct somebody else's coding in a proprietary application.  The function is seyup to allow the manager to select and edit a vehicle, however the Fleet Manager mentioned that this does not seems to work on certain vehicles that seemed to be entered in to the system more than once.  I believe the truck number is supposed to be Unique, but I was shown a few examples where that vehicle shows up in the dashboard multiple times, which in turn does not bring up an editable record when selected.  I think that there should be some kind of verification that restricts the results to a single(latest) record if in fact there are more than one record returned.  Does this seem like a plausible cause for this behavior?  I could be missing something entirely.  I have only had 1 day to review the code and how it interacts with the users.

 

    //case for filling out the edit truck form when a truck is clicked
    case key_exists('truckRequest', $_POST):
        $selectStatement = sprintF("SELECT truck_num, oil_due, inspect_date, active FROM Trucks WHERE truck_num = %s", $_POST['truckRequest']);
        $query = mysqli_query($web_mysql_connect,$selectStatement);
        while($queryArray = mysqli_fetch_array($query)){
            echo (json_encode($queryArray));
        }
        break;

 

[mac_gyver]

there's so much wrong with just this small section of code -

1. it is using a post method form for determining which record to edit. this should use a get method form/link, so that if the user wants to return to the same point, they can bookmark the url or if there's a problem they can submit the url so that someone else can view and see the problem.
2. the only time you should edit/update data is when correcting a mistake in a value or just changing a status value, not when performing normal data operations. for normal data operations, you should insert a new row in an appropriately defined table for every separate operation, so that you have an audit/history trail, that would let you detect if a programming mistake, accidental key press, or nefarious activity caused data to be entered and also so that you can produce a history report.
3. the use of sprintf() provides no security against sql special characters in a value from being able to break the sql query syntax, which is how sql injection is accomplished. the simplest, fool-proof way, to provide security in an sql context, for all data types, is to use a prepared query. since the mysqli extension is overly complicated and inconsistent, especially when dealing with prepared queries, this would be a good time to switch to the much simpler and more modern PDO extension.
4. the database should enforce uniqueness, by defining any column must not contain duplicate values as a unique index. this will produce a duplicate index error when attempting to insert/update duplicate values, that the error handling would detect and report back to the user that the value they tried to insert/update already exists.
5. the code should not use a loop to fetch data from a query that is expected to match at most one row of data.

based on the columns oil_due, inspect_date in a table named Trucks, the database is not designed correctly. There should be a table where the one-time/unique information about each vehicle is stored. this table will define a vehicle id (autoincrement primary index) that gets used when storing any related data. service records would then be stored in a separate table, one row per service, using the vehicle id to relate them back to the vehicle they belong to.

if there are multiple primary defining records for any vehicle, these should be found and consolidated into a single record, the columns that must be unique need to be defined as unique indexes, and the error handling for any insert/update query need to be changed to detect a unique index error number and report to the user that the data already exists.

if this Trucks table is actually the table holding the service records, there need to be a row per service operation per vehicle. the code would then query for any records that have a pending/open status value. when a service is performed, the status for that record would be updated to a value indicating it is 'complete' and what datetime it was completed. any future scheduled service would be inserted as a new row with a pending/open status value.

[Phi11W]

1 hour ago, CodeRed-Alpha said:

I believe the truck number is supposed to be Unique, but I was shown a few examples where that vehicle shows up in the dashboard multiple times

Fix the root cause of the problem (the duplicated data), not the symptom (the misbehaving application). 

As I have oft said elsewhere:

Quote

Bad Code breaks - period. 
Good Code gets broken - by Bad Data

Forget the Code and get into the Data.  
If you really do have duplicated [truck] data, then you'll likely have to do a lot of mucking about with the Application code to get something that looks right. 
Or you find and eliminate the duplicate Data and all the problems go away.  

I couple of things it might be:

It might be genuinely duplicated data. 
If so, first sort out the duplication and then put database constraints in place (unique keys/indexes) to prevent it "creeping" back in. 

It might be that the queries getting the data are somehow bringing back multiple copies of the same thing.  
Some tables use multiple-column, Composite keys (despite MySQLAdmin's best efforts to not support them) and if a query is written that omits one of those key fields in its joins, you wind up with exactly this kind of duplication. 

Regards, 
   Phill  W.

 

[ginerjm]

Duplicate truck ids?  Perhaps a number gets re-used when a vehicle ages out of use.  Maybe you need a 'status' code in the table to recognize old trucks that are only there for reference of old inventory and not for current reporting?  Or perhaps there already exists some kind of code on the table that you are not using.

[CodeRed-Alpha]

20 minutes ago, Phi11W said:

Fix the root cause of the problem (the duplicated data), not the symptom (the misbehaving application). 

As I have oft said elsewhere:

Forget the Code and get into the Data.  
If you really do have duplicated [truck] data, then you'll likely have to do a lot of mucking about with the Application code to get something that looks right. 
Or you find and eliminate the duplicate Data and all the problems go away.  

I couple of things it might be:

It might be genuinely duplicated data. 
If so, first sort out the duplication and then put database constraints in place (unique keys/indexes) to prevent it "creeping" back in. 

It might be that the queries getting the data are somehow bringing back multiple copies of the same thing.  
Some tables use multiple-column, Composite keys (despite MySQLAdmin's best efforts to not support them) and if a query is written that omits one of those key fields in its joins, you wind up with exactly this kind of duplication. 

Regards, 
   Phill  W.

 

Thank you for such a fast response.  I agree, I probably approached trying to figure out he cause of the problem incorrectly.  Unfortunately I have just started this week and will not have direct access to the MySQL Data until Monday. 

 

I completely agree that more than likely the culprit is just that the MySQL Query is most likely just not limiting the results to one as I do not see that 'limit' in the query itself.  I will also not have acccess to the Dev environment to test my theories until monday either.  I think my first step is to get to know the actual data structure and the data inside of it.  Run the current Query and see what my set of results is.  Narrow it down from there.  More than likely, if I limit the results to one record, the select to edit feature will work.  Then I also need to address why it is not recognizing that there is already a record with that Vehicle Number already entered and how to handle it then. 

 

Again, I probably reached out for help a little pre-maturely without have eyes directly on the data and the database.  Thank you for your input.  I wish I had better background info.  That will come with time and access and some more conversations with the uers themselves.  I will keep you posted next week as I gain the access to test out our findings and results. 

[ginerjm]

Why not change the query you have to include all columns and specify one of the ids that produces multiple records?  Output all of the data and see what's there that might be an indicator of why there are multiples?

[CodeRed-Alpha]

On 3/24/2023 at 9:17 AM, mac_gyver said:

there's so much wrong with just this small section of code -

1. it is using a post method form for determining which record to edit. this should use a get method form/link, so that if the user wants to return to the same point, they can bookmark the url or if there's a problem they can submit the url so that someone else can view and see the problem.
2. the only time you should edit/update data is when correcting a mistake in a value or just changing a status value, not when performing normal data operations. for normal data operations, you should insert a new row in an appropriately defined table for every separate operation, so that you have an audit/history trail, that would let you detect if a programming mistake, accidental key press, or nefarious activity caused data to be entered and also so that you can produce a history report.
3. the use of sprintf() provides no security against sql special characters in a value from being able to break the sql query syntax, which is how sql injection is accomplished. the simplest, fool-proof way, to provide security in an sql context, for all data types, is to use a prepared query. since the mysqli extension is overly complicated and inconsistent, especially when dealing with prepared queries, this would be a good time to switch to the much simpler and more modern PDO extension.
4. the database should enforce uniqueness, by defining any column must not contain duplicate values as a unique index. this will produce a duplicate index error when attempting to insert/update duplicate values, that the error handling would detect and report back to the user that the value they tried to insert/update already exists.
5. the code should not use a loop to fetch data from a query that is expected to match at most one row of data.

based on the columns oil_due, inspect_date in a table named Trucks, the database is not designed correctly. There should be a table where the one-time/unique information about each vehicle is stored. this table will define a vehicle id (autoincrement primary index) that gets used when storing any related data. service records would then be stored in a separate table, one row per service, using the vehicle id to relate them back to the vehicle they belong to.

if there are multiple primary defining records for any vehicle, these should be found and consolidated into a single record, the columns that must be unique need to be defined as unique indexes, and the error handling for any insert/update query need to be changed to detect a unique index error number and report to the user that the data already exists.

if this Trucks table is actually the table holding the service records, there need to be a row per service operation per vehicle. the code would then query for any records that have a pending/open status value. when a service is performed, the status for that record would be updated to a value indicating it is 'complete' and what datetime it was completed. any future scheduled service would be inserted as a new row with a pending/open status value.

Thank you for all of your input.  We use Sprintf() here has we only try to pass strings and we have a zero trust policy so everything will be stripped or filtered as such.  However i agree with everything that your are saying.  Much of this code was written by an employee who is no longer with us and it seems as though much of the application could use some reworking including the data structure itself. 

If I were to rewrite this app, I would separate the vehicle information and the service records table and just have them referencing each other.  I will take a deeper dive in to the dataabase today as well as the functions utilized through.