Bug 31161 - OAI-PMH - Add ability to exclude suppressed records in output
Summary: OAI-PMH - Add ability to exclude suppressed records in output
Status: NEW
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Bugs List
QA Contact: Testopia
URL:
Keywords:
: 19064 (view as bug list)
Depends on:
Blocks:
 
Reported: 2022-07-15 09:30 UTC by Jonathan Field
Modified: 2024-02-14 05:29 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Field 2022-07-15 09:30:43 UTC
At the moment, if a record is suppressed in Koha it will still appear in the results set of an OAI-PMH query. It would be great to have an option to exclude a record if 942$n is set to suppress. That way OPAC suppression would be reflected in the OAI output.
Comment 1 Jonathan Field 2022-07-15 09:37:57 UTC
Just to add some extra context to this, one of the methods that EBSCO use to harvest data into EDS is OAI-PMH from Koha and some libraries don't want suppressed records going into EDS in the same way they don't want them in the OPAC.
Comment 2 Tomás Cohen Arazi 2022-07-15 12:35:58 UTC
Nice one.
Comment 3 David Cook 2022-07-18 00:09:28 UTC
This has been on my mind for a while. 

I think that we need a database column to store the 942$n data to make it easy to query via SQL.

OpacHiddenItems will still be an issue but that can be a separate issue...
Comment 4 Martin Renvoize 2022-07-22 10:42:24 UTC
I wondered if it was as simple as just running it through RecordProcessor.. but when I started to have a quick look at the code I couldn't wrap my head around it.. perhaps not as simple as it appears on the surface.
Comment 5 David Cook 2022-07-24 23:29:30 UTC
(In reply to Martin Renvoize from comment #4)
> I wondered if it was as simple as just running it through RecordProcessor..
> but when I started to have a quick look at the code I couldn't wrap my head
> around it.. perhaps not as simple as it appears on the surface.

I'd have to do a thorough double-check but I think we need to do the check at the SQL level.
Comment 6 David Cook 2022-07-24 23:34:23 UTC
(In reply to David Cook from comment #3)
> I think that we need a database column to store the 942$n data to make it
> easy to query via SQL.

I still think that this is the solution for this particular issue. 
 
> OpacHiddenItems will still be an issue but that can be a separate issue...

I actually just remembered that I actually have a solution for this! 

Check out the patch on bug 19704. I use SQL::Abstract to generate SQL based on OpacHiddenItems! 

(Note that the 942$n issue on that report is managed using ExtractValue() but it's not as efficient as just having a "biblio" or "biblioitems" column of "suppressed".)
Comment 7 David Cook 2022-07-24 23:34:56 UTC
(In reply to David Cook from comment #6)
> Check out the patch on bug 19704. I use SQL::Abstract to generate SQL based
> on OpacHiddenItems! 

Note that SQL::Abstract is a dependency of DBIx::Class, so it's pretty safe to rely on it. We could always add it to the cpanfile if folk thought it was necessary too.
Comment 8 David Cook 2022-09-27 02:05:42 UTC
Briefly looking at this again in the context of OpacHiddenItems... 

We'd need to either JOIN the items table to biblio_metadata OR we'd need to use a subquery. 

If the subquery were the same as the one used in the UNION, I wonder if it would just cache the 2nd run of the same query. That would be interesting to benchmark...
Comment 9 David Cook 2022-10-09 23:44:27 UTC
(In reply to Martin Renvoize from comment #4)
> I wondered if it was as simple as just running it through RecordProcessor..
> but when I started to have a quick look at the code I couldn't wrap my head
> around it.. perhaps not as simple as it appears on the surface.

Actually, looking at this again, I think running it through RecordProcessor might work. It would be *slow*, but in theory it should work.

The OAI-PMH feed is just a stream of records that we break up using resumption tokens. 

I suppose if OpacHiddenItems is set and OpacHiddenItemsHidesRecord is set to "Don't hide", then the timestamp for records could reflect changes to items that aren't visible in the OAI-PMH feed, but... that is a minor detail and doesn't look avoidable without making SQL changes.

I'm going to email Ere and see what he thinks about this, since he's been behind the latest and greatest OAI-PMH server changes.
Comment 10 David Cook 2022-10-10 00:05:37 UTC
Ah... but the $order_limit specifies a LIMIT which would be a problem if we were filtering out records using RecordProcessor...

That said, we could move the record fetch into a function and run it again if the $count hadn't hit the $max. That would make the response even slower than it already would be using RecordProcessor though...

But I can't think of a simple way to support "OpacHiddenItemsHidesRecord: Hide" except by post-processing outside the SQL.

--

Another thought I had was that maybe we do need to store "hidden_in_opac" in the "biblio" and "items" DB table records. 

If someone updates "OpacHiddenItems", then they need to do a "touch all items". 

In theory, this is a system preference that shouldn't be frequently changed anyway.
Comment 11 David Cook 2022-10-10 00:16:35 UTC
One downside of hiding records/items in the OAI-PMH is that if you turn on your hiding after you've already harvested a Koha DB, then you'll never get the update...

In theory, maybe it makes sense to mark a bib record as deleted - if it's been marked as hidden. 

That might not be compatible with the system preference "OpacSuppressionRedirect" though. If that syspref says "404" and OAI-PMH says "deleted"... that means that the record did once exist...

And since deleted records are shown first and then existent records are shown second, a savvy consumer of OAI-PMH feeds could use it to infer that a record has been hidden rather than deleted...

--

But it's an interesting idea. When I harvest records from Koha using plugins, I'll return "deleted" when a record is "hidden", as that allows newly hidden records/items to be removed from the third-party system automatically. 

--

I suppose that's only really relevant for supporting "OpacSuppression" and "OpacHiddenItemsHidesRecord: Hide"

Technically, "OpacHiddenItems" could be supported now (with the only information leaking being based on update timestamps for hidden items but technically a record could have its timestamp updated with zero bib metadata changed so that's not necessarily revealing).
Comment 12 David Cook 2022-10-10 00:29:12 UTC
I've opened bug 31725 to address the hiding of items using OpacHiddenItems.

And it's got me thinking that the "deleted" status might be the way to go for records hidden using OpacSuppression or OpacHiddenItems.

It's not a 100% perfect solution, but I think it could be a "good enough" solution.
Comment 13 Ere Maijala 2022-10-10 06:21:18 UTC
(In reply to David Cook from comment #12)
> I've opened bug 31725 to address the hiding of items using OpacHiddenItems.
> 
> And it's got me thinking that the "deleted" status might be the way to go
> for records hidden using OpacSuppression or OpacHiddenItems.
> 
> It's not a 100% perfect solution, but I think it could be a "good enough"
> solution.

I think it's the only solution that would work without forementioned side-effects. It's important to report the records as deleted particularly since they might not have been suppressed before. And that'd be the case for practically every vanilla Koha instance harvested with OAI-PMH up until this change.

That said, there are use cases where OAI-PMH is used for non-opac functionality and suppressed records should be included just like before. For flexibility it should be possible to configure suppression per OAI-PMH set in addition to a default setting.
Comment 14 David Cook 2022-10-11 04:49:59 UTC
(In reply to Ere Maijala from comment #13)
> I think it's the only solution that would work without forementioned
> side-effects. It's important to report the records as deleted particularly
> since they might not have been suppressed before. And that'd be the case for
> practically every vanilla Koha instance harvested with OAI-PMH up until this
> change.

I'm glad to hear that you agree!
 
> That said, there are use cases where OAI-PMH is used for non-opac
> functionality and suppressed records should be included just like before.

Do you have some use cases in mind?

> For flexibility it should be possible to configure suppression per OAI-PMH
> set in addition to a default setting.

GetRecord isn't affected by sets, so are you thinking of a flag in the "oai_sets" data that would disable the suppression/hiding of records?

--

I'm planning to trial this hiding locally by using suppression/hiding by default, and then using a system preference to disable it (to cater for any use cases that come up where libraries might need the unfiltered records still).
Comment 15 David Cook 2022-10-11 05:38:25 UTC
So it turns out the OAI-PMH already hides items using OpacHiddenItems:
- Koha::OAI::Server::Repository->get_biblio_marcxml()
- C4::Biblio::GetMarcBiblio
- C4::Biblio::EmbedItemsInMarcBiblio

To avoid double-handling and performance penalties for determining record-level suppression using OpacHiddenItems, it would make sense for C4::Biblio::GetMarcBiblio to return null on a hidden record, and for C4::Biblio::EmbedItemsInMarcBiblio to return a "hide_bib" status if it's hidden all the items via OpacHiddenItems.

--

Of course, C4::Biblio::GetMarcBiblio is a very entrenched function, so I don't think we can alter default behaviour. I'm thinking that we add a parameter like "suppression".

This parameter would be an empty hash reference, which can be checked after C4::Biblio::GetMarcBiblio is called without affecting its return values. 

If OpacSuppression isn't turned on or isn't applicable, then the "suppression" gets passed to C4::Biblio::EmbedItemsInMarcBiblio as well. 

If the "suppression" returns a "hide_biblio" after that call, then C4::Biblio::GetMarcBiblio returns null.
Comment 16 David Cook 2022-10-11 06:17:51 UTC
All right... that wasn't so bad in the end.

A bit of code in C4::Biblio::GetMarcBiblio, C4::Biblio::EmbedItemsInMarcBiblio, Koha::OAI::Server::GetRecord->new, and Koha::OAI::Server::Repository->get_biblio_marcxml

--

There are no new database calls (only syspref calls to cache), so the only overhead added is CPU time and that will be negligible.

--

One could argue that there are alternatives to my hashref parameter idea, but it works.

I'll likely be implementing this locally in 21.11 tomorrow night. 

--

I've run out of time today but it shouldn't be a big drama to add unit tests (at least for the C4::Biblio code).
Comment 17 David Cook 2022-10-11 06:20:58 UTC
I ended up having to add more disparate lines of code than I would've liked, so I will be keen to upstream this one. 

The Koha::OAI::Server::GetRecord change could actually be a separate bug report, since that change isn't contingent on bug 31161.
Comment 18 Ere Maijala 2022-10-11 07:06:29 UTC
(In reply to David Cook from comment #14)
> (In reply to Ere Maijala from comment #13)
> > I think it's the only solution that would work without forementioned
> > side-effects. It's important to report the records as deleted particularly
> > since they might not have been suppressed before. And that'd be the case for
> > practically every vanilla Koha instance harvested with OAI-PMH up until this
> > change.
> 
> I'm glad to hear that you agree!
>  
> > That said, there are use cases where OAI-PMH is used for non-opac
> > functionality and suppressed records should be included just like before.
> 
> Do you have some use cases in mind?

Anything non-OPAC could qualify, e.g. a union catalog based on OAI-PMH. Or even an OPAC instance for staff use only.

> > For flexibility it should be possible to configure suppression per OAI-PMH
> > set in addition to a default setting.
> 
> GetRecord isn't affected by sets, so are you thinking of a flag in the
> "oai_sets" data that would disable the suppression/hiding of records?

GetRecord response has header/setSpec to indicate which sets the record belongs to. It should reflect suppression by excluding a set where the record would be marked deleted. So if we have sets "all" and "opac", a suppressed record would only have <header><setSpec>all</setSpec></header>.
Comment 19 David Cook 2022-10-11 23:24:17 UTC
(In reply to Ere Maijala from comment #18)
> Anything non-OPAC could qualify, e.g. a union catalog based on OAI-PMH. Or
> even an OPAC instance for staff use only.

I was thinking about it again this morning and thinking that it could be any non-public system. 

> > > For flexibility it should be possible to configure suppression per OAI-PMH
> > > set in addition to a default setting.
> > 
> > GetRecord isn't affected by sets, so are you thinking of a flag in the
> > "oai_sets" data that would disable the suppression/hiding of records?
> 
> GetRecord response has header/setSpec to indicate which sets the record
> belongs to. It should reflect suppression by excluding a set where the
> record would be marked deleted. So if we have sets "all" and "opac", a
> suppressed record would only have <header><setSpec>all</setSpec></header>.

I was thinking that a suppressed record wouldn't be retrievable at all by GetRecord if it's suppressed/hidden by the OPAC business rules.

We have libraries who don't want records to leave Koha or be exposed in any public capacity if they're suppressed/hidden, and I think that makes sense.

--

An alternative could be to have public and private OAI-PMH endpoints but that wouldn't necessarily be backwards compatible with all harvesting systems... 

Now that I think about it, we do have non-Koha systems that have public and private OAI-PMH endpoints. It would probably be the cleanest solution, but it would involve work to transition.

Since Koha tries not to change behaviour with upgrades, I suppose the oai.pl would keep the current behaviour, and "public/oai.pl" or "public_oai.pl" would be the endpoint that doesn't send suppressed/hidden record metadata.

--

I suppose there could also be a few system preference options regarding record suppression/hiding. There could be a "full block" where records are suppressed/hidden regardless. Then there could be a "partial block" where records are retrievable by default, unless the "set" says not to block it. And then "no block" would be showing records regardless.

(Maybe "no block" should also turn off the current "OpacHiddenItem" handling too then?)
Comment 20 David Cook 2022-10-11 23:59:50 UTC
*** Bug 19064 has been marked as a duplicate of this bug. ***
Comment 21 Tomás Cohen Arazi 2022-10-12 00:03:24 UTC
Can we add 'supressed' and 'supressed_date' to the biblio table and make our high level methods take care of syncing things?

That would make it easy to build a performant query for identifying (to be marked as) deleted records in OAI.
Comment 22 David Cook 2022-10-12 00:44:43 UTC
(In reply to Tomás Cohen Arazi from comment #21)
> Can we add 'supressed' and 'supressed_date' to the biblio table and make our
> high level methods take care of syncing things?

Can you elaborate on that?

OpacSuppression could work with that, but I don't know how you'd make it work with OpacHiddenItems with OpacHiddenItemsHidesRecord. (Unless you require people to touch_all_biblios.pm after changing OpacHiddenItems and OpacHiddenItemsHidesRecord.)
 
> That would make it easy to build a performant query for identifying (to be
> marked as) deleted records in OAI.

That could be interesting. It would need to be done via a join or a subquery, but it would still probably be fairly performant.
Comment 23 David Cook 2022-10-12 04:28:02 UTC
Just realized that while my changes work well for GetRecord and ListRecords, they don't for ListIdentifiers.

For that, we'll have to fetch the metadata in order to know whether or not it needs to be suppressed/hidden.

That is unless we do the analysis at the SQL level.
Comment 24 David Cook 2022-10-12 04:49:54 UTC
(In reply to David Cook from comment #23)
> Just realized that while my changes work well for GetRecord and ListRecords,
> they don't for ListIdentifiers.
> 
> For that, we'll have to fetch the metadata in order to know whether or not
> it needs to be suppressed/hidden.
> 
> That is unless we do the analysis at the SQL level.

Ugh, and even fetching the metadata doesn't work for OpacHiddenItems unless that metadata format includes items. 

This is so frustrating.
Comment 25 David Cook 2022-10-12 04:58:17 UTC
(In reply to David Cook from comment #22)
> (In reply to Tomás Cohen Arazi from comment #21)
> > Can we add 'supressed' and 'supressed_date' to the biblio table and make our
> > high level methods take care of syncing things?

I think we need to bite the bullet and store "suppressed" when "OpacSuppression" and "OpacHiddenItemsHidesRecord" are set, and just add the caveat that any changes to these settings will require a data regeneration. 

Doing the calculation 1 time and then just working off the stored state will make things so much easier across the board.

Plus, we could index that state, so that "OpacHiddenItemsHidesRecord" actually hides the record in the Zebra results as well, which would lead to better search result quality.
Comment 26 David Cook 2022-10-12 05:57:28 UTC
For now, I will proceed with my patches to solve some immediate local problems, but they're probably not worth upstreaming as they don't comprehensively solve the problem.

For that, I think we need to change how we handle OpacSuppression and OpacHiddenItems and to use SQL for the suppression/hidden lookup.
Comment 27 Ere Maijala 2022-10-12 06:13:20 UTC
The thing is that an OAI-PMH provider should really have a publishing process that handles everything needed to publish a set of records for harvesting and keep it up to date. This would make it possible to:

1. Publish a set once in a format that makes harvesting blazing fast.
2. Update the set incrementally with changes in bibs and items.
3. Re-publish all if suppression rules change.
4. Take care of deletions when they affect the records that have been published in the set. If a record has never been a part of a set, it doesn't need to be returned even as deleted.
5. Ensure that a set doesn't change while harvesting (requires set versioning).

The obvious downside is that a publishing process is relatively heavy, and keeping sets up to date in a near-real-time manner is more difficult. Also sets published this way would require more database space.


--- 
Now to more practical stuff. The multiple endpoints idea works for me. I don't think there's any other interoperable way if you need to have different rules. You could also have several endpoints like 'internal/oai.pl', 'opac/oai.pl' and 'test/oai.pl', no problem. From the protocol point of view they'd just be separate providers. However, they should also have separate OAI identifiers. Things could go really bad if a harvester receives the same record from multiple endpoints.
Comment 28 David Cook 2022-10-12 23:06:17 UTC
(In reply to Ere Maijala from comment #27)
> Now to more practical stuff. The multiple endpoints idea works for me. I
> don't think there's any other interoperable way if you need to have
> different rules. You could also have several endpoints like
> 'internal/oai.pl', 'opac/oai.pl' and 'test/oai.pl', no problem. From the
> protocol point of view they'd just be separate providers. However, they
> should also have separate OAI identifiers. Things could go really bad if a
> harvester receives the same record from multiple endpoints.

According to the OAI spec, OAI identifiers are only unique within their repository/0provider, but I don't think a lot of OAI harvesters adhere to the OAI spec in that regard... so that's a good point.
Comment 29 Ere Maijala 2022-10-13 06:44:58 UTC
(In reply to David Cook from comment #28)
> According to the OAI spec, OAI identifiers are only unique within their
> repository/0provider, but I don't think a lot of OAI harvesters adhere to
> the OAI spec in that regard... so that's a good point.

I'll soften my stance a bit. I suppose it boils down to whether two endpoints are considered the same repository. It might also be enough to just document that one shouldn't harvest from multiple endpoints like they were the same repository.
Comment 30 David Cook 2022-10-13 22:34:54 UTC
(In reply to Ere Maijala from comment #29)
> I'll soften my stance a bit. I suppose it boils down to whether two
> endpoints are considered the same repository. It might also be enough to
> just document that one shouldn't harvest from multiple endpoints like they
> were the same repository.

Yeah, I think documenting could be enough. We could even have separate system preferences. 

On the non-Koha system with private/public endpoints, the private one is restricted by additional security and used for internal projects. The public one is just open to the public, so very different consumers.

But we also set that up with those separate endpoints from the initial launch. 

With Koha, it'll be harder to transition for existing consumers of the OAI-PMH... 

That said, people looking for support for suppressed/hidden records, they'll probably be doing it intentionally and thus be able to coordinate a migration to a new public endpoint more easily...
Comment 31 David Cook 2022-11-02 05:40:50 UTC
FYI on October 12th I updated our OAI-PMH servers to honour OpacSuppression and OpacHiddenItems, so we've been running it in production for nearly a month. 

So far so good.
Comment 32 Martin Renvoize 2022-11-09 14:31:23 UTC
Any chance of sharing those patches David?
Comment 33 David Cook 2022-11-09 22:39:10 UTC
(In reply to Martin Renvoize from comment #32)
> Any chance of sharing those patches David?

They're a bit hacky but I could certainly share them.
Comment 34 David Cook 2023-03-28 06:33:01 UTC
Looks like Koha has changed quite a bit since I wrote those patches so I'll need to refactor them anyway...