From 5693b8c657b58b438e9c93a2d41ca57ac4d002c8 Mon Sep 17 00:00:00 2001 From: Benn Oshrin Date: Fri, 21 Dec 2018 21:35:59 -0500 Subject: [PATCH] Code cleanup --- app/config/routes.php | 5 +- app/config/schema/schema.xml | 9 - app/src/Controller/ApiUsersController.php | 20 ++ app/src/Controller/TierApiController.php | 355 ++++++++-------------- app/src/Lib/Match/MatchService.php | 183 ++++++----- app/src/Lib/Match/ResultManager.php | 4 + app/src/Locale/en_US/default.po | 11 +- app/src/Template/ApiUsers/columns.inc | 4 +- 8 files changed, 268 insertions(+), 323 deletions(-) diff --git a/app/config/routes.php b/app/config/routes.php index 2d56b3ec5..bf1402667 100644 --- a/app/config/routes.php +++ b/app/config/routes.php @@ -61,13 +61,12 @@ */ // Match Request -// XXX should we use TierApiV1Controller? or v1match? $routes->get('/api/:matchgrid_id/v1/matchRequests/:id', ['controller' => 'TierApi', 'action' => 'viewMatchRequest']); $routes->get('/api/:matchgrid_id/v1/matchRequests', ['controller' => 'TierApi', 'action' => 'viewMatchRequests']); $routes->put('/api/:matchgrid_id/v1/people/:sor/:sorid', ['controller' => 'TierApi', 'action' => 'match']); $routes->post('/api/:matchgrid_id/v1/people/:sor/:sorid', ['controller' => 'TierApi', 'action' => 'search']); -// XXX do we need the ?* here? (it breaks matchRequests -> viewMatchRequests) - $routes->get('/api/:matchgrid_id/v1/people/:sor/:sorid?*', ['controller' => 'TierApi', 'action' => 'search']); +// This doesn't match and we end up in current(), so we just check there +// $routes->get('/api/:matchgrid_id/v1/people/:sor/:sorid?*', ['controller' => 'TierApi', 'action' => 'search']); $routes->delete('/api/:matchgrid_id/v1/people/:sor/:sorid', ['controller' => 'TierApi', 'action' => 'remove']); $routes->get('/api/:matchgrid_id/v1/people/:sor/:sorid', ['controller' => 'TierApi', 'action' => 'current']); $routes->get('/api/:matchgrid_id/v1/people/:sor', ['controller' => 'TierApi', 'action' => 'inventory']); diff --git a/app/config/schema/schema.xml b/app/config/schema/schema.xml index 6d12b2485..4e083a590 100644 --- a/app/config/schema/schema.xml +++ b/app/config/schema/schema.xml @@ -24,10 +24,6 @@ must be specified in raw SQL, which needs the prefixed table name. --> - - @@ -171,11 +167,6 @@ rule_id - - - - attribute_id -
diff --git a/app/src/Controller/ApiUsersController.php b/app/src/Controller/ApiUsersController.php index 9579cc250..91ab144fa 100644 --- a/app/src/Controller/ApiUsersController.php +++ b/app/src/Controller/ApiUsersController.php @@ -29,7 +29,27 @@ namespace App\Controller; +use Cake\Routing\Router; + class ApiUsersController extends StandardController { + /** + * Callback run prior to the view rendering. + * + * @since COmanage Match v1.0.0 + * @param Event $event Cake Event + */ + + public function beforeRender(\Cake\Event\Event $event) { + parent::beforeRender($event); + + // Populate a pointer to the REST API for configuration purposes + $mgid = isset($this->cur_mg->id) ? $this->cur_mg->id : null; + + $fragment = "/api/" . ($mgid ? $mgid : "{matchgrid_id}") . "/v1"; + + $this->set('vv_api_uri', Router::url($fragment, true)); + } + /** * Authorization for this Controller, called by Auth component * - postcondition: $vv_permissions set with calculated permissions for this Controller diff --git a/app/src/Controller/TierApiController.php b/app/src/Controller/TierApiController.php index eda938e60..68d0bb585 100644 --- a/app/src/Controller/TierApiController.php +++ b/app/src/Controller/TierApiController.php @@ -48,76 +48,24 @@ class TierApiController extends AppController { */ public function initialize() { - // We should merge back in so we don't have a fully custom initialize -// parent::initialize(); + // We have substantial differences from AppController::initialize(), so we + // completely override here. $this->loadComponent('RequestHandler'); -// $this->loadComponent('Flash'); -/* - $this->loadComponent('Auth', [ - 'authorize' => 'Controller', - 'authenticate' => [ - 'Env' - ], - 'unauthorizedRedirect' => '/match/webroot/auth/login' -/ - 'loginAction' => [ - 'controller' => 'Users', - 'action' => 'login' - ], - // If unauthorized, return them to page they were just on - 'unauthorizedRedirect' => $this->referer() - / - ]);*/ - - // Allow the display action so our PagesController - // continues to work. Also enable the read only actions. -// $this->Auth->allow(['display', 'view', 'index']); - -// XXX do we want to reenable either of these in some limited mode? - /* - * Enable the following components for recommended CakePHP security settings. - * see https://book.cakephp.org/3.0/en/controllers/components/security.html - */ -// $this->loadComponent('Security'); -// XXX this is deprecated, look at https://book.cakephp.org/3.0/en/controllers/middleware.html#csrf-middleware -// $this->loadComponent('Csrf'); - } - - /** - * Callback run prior to the request action. - * - * @since COmanage Match v1.0.0 - * @param Event $event Cake Event - */ - - public function beforeFilter(\Cake\Event\Event $event) { - // We need the current matchgrid (if set) before we configuration authentication - - $mgid = $this->request->getParam('matchgrid_id'); - $this->loadComponent('Auth', [ 'authorize' => 'Controller', 'authenticate' => [ 'Basic' => [ 'fields' => ['username' => 'username', 'password' => 'password'], 'userModel' => 'ApiUsers', - // Custom finder to constrain users to the request matchgrid - // We don't currently use this since Platform API users can access - // any matchgrid, but won't have the matchgrid ID set. (We could - // retrieve where $mgid||null, but we still have to filter in - // isAuthorized anyway.) -// 'finder' => ['withinMatchgrid' => ['matchgrid' => $mgid]] - // But we do want to pull SoR information for authz purposes + // We pull SoR information for authz purposes (handled in isAuthorized) 'finder' => 'authorization' ] ], 'storage' => 'Memory', 'unauthorizedRedirect' => false ]); - - parent::beforeFilter($event); } /** @@ -127,45 +75,15 @@ public function beforeFilter(\Cake\Event\Event $event) { */ public function current() { -// XXX some overlap with inventory() - $result = null; - $statusCode = 200; - - $matchgridId = (int)$this->request->getParam('matchgrid_id'); - $sor = $this->request->getParam('sor'); - $sorid = $this->request->getParam('sorid'); - - try { - $MatchService = new \App\Lib\Match\MatchService(); - - $MatchService->connect(); - $MatchService->setConfig($matchgridId); + if(!empty($this->request->getQueryParams())) { + // This is actually a Search Only request via GET, but routes.php doesn't + // quite want to send the request to search(). Regardless, we don't + // (currently) support search over GET, so throw an error. - $results = $MatchService->getSorAttributes($sor, $sorid); -// XXX $results->getResultsForJson(); but $result is an array, not a Result Manager - - if($results->count()==0) { - $statusCode = 404; - } else { - $result = $results->getResultsForJson('current'); - } - // We shouldn't get more than one row - - $MatchService->disconnect(); - } - catch(\Exception $e) { - $statusCode = 500; - $result['error'] = $e->getMessage(); - Log::write('error', $e->getMessage()); + throw new \Cake\Http\Exception\MethodNotAllowedException("GET not supported for Search Only, use POST instead"); } - $this->viewBuilder()->setLayout('rest'); - - $this->response = $this->response->withStatus($statusCode); - - // Set the result data and render the default API response view - $this->set('vv_result', $result); - $this->render('response'); + $this->dispatch('doCurrent'); } /** @@ -175,11 +93,11 @@ public function current() { * @param string $func Function to call */ -// XXX merge all calls to use this protected function dispatch(string $func) { -/* XXX check to see if requested vars exist, throw error otherwise - $sor = $this->request->getParam('sor'); - $sorId = $this->request->getParam('sorid');*/ + // Note that in general routes.php will prevent us from being called without + // our core parameters (typically 'sor' or 'sorid', according to the + // requested operation). + $matchgridId = (int)$this->request->getParam('matchgrid_id'); try { @@ -207,32 +125,62 @@ protected function dispatch(string $func) { $this->render('response'); } -// XXX docblock + /** + * Handle an API Request Current Values request, ie: GET /v1/people/sor/sorid + * + * @since COmanage Match v1.0.0 + * @param MatchService $MatchService Match Service + */ + + protected function doCurrent(\App\Lib\Match\MatchService $MatchService) { + $sor = $this->request->getParam('sor'); + $sorid = $this->request->getParam('sorid'); + + $resultManager = $MatchService->getSorAttributes($sor, $sorid); + + if($resultManager->count()==0) { + $this->statusCode = 404; + } else { + $this->statusCode = 200; + $this->result = $resultManager->getResultsForJson('current'); + } + // We shouldn't get more than one row + } + + /** + * Handle an API Inventory of Requests request, , ie: GET /v1/people/sor + * + * @since COmanage Match v1.0.0 + * @param MatchService $MatchService Match Service + */ + + protected function doInventory(\App\Lib\Match\MatchService $MatchService) { + $sor = $this->request->getParam('sor'); + + $this->result['sorids'] = $MatchService->getSorIds($sor); + $this->statusCode = 200; + } + + /** + * Perform a match operation. + * + * @since COmanage Match v1.0.0 + * @param boolean $searchOnly If true, do not cause any state changes + */ protected function doMatchRequest(bool $searchOnly=false) { $statusCode = 500; $result = []; -// debug('people controller match'); -// XXX need to expose api URL somewhere (https://server/match/api/matchgrid_id/) so -// admins can configure other end $matchgridId = (int)$this->request->getParam('matchgrid_id'); $sor = $this->request->getParam('sor'); -// XXX We should do an authz check on $sor/$matchgridId, based on requester -// and also verify they were provided -// and also for all the other calls (current, remove, etc) $sorid = $this->request->getParam('sorid'); // Request attributes are here (json body) $json = $this->request->input('json_decode'); -// XXX for $searchOnly, attributes can come from GET, see $this->request->getQueryParams() -// if we don't implement this at first commit throw NOT IMPLEMENTED 500 error if GET -// XXX walk through and add appropriate logging -// should debugging be localized? probably not Log::write('debug', $sor . "/" . $sorid . ($searchOnly ? " Search" : " Match") . " request received for matchgrid " . $matchgridId); try { -// XXX Maybe move this test higher? if(empty($json)) { throw new \InvalidArgumentException('No JSON record found or body not successfully parsed'); } @@ -247,29 +195,49 @@ protected function doMatchRequest(bool $searchOnly=false) { Log::write('debug', $sor . "/" . $sorid . " Obtaining configuration for matchgrid " . $matchgridId); $MatchService->setConfig($matchgridId); -// XXX should we start a transaction? and maybe call connect/disconnect from here -// (poc did not set up transactions) + // We don't use transactions here because for the most part they wouldn't + // accomplish anything. ie: + // (1) For Update Match Attributes, getRequestIdForSorId() results will be static + // (unless two admin operations happen simultaneously, which shouldn't + // ordinarily happen). + // (2) Forced Reconciliation only performs one write operation. + // (3) Search/Update could use transactions in theory, but we'd need to use + // a read lock ("FOR UPDATE", or $dbc->RowLock()), and if there are no + // existing records, the read lock won't lock anything. Even if there + // is a match, we're not updating the prior record, we're updating the + // new record (which presumably no other actor is modifying at the same + // time). + + // The Reference ID included in the request, if any + $requestedReferenceId = $AttributeManager->getRequestedReferenceId(); + + // The current row ID for this $sor + $sorid, if any + $curid = $MatchService->getRequestIdForSorId($sor, $sorid); + + // The current Reference ID for the current row ID, if any + $currentReferenceId = ($curid ? $MatchService->getReferenceIdForRequest($curid) : null); if(!$searchOnly - && !$AttributeManager->getRequestedReferenceId() - && ($curid = $MatchService->searchSorId($sor, $sorid))) { + && !$requestedReferenceId + && $curid + && $currentReferenceId) { // If we already have a record for sor+sorid and no referenceId was - // provided, this is an Update Match Attributes request. + // provided, this is an Update Match Attributes request. However, if + // there is no referenceId in the matchgrid entry, then we instead want + // to reprocess the record as a standard search request Log::write('debug', $sor . "/". $sorid . " Updating existing SOR attributes for Row ID " . $curid); $MatchService->updateSorAttributes($curid, $AttributeManager); $statusCode = 200; - } elseif(!$searchOnly && $AttributeManager->getRequestedReferenceId()) { + } elseif(!$searchOnly && $requestedReferenceId) { // Forced Reconciliation request. Skip the search and jump to the insert. // (attachReferenceId will insert or update as appropriate.) - // If no attributes were provided in the JSON, then this is a Reassign - // Reference Identifier request, which we handle basically the same way. - // MatchService::update will figure out what to do. + Log::write('debug', $sor . "/". $sorid . " Processing forced reconciliation request for Reference ID " . $requestedReferenceId); - $referenceId = $MatchService->attachReferenceId($sor, $sorid, $AttributeManager, $AttributeManager->getRequestedReferenceId()); + $referenceId = $MatchService->attachReferenceId($sor, $sorid, $AttributeManager, $requestedReferenceId); $result = ['referenceId' => $referenceId]; @@ -284,13 +252,19 @@ protected function doMatchRequest(bool $searchOnly=false) { if($searchOnly) { $statusCode = 404; } else { - $referenceId = $MatchService->assignReferenceId($sor, $sorid, $AttributeManager); + if($curid) { + // We need to treat this as a forced reconciliation request, but it's really + // an SOR performing an update match attributes request on a record without + // an existing referenceid + $referenceId = $MatchService->attachReferenceId($sor, $sorid, $AttributeManager, 'new'); + } else { + $referenceId = $MatchService->assignReferenceId($sor, $sorid, $AttributeManager); + } $result = ['referenceId' => $referenceId]; $statusCode = 201; } - // XXX or return 404 depending on read-only/read-write } elseif($results->getConfidenceMode() == ConfidenceModeEnum::Canonical) { // Exact match @@ -298,6 +272,8 @@ protected function doMatchRequest(bool $searchOnly=false) { if(!empty($refIds[0])) { if(!$searchOnly) { + // This should also correctly handle an update match attribute request + // that did not originally have a referenceid $MatchService->attachReferenceId($sor, $sorid, $AttributeManager, $refIds[0]); } @@ -312,9 +288,6 @@ protected function doMatchRequest(bool $searchOnly=false) { $SoR = TableRegistry::get('SystemsOfRecord'); -// XXX custom finder? -// XXX this will throw 500 if $sor is not defined -// XXX maybe merge this into the authz check? $sorobj = $SoR->find('all')->where(['matchgrid_id' => $matchgridId, 'label' => $sor])->firstOrFail(); if(!$searchOnly && $sorobj->resolution_mode == ResolutionModeEnum::External) { @@ -323,23 +296,22 @@ protected function doMatchRequest(bool $searchOnly=false) { $matchRequest = $MatchService->insertPending($sor, $sorid, $AttributeManager); $result['matchRequest'] = $matchRequest; - } elseif($sorobj->resolution_mode == ResolutionModeEnum::Interactive) { + } else { + // Interactive SOR, or searchOnly (which can't return a 202) $statusCode = 300; $result['candidates'] = $results->getResultsForJson(); } - // XXX else throw an error? } } $MatchService->disconnect(); } -// try/catch and coerce errors into a 400 or 500 response + // Coerce errors into a 400 or 500 response catch(\InvalidArgumentException $e) { $statusCode = 400; $result['error'] = $e->getMessage(); Log::write('error', $e->getMessage()); -// XXX put the error in the json body somewhere? } catch(\RuntimeException $e) { $statusCode = 500; @@ -362,16 +334,6 @@ protected function doMatchRequest(bool $searchOnly=false) { $this->set('vv_result', $result); $this->render('response'); } - - // empty -// debug($this->data); - // this has server vars -// debug($this->request->getServerParams()); - // empty -// debug($this->request->getQuery()); -// debug($this->request->getQueryParams()); - // empty -// debug($this->request->getData()); /** * Handle an API Join Reference Identifiers request, ie: PUT /v1/referenceIds/id @@ -381,7 +343,6 @@ protected function doMatchRequest(bool $searchOnly=false) { */ protected function doMerge(\App\Lib\Match\MatchService $MatchService) { -// XXX check that value is provided // Reference ID we want to keep / merge to $targetId = $this->request->getParam('id'); @@ -391,22 +352,27 @@ protected function doMerge(\App\Lib\Match\MatchService $MatchService) { $MatchService->merge($targetId, $json->referenceIds); $this->statusCode = 200; + } else { + $this->statusCode = 400; } - /* - if(!empty($this->request->getQuery('status'))) { - // Obtain pending/resolved requests - -// XXX should we filter_var getQuery? (for now it's not needed, but maybe if getRequests changed?) -// XXX should check that 'status' is set? - $r = $MatchService->getRequests($this->request->getQuery('status')); - } elseif(!empty($this->request->getQuery('referenceId'))) { - // Obtain SOR Records request - - $r = $MatchService->getRequestsForReferenceId($this->request->getQuery('referenceId')); - } + } + + /** + * Handle an API Delete Current Values request + * + * @since COmanage Match v1.0.0 + * @param MatchService $MatchService Match Service + */ + + protected function doRemove(\App\Lib\Match\MatchService $MatchService) { + $sor = $this->request->getParam('sor'); + $sorId = $this->request->getParam('sorid'); - $this->result['matchRequests'] = $r->getResultsForJson('pending'); - $this->statusCode = 200;*/ + if($MatchService->remove($sor, $sorId)) { + $this->statusCode = 200; + } else { + $this->statusCode = 404; + } } /** @@ -417,7 +383,6 @@ protected function doMerge(\App\Lib\Match\MatchService $MatchService) { */ protected function doViewMatchRequest(\App\Lib\Match\MatchService $MatchService) { -// XXX should check that 'id' is set? $results = $MatchService->getRequest((int)$this->request->getParam('id')); if($results->count() == 0) { @@ -438,8 +403,9 @@ protected function doViewMatchRequest(\App\Lib\Match\MatchService $MatchService) return; } -// XXX it's plausible (but unlikely) that a pending match could becoume canonically resolvable -// after it has been submitted (eg: if some bad conflicting data was cleaned up) + // It's plausible (but unlikely) that a pending match could becoume canonically resolvable + // after it has been submitted (eg: if some bad conflicting data was cleaned up), but for + // the moment we don't try to catch that. $this->statusCode = 300; // Extract the SOR and SORID @@ -481,8 +447,6 @@ protected function doViewMatchRequests(\App\Lib\Match\MatchService $MatchService if(!empty($this->request->getQuery('status'))) { // Obtain pending/resolved requests -// XXX should we filter_var getQuery? (for now it's not needed, but maybe if getRequests changed?) -// XXX should check that 'status' is set? $r = $MatchService->getRequests($this->request->getQuery('status')); } elseif(!empty($this->request->getQuery('referenceId'))) { // Obtain SOR Records request @@ -494,37 +458,14 @@ protected function doViewMatchRequests(\App\Lib\Match\MatchService $MatchService $this->statusCode = 200; } -// XXX docbock + /** + * Handle an API Inventory of Requests request, , ie: GET /v1/people/sor + * + * @since COmanage Match v1.0.0 + */ + public function inventory() { - $result = []; - $statusCode = 200; - - $matchgridId = (int)$this->request->getParam('matchgrid_id'); - $sor = $this->request->getParam('sor'); - - try { - $MatchService = new \App\Lib\Match\MatchService(); - - $MatchService->connect(); - $MatchService->setConfig($matchgridId); - - $result['sorids'] = $MatchService->getSorIds($sor); - - $MatchService->disconnect(); - } - catch(\Exception $e) { - $statusCode = 500; - $result['error'] = $e->getMessage(); - Log::write('error', $e->getMessage()); - } - - $this->viewBuilder()->setLayout('rest'); - - $this->response = $this->response->withStatus($statusCode); - - // Set the result data and render the default API response view - $this->set('vv_result', $result); - $this->render('response'); + $this->dispatch('doInventory'); } /** @@ -537,6 +478,9 @@ public function inventory() { */ public function isAuthorized(Array $user) { + // Unlike most other controllers, authorization for the API is determined by + // the data available in the request. + Log::write('debug', 'TierApiController::isAuthorized() request for ' . $user['username']); // Because we're using BasicAuthenticate, $user will have the record from api_users. @@ -626,7 +570,6 @@ public function match() { * @since COmanage Match v1.0.0 */ -// XXX what should the authz be on this? public function merge() { $this->dispatch('doMerge'); } @@ -638,40 +581,7 @@ public function merge() { */ public function remove() { -// XXX should authz for this be configurable? ie: perhaps only admins can do this, and not SORs - $result = []; - $statusCode = 200; - - $matchgridId = (int)$this->request->getParam('matchgrid_id'); - $sor = $this->request->getParam('sor'); - $sorId = $this->request->getParam('sorid'); - - try { - $MatchService = new \App\Lib\Match\MatchService(); - - $MatchService->connect(); - $MatchService->setConfig($matchgridId); - - if(!$MatchService->remove($sor, $sorId)) { - $statusCode = 404; - } - - $MatchService->disconnect(); - } -// XXX Should we return 404 on NOT FOUND? - catch(\Exception $e) { - $statusCode = 500; - $result['error'] = $e->getMessage(); - Log::write('error', $e->getMessage()); - } - - $this->viewBuilder()->setLayout('rest'); - - $this->response = $this->response->withStatus($statusCode); - - // Set the result data and render the default API response view - $this->set('vv_result', $result); - $this->render('response'); + $this->dispatch('doRemove'); } /** @@ -681,7 +591,8 @@ public function remove() { */ public function search() { -// XXX make sure not to return pending records + // Note a GET Search Only request will end up at current() due to routes.php. + $this->doMatchRequest(true); } diff --git a/app/src/Lib/Match/MatchService.php b/app/src/Lib/Match/MatchService.php index e3c9aaf09..b3c53ff5d 100644 --- a/app/src/Lib/Match/MatchService.php +++ b/app/src/Lib/Match/MatchService.php @@ -38,6 +38,7 @@ use \App\Lib\Enum\ConfidenceModeEnum; use \App\Lib\Enum\ReferenceIdEnum; use \App\Lib\Enum\SearchTypeEnum; +use \App\Lib\Enum\StatusEnum; require(ROOT . DS . "vendor" . DS . "adodb" . DS . "adodb-php" . DS . "adodb-xmlschema03.inc.php"); @@ -142,6 +143,50 @@ public function getRequest(int $id) { return $results; } + /** + * Obtain the given Reference ID for the specified match request. + * + * @since COmanage Match v1.0.0 + * @param int $id Match Request ID + * @return string Reference ID, or null if no record found or Reference ID not set + */ + + public function getReferenceIdForRequest(int $id) { + $sql = "SELECT referenceid + FROM " . $this->mgTable . " + WHERE id=?"; + + $vals = [$id]; + + $stmt = $this->dbc->Prepare($sql); + + return $this->dbc->GetOne($stmt, $vals); + } + + /** + * Obtain the request ID for an existing record for $sor + $sorid. + * + * @since COmanage Match v1.0.0 + * @param string $sor Requesting System of Record + * @param string $sorid Requesting SOR's Identifier + * @return int Row ID or NULL + */ + + public function getRequestIdForSorId(string $sor, string $sorid) { + $sql = "SELECT id + FROM " . $this->mgTable . " + WHERE sor=? + AND sorid=?"; + + $vals = [$sor, $sorid]; + + $stmt = $this->dbc->Prepare($sql); + + $r = $this->dbc->GetOne($stmt, $vals); + + return ($r ? (int)$r : null); + } + /** * Obtain match requests of a given status. * @@ -271,17 +316,13 @@ public function getSorIds(string $sor) { * @param AttributeManager $attributes Request Attributes * @param string $referenceId Requested Reference ID, or "new" to assign a new Reference ID * @return string Row ID + * @throws RuntimeException */ protected function insert(string $sor, string $sorid, AttributeManager $attributes, string $referenceId=null) { -// XXX test for error on already existing SOR+SORID and handle "gracefully" -// note attachReferenceId handles upsert for forced reconciliation request -// what about attribute update request? -// XXX should we check that sor + sorid doesn't exist already (maybe in search())? The UNIQUE constraint should -// throw an error if it does, can we catch that and coerce it into a more informative error? -// Though if it's unreconciled we could replace it (PoC did that) - -// XXX the poc did a delete/insert but that loses the original request time + // For most cases, code will either call upsert() or do other sanity checking + // such that insert shouldn't typically fail if there is a row for $sor+$sorid + // already in the matchgrid. // Request time is now, resolution time is also now if a referenceId was specified. $requestTime = gmdate('Y-m-d H:i:s'); @@ -302,7 +343,6 @@ protected function insert(string $sor, string $sorid, AttributeManager $attribut } // "RETURNING id" is not portable SQL -// XXX should we be using adodb's parameter generator? $sql = "INSERT INTO " . $this->mgTable . " (" . implode(",", $attrs) . ") VALUES (" . str_repeat("?,", count($vals)-1) . "?) RETURNING id"; @@ -313,8 +353,7 @@ protected function insert(string $sor, string $sorid, AttributeManager $attribut if(!$rowid) { Log::write('error', $this->dbc->errorMsg()); -// XXX throw an exception - print $this->dbc->errorMsg(); + throw new \RuntimeException($this->dbc->errorMsg()); } Log::write('debug', "Inserted new matchgrid entry at row ID " . $rowid); @@ -333,7 +372,9 @@ protected function insert(string $sor, string $sorid, AttributeManager $attribut */ public function insertPending(string $sor, string $sorid, AttributeManager $attributes) { - return $this->insert($sor, $sorid, $attributes); + // We call upsert in case an SOR issues an update match attributes request on + // a record that doesn't yet have a match reference ID. + return $this->upsert($sor, $sorid, $attributes); } /** @@ -395,7 +436,6 @@ public function remove(string $sor, string $sorid) { * @param string $sor SOR Label * @param string $sorid SOR ID * @param AttributeManager $attributes Search attibutes - * @param string $referenceId Reference ID, if known * @return ResultManager Result Manager * @throws LogicException * @throws RuntimeException @@ -404,13 +444,11 @@ public function remove(string $sor, string $sorid) { protected function search(string $mode, string $sor, string $sorid, - AttributeManager $attributes, - string $referenceId=null) { // XXX do we need this? + AttributeManager $attributes) { $results = new ResultManager; $results->setConfidenceMode($mode); $results->setConfig($this->mgConfig->attributes); - // XXX if $referenceId is provided, should we only look for sor+sorid+referenceid (ignoring $attributes?) $ruleObjs = ($mode == ConfidenceModeEnum::Canonical ? "canonical" : "potential") . "_rules"; @@ -459,11 +497,9 @@ protected function search(string $mode, . $maxdistance; break; case SearchTypeEnum::Exact: -// XXX should we be using adodb's parameter generator? $andclause = $colclause . "=?"; break; case SearchTypeEnum::Substring: -// XXX document that initial position is 1, not 0 (ie: from 1 for 3 = SMI for SMITH) $andclause = "SUBSTRING(" . $colclause . " FROM " @@ -477,7 +513,7 @@ protected function search(string $mode, . ")"; break; default: - throw new LogicException(__('matchgrid.er.search_type', [$ruleattr->search_type])); + throw new LogicException(__('match.er.search_type', [$ruleattr->search_type])); break; } @@ -485,8 +521,6 @@ protected function search(string $mode, $vals[] = $val; } -// XXX Maybe add a special setting to enable logging of SQL? -// XXX add timing on SQL queries? or overall transaction? LOG::write('debug', $sor . "/" . $sorid . " SQL: " . $sql); $stmt = $this->dbc->Prepare($sql); @@ -512,13 +546,10 @@ protected function search(string $mode, } return $results; - - // XXX review POC for additional logic implemented in searchDatabase() and buildAttributeSql() } /** * Attempt to obtain a reference ID based on search attributes. -// XXX should this be renamed? @return is ResultManager, not ReferenceId(string) * * @since COmanage Match v1.0.0 * @param string $sor Requesting System of Record @@ -527,14 +558,9 @@ protected function search(string $mode, * @return ResultManager Result Manager */ -// XXX maybe the public function should be "search" (and "insert"), and the private -// functions should be doSearch/doInsert, or similar public function searchReferenceId(string $sor, string $sorid, AttributeManager $attributes) { -// Log::write('debug', "Match request received for matchgrid " . $matchgridId . ": " . $sor . "/" . $sorid); - -// throw error if not obtained // First try canonical matches - $canonicalMatches = $this->search(ConfidenceModeEnum::Canonical, $sor, $sorid, $attributes);//, $referenceId); + $canonicalMatches = $this->search(ConfidenceModeEnum::Canonical, $sor, $sorid, $attributes); // XXX add some logging on match candidates? or maybe in search() switch($canonicalMatches->count()) { @@ -559,48 +585,23 @@ public function searchReferenceId(string $sor, string $sorid, AttributeManager $ return $potentialMatches; } - /** - * Search for an existing record for $sor + $sorid. - * - * @since COmanage Match v1.0.0 - * @param string $sor Requesting System of Record - * @param string $sorid Requesting SOR's Identifier - * @return int Row ID - */ - - public function searchSorId(string $sor, string $sorid) { -// XXX need to implement strtolower, preg_replace, etc (see PoC buildAttributeSql) - $sql = "SELECT id - FROM " . $this->mgTable . " - WHERE sor=? - AND sorid=?"; - - $vals = [$sor, $sorid]; - - $stmt = $this->dbc->Prepare($sql); - - $r = $this->dbc->GetOne($stmt, $vals); - - return $r; - -// Log::write('debug', $sor . "/" . $sorid . " Matched " . $count . " candidate(s) using rule " . $rule->name); - } - /** * Obtain and store the configuration for the specified matchgrid. * * @since COmanage Match v1.0.0 - * @param int $matchgridId Matchgrid ID + * @param int $matchgridId Matchgrid ID + * @throws Cake\Datasource\Exception\RecordNotFoundException */ public function setConfig(int $matchgridId) { $Matchgrids = TableRegistry::get('Matchgrids'); -// XXX what error does this throw if not found? $this->mgConfig = $Matchgrids->findById($matchgridId) + ->where(['status' => StatusEnum::Active]) ->contain(['Attributes' => 'AttributeGroups', 'CanonicalRules' => [ -// XXX we already pull attributes above, but this makes it easier to access them in search() + // We already pull attributes above, but this makes it + // easier to access them in search() 'RuleAttributes' => ['Attributes' => 'AttributeGroups'], 'sort' => ['CanonicalRules.ordr' => 'ASC'] ], @@ -610,8 +611,6 @@ public function setConfig(int $matchgridId) { ]]) ->firstOrFail(); -// XXX prefix should be inserted in parent call? or maybe as virtual field? -// XXX throw an error if matchgrid is not active $this->mgTable = "mg_" . $this->mgConfig->table_name; } @@ -619,15 +618,14 @@ public function setConfig(int $matchgridId) { * Update an existing Matchgrid record. * * @since COmanage Match v1.0.0 - * @param string $rowid Row ID + * @param int $rowid Row ID * @param AttributeManager $attributes Request Attributes * @param string $referenceId Requested Reference ID * @return string Row ID * @throws RuntimeException */ - protected function update(string $rowid, AttributeManager $attributes, string $referenceId=null) { -// XXX note update doesn't allow sor/sorid to be changed + protected function update(int $rowid, AttributeManager $attributes, string $referenceId=null) { // XXX create a history record // We don't update request time $resolutionTime = ($referenceId ? gmdate('Y-m-d H:i:s') : null); @@ -641,6 +639,13 @@ protected function update(string $rowid, AttributeManager $attributes, string $r if($attributes->attributesAvailable()) { foreach($this->mgConfig->attributes as $attr) { + // By default, this array shouldn't have sor or sorid. However, we'll double + // check in case an admin tries to explicitly list it... we don't want the + // ability to update sor or sorid via this interface. + + if(in_array(strtolower($attr->name), ['sor', 'sorid'])) + continue; + // Only add this attribute if there is a value specified $val = $attributes->getValueByAttribute($attr); @@ -680,13 +685,13 @@ protected function update(string $rowid, AttributeManager $attributes, string $r * Update the Attributes associated with an existing Matchgrid record. * * @since COmanage Match v1.0.0 - * @param string $rowid Row ID + * @param int $rowid Row ID * @param AttributeManager $attributes Request Attributes * @return string Row ID * @throws RuntimeException */ - public function updateSorAttributes(string $rowid, AttributeManager $attributes) { + public function updateSorAttributes(int $rowid, AttributeManager $attributes) { return $this->update($rowid, $attributes); } @@ -699,25 +704,45 @@ public function updateSorAttributes(string $rowid, AttributeManager $attributes) * @param AttributeManager $attributes Request Attributes * @param string $referenceId Requested Reference ID, or "new" to assign a new Reference ID * @return string Row ID + * @throws RuntimeException */ protected function upsert(string $sor, string $sorid, AttributeManager $attributes, string $referenceId=null) { // Dispatch to insert() or update() - -// XXX This should be a SELECT .. FOR UPDATE, but only if we're in a transaction - $sql = "SELECT id - FROM " . $this->mgTable . " - WHERE sor=" . $this->dbc->Param('a') . " - AND sorid=" . $this->dbc->Param('b'); - $stmt = $this->dbc->Prepare($sql); + $rowid = null; - $rowid = $this->dbc->GetOne($stmt, [$sor, $sorid]); + $dbh->startTrans(); - if($rowid) { - return $this->update($rowid, $attributes, $referenceId); - } else { - return $this->insert($sor, $sorid, $attributes, $referenceId); + try { + // SELECT FOR UPDATE only locks rows of matching records. ie: On an insert + // it won't lock anything. But the unique constraint on sor+sorid should + // prevent duplicate inserts. + $sql = "SELECT id + FROM " . $this->mgTable . " + WHERE sor=" . $this->dbc->Param('a') . " + AND sorid=" . $this->dbc->Param('b') . " + FOR UPDATE"; + + $stmt = $this->dbc->Prepare($sql); + + $rowid = (int)$this->dbc->GetOne($stmt, [$sor, $sorid]); + + if($rowid !== null) { + // $rowid should be the same before and after + $rowid = $this->update($rowid, $attributes, $referenceId); + } else { + $rowid = $this->insert($sor, $sorid, $attributes, $referenceId); + } + + $dbh->commit(); + } + catch(\Exception $e) { + $dbh->failTrans(); + Log::write('error', $sor . "/" . $sorid . " Upsert error: " . $e->getMessage()); + throw new \RuntimeException($e->getMessage()); } + + return $rowid; } } diff --git a/app/src/Lib/Match/ResultManager.php b/app/src/Lib/Match/ResultManager.php index 4d3272a15..66493d527 100644 --- a/app/src/Lib/Match/ResultManager.php +++ b/app/src/Lib/Match/ResultManager.php @@ -59,6 +59,10 @@ public function add(array $attributes) { // what if there are two SOR entries with different attributes but the same referenceId? // $this->results[$referenceId][$rowId] = $attributes; + if(empty($attributes)) { + return; + } + $referenceId = null; $rowId = null; $parsed = []; diff --git a/app/src/Locale/en_US/default.po b/app/src/Locale/en_US/default.po index 313c8ec48..b2d5a80d7 100644 --- a/app/src/Locale/en_US/default.po +++ b/app/src/Locale/en_US/default.po @@ -43,10 +43,10 @@ msgstr "Version {0}" ### Banners msgid "match.banner.api_users.matchgrid" -msgstr "This page is for configuring Matchgrid API Users. Platform API Users can only be created by Platform Administrators via the platform level menu option." +msgstr "This page is for configuring Matchgrid API Users. Platform API Users can only be created by Platform Administrators via the platform level menu option.
The Match API is available at {0}" msgid "match.banner.api_users.platform" -msgstr "This page is for configuring Platform API Users, which have full read/write access to the entire platform. To create API Users restricted to a given Matchgrid, go to the management page for the desired Matchgrid and select API Users from there." +msgstr "This page is for configuring Platform API Users, which have full read/write access to the entire platform. To create API Users restricted to a given Matchgrid, go to the management page for the desired Matchgrid and select API Users from there.
The Match API is available at {0}" ### Command Line text msgid "match.cmd.db.ok" @@ -92,10 +92,6 @@ msgstr "{0,plural,=1{Permission} other{Permissions}}" msgid "match.ct.systems_of_record" msgstr "{0,plural,=1{System of Record} other{Systems of Record}}" -# XXX toss? -# msgid "match.ct.rule_set_attributes" -# msgstr "{0,plural,=1{Rule Set Attribute} other{Rule Set Attributes}}" - msgid "match.ct.rules" msgstr "{0,plural,=1{Rule} other{Rules}}" @@ -200,8 +196,7 @@ msgstr "Save Failed ({0})" msgid "match.er.unauthorized" msgstr "{0} does not have any valid permissions" -# XXX rekey? -msgid "matchgrid.er.search_type" +msgid "match.er.search_type" msgstr "Unknown search type '{0}'" ### Fields diff --git a/app/src/Template/ApiUsers/columns.inc b/app/src/Template/ApiUsers/columns.inc index f42854726..0d2dc59e6 100644 --- a/app/src/Template/ApiUsers/columns.inc +++ b/app/src/Template/ApiUsers/columns.inc @@ -28,9 +28,9 @@ $banners = []; if(!empty($vv_cur_mg)) { - $banners[] = __('match.banner.api_users.matchgrid'); + $banners[] = __('match.banner.api_users.matchgrid', [$vv_api_uri]); } else { - $banners[] = __('match.banner.api_users.platform'); + $banners[] = __('match.banner.api_users.platform', [$vv_api_uri]); } $indexColumns = [