| Index: branches/RL2/extensions/Gadgets/backend/LocalGadgetRepo.php |
| — | — | @@ -42,83 +42,50 @@ |
| 43 | 43 | return true; |
| 44 | 44 | } |
| 45 | 45 | |
| 46 | | - public function addGadget( Gadget $gadget ) { |
| | 46 | + public function modifyGadget( Gadget $gadget, $timestamp = null ) { |
| 47 | 47 | if ( !$this->isWriteable() ) { |
| 48 | 48 | return Status::newFatal( 'gadget-manager-readonly-repository' ); |
| 49 | 49 | } |
| 50 | 50 | |
| 51 | | - // Try to detect a naming conflict beforehand |
| | 51 | + $dbw = $this->getMasterDB(); |
| 52 | 52 | $this->loadData(); |
| 53 | 53 | $name = $gadget->getName(); |
| 54 | | - if ( isset( $this->data[$name] ) ) { |
| 55 | | - return Status::newFatal( 'gadget-manager-create-exists', $name ); |
| 56 | | - } |
| 57 | | - |
| 58 | 54 | $json = $gadget->getJSON(); |
| 59 | | - $dbw = $this->getMasterDB(); |
| 60 | | - $ts = $dbw->timestamp(); |
| 61 | | - // Use INSERT IGNORE so we don't die when there's a race condition causing a naming conflict |
| 62 | | - $dbw->insert( 'gadgets', array( array( |
| 63 | | - 'gd_name' => $name, |
| 64 | | - 'gd_blob' => $json, |
| 65 | | - 'gd_shared' => $gadget->isShared(), |
| 66 | | - 'gd_timestamp' => $ts |
| 67 | | - ) ), __METHOD__, array( 'IGNORE' ) |
| | 55 | + $ts = $dbw->timestamp( $gadget->getTimestamp() ); |
| | 56 | + $newTs = $dbw->timestamp( $timestamp ); |
| | 57 | + $row = array( |
| | 58 | + 'gd_name' => $name, |
| | 59 | + 'gd_blob' => $json, |
| | 60 | + 'gd_shared' => $gadget->isShared(), |
| | 61 | + 'gd_timestamp' => $newTs |
| 68 | 62 | ); |
| 69 | 63 | |
| 70 | | - // Detect naming conflict after the fact |
| 71 | | - if ( $dbw->affectedRows() === 0 ) { |
| 72 | | - return Status::newFatal( 'gadget-manager-create-exists', $name ); |
| | 64 | + // First INSERT IGNORE the row, in case the gadget doesn't already exist |
| | 65 | + $dbw->begin(); |
| | 66 | + $created = false; |
| | 67 | + $dbw->insert( 'gadgets', $row, __METHOD__, array( 'IGNORE' ) ); |
| | 68 | + $created = $dbw->affectedRows() > 0; |
| | 69 | + // Then UPDATE it if it did already exist |
| | 70 | + if ( !$created ) { |
| | 71 | + $dbw->update( 'gadgets', $row, array( |
| | 72 | + 'gd_name' => $name, |
| | 73 | + 'gd_timestamp <= ' . $dbw->addQuotes( $ts ) // for conflict detection |
| | 74 | + ), __METHOD__ |
| | 75 | + ); |
| 73 | 76 | } |
| | 77 | + $dbw->commit(); |
| 74 | 78 | |
| 75 | | - // Update our in-object cache |
| 76 | | - // This looks stupid: we have an object that we could be caching. But I prefer |
| 77 | | - // to keep $this->data in a consistent format and have getGadget() always return |
| 78 | | - // a clone. If it returned a reference to a cached object, the caller could change |
| 79 | | - // that object and cause weird things to happen. |
| 80 | | - $this->data[$name] = array( 'json' => $json, 'timestamp' => $ts ); |
| 81 | | - |
| 82 | | - return Status::newGood(); |
| 83 | | - } |
| 84 | | - |
| 85 | | - public function modifyGadget( Gadget $gadget ) { |
| 86 | | - if ( !$this->isWriteable() ) { |
| 87 | | - return Status::newFatal( 'gadget-manager-readonly-repository' ); |
| 88 | | - } |
| 89 | | - |
| 90 | | - $this->loadData(); |
| 91 | | - $name = $gadget->getName(); |
| 92 | | - if ( !isset( $this->data[$name] ) ) { |
| 93 | | - return Status::newFatal( 'gadget-manager-nosuchgadget', $name ); |
| 94 | | - } |
| 95 | | - |
| 96 | | - $json = $gadget->getJSON(); |
| 97 | | - $ts = $gadget->getTimestamp(); |
| 98 | | - $dbw = $this->getMasterDB(); |
| 99 | | - $newTs = $dbw->timestamp(); |
| 100 | | - $dbw->update( 'gadgets', array( |
| 101 | | - 'gd_blob' => $json, |
| 102 | | - 'gd_shared' => $gadget->isShared(), |
| 103 | | - 'gd_timestamp' => $dbw->timestamp() |
| 104 | | - ), array( |
| 105 | | - 'gd_name' => $name, |
| 106 | | - 'gd_timestamp' => $ts // for conflict detection |
| 107 | | - ), __METHOD__, array( 'IGNORE' ) |
| 108 | | - ); |
| 109 | | - |
| 110 | 79 | // Detect conflicts |
| 111 | 80 | if ( $dbw->affectedRows() === 0 ) { |
| 112 | | - // Some conflict occurred. Either the UPDATE failed because the |
| 113 | | - // timestamp condition didn't match, in which case someone else |
| 114 | | - // modified the gadget concurrently, or it failed to find a row |
| 115 | | - // for this gadget name at all, in which case someone else deleted |
| 116 | | - // the gadget entirely. We don't really care what happened, we'll |
| 117 | | - // just return an error and let the caller figure it out. |
| | 81 | + // Some conflict occurred |
| 118 | 82 | return Status::newFatal( 'gadgets-manager-modify-conflict', $name, $ts ); |
| 119 | 83 | } |
| 120 | 84 | |
| 121 | 85 | // Update our in-object cache |
| 122 | | - // See comment in addGadget() for an explanation of why it's done this way |
| | 86 | + // This looks stupid: we have an object that we could be caching. But I prefer |
| | 87 | + // to keep $this->data in a consistent format and have getGadget() always return |
| | 88 | + // a clone. If it returned a reference to a cached object, the caller could change |
| | 89 | + // that object and cause weird things to happen. |
| 123 | 90 | $this->data[$name] = array( 'json' => $json, 'timestamp' => $newTs ); |
| 124 | 91 | |
| 125 | 92 | return Status::newGood(); |
| — | — | @@ -168,6 +135,8 @@ |
| 169 | 136 | * $this->data must call this before accessing $this->data . |
| 170 | 137 | */ |
| 171 | 138 | protected function loadData() { |
| | 139 | + // FIXME: Make the cache shared somehow, it's getting repopulated for every instance now |
| | 140 | + // FIXME: Reconsider the query-everything behavior; maybe use memc? |
| 172 | 141 | if ( is_array( $this->data ) ) { |
| 173 | 142 | // Already loaded |
| 174 | 143 | return; |
| Index: branches/RL2/extensions/Gadgets/backend/GadgetRepo.php |
| — | — | @@ -56,22 +56,15 @@ |
| 57 | 57 | abstract public function getDB(); |
| 58 | 58 | |
| 59 | 59 | /** |
| 60 | | - * Add a new gadget to the repository. Will fail if there is already |
| 61 | | - * a gadget with the same name. |
| 62 | | - * @param $gadget Gadget object |
| 63 | | - * @return Status |
| 64 | | - */ |
| 65 | | - abstract public function addGadget( Gadget $gadget ); |
| 66 | | - |
| 67 | | - /** |
| 68 | | - * Modify an existing gadget, replacing its metadata with the |
| | 60 | + * Modify a gadget, replacing its metadata with the |
| 69 | 61 | * metadata in the provided Gadget object. The name is taken |
| 70 | | - * from the Gadget object as well. Will fail if there is no |
| 71 | | - * gadget by the same name. |
| | 62 | + * from the Gadget object as well. If no Gadget exists by that name, |
| | 63 | + * it will be created. |
| 72 | 64 | * @param $gadget Gadget object |
| | 65 | + * @param $timestamp Timestamp to record for this action, or current timestamp if null |
| 73 | 66 | * @return Status |
| 74 | 67 | */ |
| 75 | | - abstract public function modifyGadget( Gadget $gadget ); |
| | 68 | + abstract public function modifyGadget( Gadget $gadget, $timestamp = null ); |
| 76 | 69 | |
| 77 | 70 | /** |
| 78 | 71 | * Irrevocably delete a gadget from the repository. Will fail |