Skip to content

Added On Duplicate Update Function to Database Class#3694

Merged
driusan merged 7 commits intoaces:minorfrom
HenriRabalais:addOnDuplicateKeyUpdateFunctionToDatabaseClass
Aug 23, 2018
Merged

Added On Duplicate Update Function to Database Class#3694
driusan merged 7 commits intoaces:minorfrom
HenriRabalais:addOnDuplicateKeyUpdateFunctionToDatabaseClass

Conversation

@HenriRabalais
Copy link
Copy Markdown
Collaborator

@HenriRabalais HenriRabalais commented May 28, 2018

This pull request adds two new methods to the database class: insertOnDuplicateUpdate() and unsafeInsertOnDuplicateUpdate()

These methods will attempt to insert a new row into the database given the query array passed as an argument. However, if one of the values to be inserted is a duplicate value for a unique key column, the query will be changed to an update statement for the row where the duplicate was found.

The unsafe version of the method will not escape insertion of HTML characters or JSON objects.

Reference:
https://dev.mysql.com/doc/refman/8.0/en/insert-on-duplicate.html

This PR also includes a use-case in the media module, which consolidates a few lines of code using the new function.

How to test:

  1. Go to Media module and click on the Upload tab.
  2. Upload a file according the naming convention stated at the top of the form.
  3. Confirm that the file was uploaded by finding the entry in the Media module filter.
  4. Click the edit button, edit a field (e.g. data) and click Update.
  5. Ensure that the file was properly edited by checking the entry in the Media module filter.

@HenriRabalais HenriRabalais added Category: Feature PR or issue that aims to introduce a new feature [branch] minor labels May 28, 2018
@HenriRabalais HenriRabalais requested review from driusan and ridz1208 May 28, 2018 18:14
@HenriRabalais HenriRabalais force-pushed the addOnDuplicateKeyUpdateFunctionToDatabaseClass branch from a2b5d23 to 214716f Compare May 28, 2018 19:47
PapillonMcGill
PapillonMcGill previously approved these changes Jun 4, 2018
um4r12
um4r12 previously requested changes Jun 4, 2018
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small typo: determines whether the insert throws and error -> determines whether the insert throws an error.

@nicolasbrossard
Copy link
Copy Markdown
Contributor

Let's say a call to your new method is made in these situations:

  1. Record does not exist and is inserted.
  2. Record exists and is updated.
  3. Record exists but is not updated.

What will be returned by getLastInsertID() for each case? Might be worth adding this to the documentation somewhere.

@HenriRabalais
Copy link
Copy Markdown
Collaborator Author

HenriRabalais commented Jun 4, 2018

@nicolasbrossard

Since getLastInsertID() only returns the value of the last insert to the database, the following would occur:

  1. Returns ID of newly inserted row.
  2. Does not return ID of updated row.
  3. Does not return ID of updated row.

Perhaps it would be useful to have the insertOnDuplicateKeyUpdate() function return whether the a row was updated or inserted.
@driusan what are your thoughts?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

void

@driusan
Copy link
Copy Markdown
Collaborator

driusan commented Jun 6, 2018

Does MySQL or the PDO provide any way to determine if the row was inserted or updated? I'm not sure that it's worth the effort, because I'm not sure what practical use cases it would be needed in, but if it's trivial to add, go ahead.

Assuming that there's no way (short of a second select query) to determine that, I think documenting it in the function comment is sufficient.

@HenriRabalais
Copy link
Copy Markdown
Collaborator Author

HenriRabalais commented Jun 11, 2018

Sounds good. Looks like there's ways to go about doing this, but it would required some combination of ROW_COUNT and LAST_INSERT_ID.

The MySQL documentation states:
With ON DUPLICATE KEY UPDATE, the affected-rows value per row is 1 if the row is inserted as a new row and 2 if an existing row is updated.

@HenriRabalais HenriRabalais force-pushed the addOnDuplicateKeyUpdateFunctionToDatabaseClass branch from 9b5c60f to 955d099 Compare June 11, 2018 15:52
@colezweber colezweber added the Passed manual tests PR has been successfully tested by at least one peer label Jun 18, 2018
@colezweber
Copy link
Copy Markdown
Contributor

After function rename commit, I was able to edit the date of administration and changes appeared in the module!

@miebeers miebeers self-requested a review June 18, 2018 18:41
Copy link
Copy Markdown
Contributor

@miebeers miebeers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Was able to successfully upload a file, edit a field, and see the changes reflected in the database.

@victoriafoing
Copy link
Copy Markdown
Contributor

Passed manual tests for me too.

@driusan
Copy link
Copy Markdown
Collaborator

driusan commented Jun 19, 2018

Looks good, but can you send it to 20.0-release branch so that it can be used after the next release instead of needing to wait for 20.1? I think I saw a PR to 20.0 that I wanted to recommend using this for, but then realized it's not merged there..

@driusan
Copy link
Copy Markdown
Collaborator

driusan commented Jun 19, 2018

Nevermind, I don't think it's appropriate where I thought in 20.0, so 20.1 is fine.

@zaliqarosli
Copy link
Copy Markdown
Contributor

@driusan hey dave, is this PR ready to go?


if ($onDuplicateUpdate) {
$prepQ .= ' ON DUPLICATE KEY UPDATE ';
$prepQ .= $this->_implodeAsPrepared(",", $set, $exec_params);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

from looking at line 382, should this line have a space after the comma in quotes? also, could we keep consistent with double and single quotes or is there a reason for the use of both here?

Copy link
Copy Markdown
Collaborator Author

@HenriRabalais HenriRabalais Jul 30, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, no space is necessary after the comma here. The _implodeAsPrepared function takes care of that, while _implodeWithKeys is a less specialized function that doesn't. And I left it as a double quotes because the line is identical to line 380, not for any specific functional purpose. If anything, in the context of this class, the single quotes on lines 379, 381 and 382 look out of place. Actually, the whole class is a bit of a mix tbh. But I really don't mind either way.

@driusan
Copy link
Copy Markdown
Collaborator

driusan commented Aug 23, 2018

I thought this was already merged..

@ridz1208 ridz1208 added this to the 20.1.0 milestone Sep 23, 2018
@ridz1208 ridz1208 added the Release: Add to release notes PR whose changes should be highlighted in the release notes label Dec 3, 2018
@HenriRabalais HenriRabalais deleted the addOnDuplicateKeyUpdateFunctionToDatabaseClass branch May 7, 2020 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Category: Feature PR or issue that aims to introduce a new feature Passed manual tests PR has been successfully tested by at least one peer Release: Add to release notes PR whose changes should be highlighted in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants