3
\$\begingroup\$

The frontend has a table with hundreds of rows. Each row consists of different inputs (text, file, checkbox etc.).

The user can change any row and any input he wants before clicking the "Save Changes" button. It then needs to update the data in the database.

I want to only send the modified data, or at least only the modified rows entirely, instead of all the rows to prevent sending of too much data (I've seen some code where the developer just wrapped the entire table in a form element and sent everything).

This is how I currently do it: I create an empty form with just the submit button ("Save Changes"), and whenever a row is modified, I add a "form" attribute with the form id, "connecting" between the modified rows and the form (source: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input#form).

Also, while creating the table on page load, I construct the inputs name attributes in a way that will connect each row to the DB row's id, for example:

name="data[{{$row->id}}][email]"

Then, when I send it to the backend, the data is organized in arrays by the ids of which I need to update in the database. Then it's simply iterating it and updating the DB:

<form id="my-form">
  <input type="submit" value="Save Changes">
</form>

<table id="my-table">
  @foreach($rows as $row)
  <tr>
    <td><input type="text" name="data[{{$row->id}}][username]" value="Foo"></td>
    <td><input type="email" name="data[{{$row->id}}][email]" value="[email protected]"></td>
    <td><input type="file" name="data[{{$row->id}}][picture]"></td>
  </tr>
  @endforeach
</table>

<script>
document.querySelector('#my-table').addEventListener("change", (event) => {
  let row = event.target.closest('tr');
  let rowInputs = row.querySelectorAll('input');
  rowInputs.forEach((input) => {
    input.setAttribute('form', 'my-form');
  });
});

document.querySelector('#my-form').addEventListener("submit", (event) => {
  event.preventDefault();
  let formData = new FormData(event.target);
  for (let pair of formData.entries()) {
    console.log(`${pair[0]}: ${pair[1]}`);
  }
});
</script>

Working example: I made a simple working example with only 2 rows and fake ids to illustrate (because the @foreach may contain hundreds of rows):

document.querySelector('#my-table').addEventListener("change", (event) => {
  let row = event.target.closest('tr');
  let rowInputs = row.querySelectorAll('input');
  rowInputs.forEach((input) => {
    input.setAttribute('form', 'my-form');
  });
});

document.querySelector('#my-form').addEventListener("submit", (event) => {
  event.preventDefault();
  let formData = new FormData(event.target);
  for (let pair of formData.entries()) {
    console.log(`${pair[0]}: ${pair[1]}`);
  }
});
<form id="my-form">
  <input type="submit" value="Save Changes">
</form>

<table id="my-table">
  <tr>
    <td><input type="text" name="data[24][username]" value="Foo"></td>
    <td><input type="email" name="data[24][email]" value="[email protected]"></td>
    <td><input type="file" name="data[24][picture]"></td>
  </tr>
  <tr>
    <td><input type="text" name="data[500][username]" value="Bar"></td>
    <td><input type="email" name="data[500][email]" value="[email protected]"></td>
    <td><input type="file" name="data[500][picture]"></td>
  </tr>
</table>

Then in the backend I can save the changes to the DB:

Note: The below code is just a partial code for better understanding of the above. Because I used Laravel syntax, I wanted to give a more "full picture" of the process.

public function save_changes(Request $request)
{
    // User is authenticated, and inputs are validated before the update

    foreach($request->all() as $id => $data)
    {
        DB::table('db_table')
        ->where('id', $id)
        ->update($data);
    }
}
\$\endgroup\$

1 Answer 1

3
\$\begingroup\$

trust

This code appears to assume the end user is already suitably authenticated and is trusted. Write down such assumptions explicitly in the source code, as comments. Defending against injection attacks appears to be out-of-scope for this code.

unique key

Offering identifiers like my-table and db_table as part of the Review Context doesn't give reviewers much to work with. I will assume we have a registered_user table, and that we had some good reason for choosing an integer surrogate PK rather than some natural key like username.

It is reasonable that one or more columns like username or email might have a UNIQUE index constraint on them. In which case it's unclear how save_changes() behaves -- it would benefit from some descriptive comments or from a "bad UPDATE" unit test.

An UPDATE can go south for many reasons, such as violating a UNIQUE constraint by enrolling two users with same username, trying to store "two" into an INTEGER num_widgets column, or trying to update a currently non-existant id number. Suppose we get a pair of rows, one "good", the other "bad". The save_changes() function does not make it clear whether a single BEGIN TRANSACTION wraps around both UPDATE attempts, or if we do a separate COMMIT per row. It should document this detail. When finished, the end user should understand which rows made it into the table and which remain uncommitted.

If user dirties N rows, it may take perceptibly longer for N commits to complete, compared with just a single commit. Consider optimistically wrapping all rows in a single transaction, and only falling back to per-row COMMIT upon seeing a failure reported.

\$\endgroup\$
1
  • \$\begingroup\$ I'm sorry you are correct. The focus was more on the way the code is being handled in the frontend. But because I'm using Laravel syntax I wanted to give the whole picture. I added the comments. User is indeed authenticated and data is validated in the backend before the update. Also I had no real reason to use the integer PK. In this case I should use another unique index like the username? (But also I would like to be reviewed on the frontend part please) \$\endgroup\$
    – pileup
    Commented Mar 11 at 6:54

Not the answer you're looking for? Browse other questions tagged or ask your own question.