Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calling model::create() does an _exists() twice if the record doesn't exist #14256

Closed
maxgalbu opened this issue Jul 23, 2019 · 6 comments
Labels
Projects

Comments

@maxgalbu
Copy link
Contributor

@maxgalbu maxgalbu commented Jul 23, 2019

Expected and Actual Behavior

Calling model::create() does an _exists() twice if the record doesn't exist.

if this->_exists(metaData, this->getReadConnection()) {

let exists = this->_exists(metaData, readConnection);

Details

  • Phalcon version: (php --ri phalcon) all the way to 4.0.x from 3.0.x
  • PHP Version: (php -v) any
  • Operating System: any
  • Installation type: any
  • Zephir version (if any): none
  • Server: any
  • Other related info (Database, table schema): any

😞 😞

@niden

This comment has been minimized.

Copy link
Member

@niden niden commented Jul 23, 2019

Looking at the code it kind of makes sense because create() throws an exception if the record exists already, where save() will change to update mode if the record exists.

Not sure how to refactor this to keep on offering the same functionality. We can certainly remove create() because it does the same thing as save() but then you will lose the functionality of having an exception thrown if the record exists.

Thoughts?

@david-duncan

This comment has been minimized.

Copy link

@david-duncan david-duncan commented Jul 23, 2019

There is definitely a bug here -

create() should check the write connection.

Replica lag means that you will never be 100% correct when checking if a record exists if you check the read connection.

@niden

This comment has been minimized.

Copy link
Member

@niden niden commented Jul 23, 2019

@david-duncan good catch

@ruudboon

This comment has been minimized.

Copy link
Member

@ruudboon ruudboon commented Aug 3, 2019

Maybe we could set operationMade to OP_CREATE in the create method before calling save and skip the check if it's already defined?

@niden

This comment has been minimized.

Copy link
Member

@niden niden commented Sep 21, 2019

Resolved

@niden niden closed this Sep 21, 2019
4.0 Release automation moved this from To do to Done Sep 21, 2019
@maxgalbu

This comment has been minimized.

Copy link
Contributor Author

@maxgalbu maxgalbu commented Sep 22, 2019

Wait it still does two selects... since we do a check on the write connection, the check on the read one is not useful... this will slow down the app if there are a lot of inserts

niden added a commit that referenced this issue Sep 22, 2019
* 4.0.x: (202 commits)
  Updated changelog
  Changing the interface to return null for getEventsManager
  Correcting interface
  Correcting tests
  Fixed Firewall\Adapter\AbstractAdapter::setEventsManager
  Remove void return type for constructors
  Fixed return types for various methods to satisfy interface declaration
  Constructors should never declare return type [skip appveyor]
  Updated changelog
  Styling fix changelog
  Fixed remove() not removing service #14396
  Forgot to push changelog
  Use write connection on create to prevent replica lag. Fix #14256
  Added license information in README
  Moved license files to 3rdparty/license. Added more licenses
  Update issue templates
  Backup old template file
  Update issue templates
  [4.0.x] - Corrected test
  [4.0.x] - Fixing stupid error
  ...
sergeyklay added a commit that referenced this issue Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4.0 Release
  
Done
4 participants
You can’t perform that action at this time.