From c4ed417c7e4c521c2c2269b55f4efc6a96c56ca7 Mon Sep 17 00:00:00 2001 From: Kirill Solomko Date: Wed, 10 Jan 2018 15:00:03 +0100 Subject: [PATCH] TT#23771 fix /api/subscriberregistration logic * Fix item update: a new reloaded item is correctly fetched fom the db * Fix Location header: path and item id are correctly delivered with the header * Fix xmlrpc ul.add call params: - correct params order - "expires" param value is properly calculated cherry-picked to mr5.5 (see TT#38607) Change-Id: I194b6bab9cb0d295e0a350e6317f7ddbcebdc021 (cherry picked from commit dbb62078e16e43d4db8c40946b438676d1537ae6) --- .../API/SubscriberRegistrationsItem.pm | 82 +++++++++++++------ .../Panel/Role/API/SubscriberRegistrations.pm | 64 ++++++++++----- lib/NGCP/Panel/Utils/Kamailio.pm | 13 ++- 3 files changed, 108 insertions(+), 51 deletions(-) diff --git a/lib/NGCP/Panel/Controller/API/SubscriberRegistrationsItem.pm b/lib/NGCP/Panel/Controller/API/SubscriberRegistrationsItem.pm index 5e5aa60a40..28a4d74db3 100644 --- a/lib/NGCP/Panel/Controller/API/SubscriberRegistrationsItem.pm +++ b/lib/NGCP/Panel/Controller/API/SubscriberRegistrationsItem.pm @@ -101,34 +101,47 @@ sub OPTIONS :Allow { sub PATCH :Allow { my ($self, $c, $id) = @_; - my $guard = $c->model('DB')->txn_scope_guard; + { my $preference = $self->require_preference($c); last unless $preference; my $json = $self->get_valid_patch_data( - c => $c, + c => $c, id => $id, media_type => 'application/json-patch+json', ); last unless $json; - my $item = $self->item_by_id($c, $id); - last unless $self->resource_exists($c, subscriberregistration => $item); my $form = $self->get_form($c); - my $old_resource = $self->resource_from_item($c, $item, $form); - my $resource = $self->apply_patch($c, $old_resource, $json); - last unless $resource; + last unless $form; + + my ($item, $old_resource, $resource); + my ($guard, $txn_ok) = ($c->model('DB')->txn_scope_guard, 0); + { + $item = $self->item_by_id($c, $id); + last unless $self->resource_exists($c, subscriberregistration => $item); + + $old_resource = $self->resource_from_item($c, $item, $form); + + $resource = $self->apply_patch($c, $old_resource, $json); + last unless $resource; - $item = $self->update_item($c, $item, $old_resource, $resource, $form); + $item = $self->update_item($c, $item, $old_resource, $resource, $form); + last unless $item; + + $guard->commit; + $txn_ok = 1; + } + last unless $txn_ok; + + $item = $self->fetch_item($c, $resource, $form, $item); last unless $item; - - $guard->commit; if ('minimal' eq $preference) { $c->response->status(HTTP_NO_CONTENT); $c->response->header(Preference_Applied => 'return=minimal'); - $c->response->header(Location => sprintf('/%s%d', $c->request->path, $item->id)); + $c->response->header(Location => sprintf('%s%d', $self->dispatch_path, $item->id)); $c->response->body(q()); } else { my $hal = $self->hal_from_item($c, $item, $form); @@ -137,40 +150,54 @@ sub PATCH :Allow { ), $hal->as_json); $c->response->headers($response->headers); $c->response->header(Preference_Applied => 'return=representation'); - $c->response->header(Location => sprintf('/%s%d', $c->request->path, $item->id)); + $c->response->header(Location => sprintf('%s%d', $self->dispatch_path, $item->id)); $c->response->body($response->content); } } + return; } sub PUT :Allow { my ($self, $c, $id) = @_; - my $guard = $c->model('DB')->txn_scope_guard; + { my $preference = $self->require_preference($c); last unless $preference; - my $item = $self->item_by_id($c, $id); - last unless $self->resource_exists($c, subscriberregistration => $item); - my $resource = $self->get_valid_put_data( - c => $c, - id => $id, - media_type => 'application/json', - ); - last unless $resource; my $form = $self->get_form($c); - my $old_resource = $self->resource_from_item($c, $item, $form); + last unless $form; - $item = $self->update_item($c, $item, $old_resource, $resource, $form); - last unless $item; + my ($item, $old_resource, $resource); + my ($guard, $txn_ok) = ($c->model('DB')->txn_scope_guard, 0); + { + $item = $self->item_by_id($c, $id); + last unless $self->resource_exists($c, subscriberregistration => $item); + + $old_resource = $self->resource_from_item($c, $item, $form); - $guard->commit; + $resource = $self->get_valid_put_data( + c => $c, + id => $id, + media_type => 'application/json', + ); + last unless $resource; + + $item = $self->update_item($c, $item, $old_resource, $resource, $form); + last unless $item; + + $guard->commit; + $txn_ok = 1; + } + last unless $txn_ok; + + $item = $self->fetch_item($c, $resource, $form, $item); + last unless $item; if ('minimal' eq $preference) { $c->response->status(HTTP_NO_CONTENT); $c->response->header(Preference_Applied => 'return=minimal'); - $c->response->header(Location => sprintf('/%s%d', $c->request->path, $item->id)); + $c->response->header(Location => sprintf('%s%d', $self->dispatch_path, $item->id)); $c->response->body(q()); } else { my $hal = $self->hal_from_item($c, $item, $form); @@ -179,10 +206,11 @@ sub PUT :Allow { ), $hal->as_json); $c->response->headers($response->headers); $c->response->header(Preference_Applied => 'return=representation'); - $c->response->header(Location => sprintf('/%s%d', $c->request->path, $item->id)); + $c->response->header(Location => sprintf('%s%d', $self->dispatch_path, $item->id)); $c->response->body($response->content); } } + return; } diff --git a/lib/NGCP/Panel/Role/API/SubscriberRegistrations.pm b/lib/NGCP/Panel/Role/API/SubscriberRegistrations.pm index 56fc2dcfaa..6cf5308af6 100644 --- a/lib/NGCP/Panel/Role/API/SubscriberRegistrations.pm +++ b/lib/NGCP/Panel/Role/API/SubscriberRegistrations.pm @@ -143,6 +143,16 @@ sub subscriber_from_id { return $sub; } +sub _item_by_aor { + my ($self, $c, $sub, $contact) = @_; + + return $self->item_rs($c)->search({ + 'me.contact' => $contact, + 'me.username' => $sub->provisioning_voip_subscriber->username, + 'me.domain' => $sub->provisioning_voip_subscriber->domain->domain, + })->first; +} + sub update_item { my ($self, $c, $item, $old_resource, $resource, $form, $create) = @_; @@ -157,13 +167,14 @@ sub update_item { ); my $sub = $self->subscriber_from_id($c, $resource->{subscriber_id}); - return unless($sub); + return unless $sub; unless($create) { $self->delete_item($c, $item); } my $cflags = 0; $cflags |= 64 if($form->values->{nat}); + NGCP::Panel::Utils::Kamailio::create_location($c, $sub->provisioning_voip_subscriber, $form->values->{contact}, @@ -172,28 +183,39 @@ sub update_item { 0, # flags $cflags ); - my $item_reloaded; - - { - NGCP::Panel::Utils::Kamailio::flush($c); - my $rs = $self->item_rs($c); - $item_reloaded = $rs->search({ - 'me.contact' => $form->values->{contact}, - 'me.username' => $sub->provisioning_voip_subscriber->username, - 'me.domain' => $sub->provisioning_voip_subscriber->domain->domain, - })->first; - } - - if($create) { - $item = $item_reloaded; - }else{ - # we need to reload it since we changed the content via an external - # xmlrpc call - $item->discard_changes; - if($item_reloaded){ - $item = $item_reloaded; + + NGCP::Panel::Utils::Kamailio::flush($c); + + return $item; +} + +sub fetch_item { + my ($self, $c, $resource, $form, $old_item) = @_; + + return unless $form; + + my $sub = $self->subscriber_from_id($c, $resource->{subscriber_id}); + return unless $sub; + + my $item; + my $flush_timeout = 5; + + while ($flush_timeout) { + $item = $self->_item_by_aor($c, $sub, $form->values->{contact}); + if ($item && (!$old_item || $item->id != $old_item->id)) { + last; } + $item = undef; + $flush_timeout--; + last unless $flush_timeout; + sleep 1; } + + unless ($item) { + $self->error($c, HTTP_UNPROCESSABLE_ENTITY, "Could not find a new registration entry in the db, that might be caused by the kamailio flush mechanism, where the item has been updated successfully"); + return; + } + return $item; } diff --git a/lib/NGCP/Panel/Utils/Kamailio.pm b/lib/NGCP/Panel/Utils/Kamailio.pm index 209e08fe81..9caede5902 100644 --- a/lib/NGCP/Panel/Utils/Kamailio.pm +++ b/lib/NGCP/Panel/Utils/Kamailio.pm @@ -47,11 +47,18 @@ sub create_location { my $aor = get_aor($c, $prov_subscriber); my $path = $c->config->{sip}->{path} || ''; - if($expires) { + if ($expires) { $expires = NGCP::Panel::Utils::DateTime::from_string($expires)->epoch; + $expires //= 0; + $expires -= time(); + # "expires" is required to be an integer but it is not a timestamp + # <=0: 1970-01-01 00:00:00 + # 1: now + # >=1: now + seconds to the future } else { $expires = 4294967295; } + $expires = 0 if $expires < 0; $flags //= 0; $cflags //= 0; my $dispatcher = NGCP::Panel::Utils::XMLDispatcher->new; @@ -63,12 +70,12 @@ sub create_location { location $aor $contact -0 +$expires $q $flags $cflags -$expires +0 EOF