From 444ab2e4a5f25a1a4f7afa1393058c1730df9fde Mon Sep 17 00:00:00 2001
From: dbs <dbs@dcc99617-32d9-48b4-a31d-7c20da2025e4>
Date: Sun, 9 Jan 2011 21:27:01 +0000
Subject: [PATCH] Prevent dupe record IDs from being returned by authority
browse/startwith
The 4xx/5xx .ref variants on the authority browse/startwith methods
could return duplicate records if a 1xx/4xx/5xx contained similar entries,
which was highly irritating in the user interfaces.
I haven't been able to figure out a way to use just json_query to return
a set of unduped records that are still ordered appropriately, so I'm
fixing it in post-production... roughly. There is probably a better way
to do this (create an SQL function and select from that, perhaps) but
this at least appears to be an improvement over the current state of affairs.
git-svn-id: svn://svn.open-ils.org/ILS/trunk@19139 dcc99617-32d9-48b4-a31d-7c20da2025e4
---
.../src/perlmods/OpenILS/Application/SuperCat.pm | 286 ++++++++++++---------
1 file changed, 167 insertions(+), 119 deletions(-)
diff --git a/Open-ILS/src/perlmods/OpenILS/Application/SuperCat.pm b/Open-ILS/src/perlmods/OpenILS/Application/SuperCat.pm
index 9285033226..df68ae646b 100644
--- a/Open-ILS/src/perlmods/OpenILS/Application/SuperCat.pm
+++ b/Open-ILS/src/perlmods/OpenILS/Application/SuperCat.pm
@@ -982,76 +982,100 @@ __PACKAGE__->register_method(
);
sub authority_tag_sf_browse {
- my $self = shift;
- my $client = shift;
+ my $self = shift;
+ my $client = shift;
- my $tag = shift;
- my $subfield = shift;
- my $value = shift;
- my $page_size = shift || 9;
- my $page = shift || 0;
+ my $tag = shift;
+ my $subfield = shift;
+ my $value = shift;
+ my $page_size = shift || 9;
+ my $page = shift || 0;
- my ($before_limit,$after_limit) = (0,0);
- my ($before_offset,$after_offset) = (0,0);
+ my ($before_limit,$after_limit) = (0,0);
+ my ($before_offset,$after_offset) = (0,0);
- if (!$page) {
- $before_limit = $after_limit = int($page_size / 2);
- $after_limit += 1 if ($page_size % 2);
- } else {
- $before_offset = $after_offset = int($page_size / 2);
- $before_offset += 1 if ($page_size % 2);
- $before_limit = $after_limit = $page_size;
- }
+ if (!$page) {
+ $before_limit = $after_limit = int($page_size / 2);
+ $after_limit += 1 if ($page_size % 2);
+ } else {
+ $before_offset = $after_offset = int($page_size / 2);
+ $before_offset += 1 if ($page_size % 2);
+ $before_limit = $after_limit = $page_size;
+ }
- my $_storage = OpenSRF::AppSession->create( 'open-ils.cstore' );
+ my $_storage = OpenSRF::AppSession->create( 'open-ils.cstore' );
- # .refs variant includes 4xx and 5xx variants for see / see also
- my @ref_tags = ();
- foreach my $tagname (@$tag) {
- push(@ref_tags, $tagname);
- if ($self->api_name =~ /\.refs\./) {
- push(@ref_tags, '4' . substr($tagname, 1, 2));
- push(@ref_tags, '5' . substr($tagname, 1, 2));
- }
- }
+ # .refs variant includes 4xx and 5xx variants for see / see also
+ my @ref_tags = ();
+ foreach my $tagname (@$tag) {
+ push(@ref_tags, $tagname);
+ if ($self->api_name =~ /\.refs\./) {
+ push(@ref_tags, '4' . substr($tagname, 1, 2));
+ push(@ref_tags, '5' . substr($tagname, 1, 2));
+ }
+ }
+ # We'll remove dupe record IDs that turn up due to 4xx and 5xx matches,
+ # so leave some room for expansion
+ if ($self->api_name =~ /\.refs\./) {
+ $after_limit *= 2;
+ $before_limit *= 2;
+ }
- my @list = ();
+ my @list = ();
+
+ if ($page < 0) {
+ my $before = $_storage->request(
+ "open-ils.cstore.json_query.atomic",
+ { select => { afr => [qw/record value/] },
+ from => { 'are', 'afr' },
+ where => {
+ '+afr' => { tag => \@ref_tags, subfield => $subfield, value => { '<' => lc($value) } },
+ '+are' => { 'deleted' => 'f' }
+ },
+ order_by => { afr => { value => 'desc' } },
+ limit => $before_limit,
+ offset => abs($page) * $page_size - $before_offset,
+ }
+ )->gather(1);
+ push @list, map { $_->{record} } reverse(@$before);
+ }
- if ($page < 0) {
- my $before = $_storage->request(
- "open-ils.cstore.json_query.atomic",
- { select => { afr => [qw/record value/] },
- from => { 'are', 'afr' },
- where => {
- '+afr' => { tag => \@ref_tags, subfield => $subfield, value => { '<' => lc($value) } },
- '+are' => { 'deleted' => 'f' }
- },
- order_by => { afr => { value => 'desc' } },
- limit => $before_limit,
- offset => abs($page) * $page_size - $before_offset,
- }
- )->gather(1);
- push @list, map { $_->{record} } reverse(@$before);
- }
+ if ($page >= 0) {
+ my $after = $_storage->request(
+ "open-ils.cstore.json_query.atomic",
+ { select => { afr => [qw/record value/] },
+ from => { 'are', 'afr' },
+ where => {
+ '+afr' => { tag => \@ref_tags, subfield => $subfield, value => { '>=' => lc($value) } },
+ '+are' => { 'deleted' => 'f' }
+ },
+ order_by => { afr => { value => 'asc' } },
+ limit => $after_limit,
+ offset => abs($page) * $page_size - $after_offset,
+ }
+ )->gather(1);
+ push @list, map { $_->{record} } @$after;
+ }
- if ($page >= 0) {
- my $after = $_storage->request(
- "open-ils.cstore.json_query.atomic",
- { select => { afr => [qw/record value/] },
- from => { 'are', 'afr' },
- where => {
- '+afr' => { tag => \@ref_tags, subfield => $subfield, value => { '>=' => lc($value) } },
- '+are' => { 'deleted' => 'f' }
- },
- order_by => { afr => { value => 'asc' } },
- limit => $after_limit,
- offset => abs($page) * $page_size - $after_offset,
- }
- )->gather(1);
- push @list, map { $_->{record} } @$after;
- }
+ my @retlist = ();
+ my %seen;
+ if ($page < 0) {
+ foreach my $record (reverse @list) {
+ next if exists $seen{$record};
+ unshift @retlist, $record;
+ $seen{$record} = 1;
+ last if scalar(@retlist) == $page_size;
+ }
+ } else {
+ foreach my $record (@list) {
+ next if exists $seen{$record};
+ push @retlist, $record;
+ $seen{$record} = 1;
+ last if scalar(@retlist) == $page_size;
+ }
+ }
- return \@list;
+ return \@retlist;
}
__PACKAGE__->register_method(
method => 'authority_tag_sf_browse',
@@ -1480,68 +1504,92 @@ __PACKAGE__->register_method(
);
sub authority_tag_sf_startwith {
- my $self = shift;
- my $client = shift;
-
- my $tag = shift;
- my $subfield = shift;
- my $value = shift;
- my $limit = shift || 10;
- my $page = shift || 0;
-
- my $offset = $limit * abs($page);
- my $_storage = OpenSRF::AppSession->create( 'open-ils.cstore' );
-
- my @ref_tags = ();
- # .refs variant includes 4xx and 5xx variants for see / see also
- foreach my $tagname (@$tag) {
- push(@ref_tags, $tagname);
- if ($self->api_name =~ /\.refs\./) {
- push(@ref_tags, '4' . substr($tagname, 1, 2));
- push(@ref_tags, '5' . substr($tagname, 1, 2));
- }
- }
-
- my @list = ();
+ my $self = shift;
+ my $client = shift;
+
+ my $tag = shift;
+ my $subfield = shift;
+ my $value = shift;
+ my $limit = shift || 10;
+ my $page = shift || 0;
+
+ my $ref_limit = $limit;
+ my $offset = $limit * abs($page);
+ my $_storage = OpenSRF::AppSession->create( 'open-ils.cstore' );
+
+ my @ref_tags = ();
+ # .refs variant includes 4xx and 5xx variants for see / see also
+ foreach my $tagname (@$tag) {
+ push(@ref_tags, $tagname);
+ if ($self->api_name =~ /\.refs\./) {
+ push(@ref_tags, '4' . substr($tagname, 1, 2));
+ push(@ref_tags, '5' . substr($tagname, 1, 2));
+ }
+ }
+ # We'll remove dupe record IDs that turn up due to 4xx and 5xx matches,
+ # so leave some room for expansion
+ if ($self->api_name =~ /\.refs\./) {
+ $ref_limit *= 2;
+ }
- if ($page < 0) {
- # Don't skip the first actual page of results in descending order
- $offset = $offset - $limit;
+ my @list = ();
+
+ if ($page < 0) {
+ # Don't skip the first actual page of results in descending order
+ $offset = $offset - $limit;
+
+ my $before = $_storage->request(
+ "open-ils.cstore.json_query.atomic",
+ { select => { afr => [qw/record value/] },
+ from => { 'afr', 'are' },
+ where => {
+ '+afr' => { tag => \@ref_tags, subfield => $subfield, value => { '<' => lc($value) } },
+ '+are' => { deleted => 'f' }
+ },
+ order_by => { afr => { value => 'desc' } },
+ limit => $ref_limit,
+ offset => $offset,
+ }
+ )->gather(1);
+ push @list, map { $_->{record} } reverse(@$before);
+ }
- my $before = $_storage->request(
- "open-ils.cstore.json_query.atomic",
- { select => { afr => [qw/record value/] },
- from => { 'afr', 'are' },
- where => {
- '+afr' => { tag => \@ref_tags, subfield => $subfield, value => { '<' => lc($value) } },
- '+are' => { deleted => 'f' }
- },
- order_by => { afr => { value => 'desc' } },
- limit => $limit,
- offset => $offset
- }
- )->gather(1);
- push @list, map { $_->{record} } reverse(@$before);
- }
+ if ($page >= 0) {
+ my $after = $_storage->request(
+ "open-ils.cstore.json_query.atomic",
+ { select => { afr => [qw/record value/] },
+ from => { 'afr', 'are' },
+ where => {
+ '+afr' => { tag => \@ref_tags, subfield => $subfield, value => { '>=' => lc($value) } },
+ '+are' => { deleted => 'f' }
+ },
+ order_by => { afr => { value => 'asc' } },
+ limit => $ref_limit,
+ offset => $offset,
+ }
+ )->gather(1);
+ push @list, map { $_->{record} } @$after;
+ }
- if ($page >= 0) {
- my $after = $_storage->request(
- "open-ils.cstore.json_query.atomic",
- { select => { afr => [qw/record value/] },
- from => { 'afr', 'are' },
- where => {
- '+afr' => { tag => \@ref_tags, subfield => $subfield, value => { '>=' => lc($value) } },
- '+are' => { deleted => 'f' }
- },
- order_by => { afr => { value => 'asc' } },
- limit => $limit,
- offset => $offset
- }
- )->gather(1);
- push @list, map { $_->{record} } @$after;
- }
+ my @retlist = ();
+ my %seen;
+ if ($page < 1) {
+ foreach my $record (reverse @list) {
+ next if exists $seen{$record};
+ unshift @retlist, $record;
+ $seen{$record} = 1;
+ last if scalar(@retlist) == $limit;
+ }
+ } else {
+ foreach my $record (@list) {
+ next if exists $seen{$record};
+ push @retlist, $record;
+ $seen{$record} = 1;
+ last if scalar(@retlist) == $limit;
+ }
+ }
- return \@list;
+ return \@retlist;
}
__PACKAGE__->register_method(
method => 'authority_tag_sf_startwith',
--
2.11.0