Bug 29484

Summary: ListSets doesn't return noSetHierarchy when appropriate
Product: Koha Reporter: Tomás Cohen Arazi <tomascohen>
Component: Web servicesAssignee: Tomás Cohen Arazi <tomascohen>
Status: CLOSED FIXED QA Contact: Jonathan Druart <jonathan.druart>
Severity: normal    
Priority: P5 - low CC: andrewfh, david, jonathan.druart, kyle, tomascohen, victor
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
This fixes Koha's OAI-PMH server so that it returns the appropriate error code when no sets are defined.
Version(s) released in:
22.05.00,21.11.01,21.05.08
Bug Depends on:    
Bug Blocks: 21102    
Attachments: Bug 29484: Regression tests
Bug 29484: Make ListSets return noSetHierarchy when no sets defined
Bug 29484: Regression tests
Bug 29484: Make ListSets return noSetHierarchy when no sets defined
Bug 29484: Regression tests
Bug 29484: Make ListSets return noSetHierarchy when no sets defined

Description Tomás Cohen Arazi 2021-11-15 12:50:51 UTC
To reproduce:
$ cpanm HTTP::OAIPMH::Validator
$ PERL5LIB=/root/.local/share/.cpan/build/HTTP-OAIPMH-Validator-1.06-0/lib perl /root/.local/share/.cpan/build/HTTP-OAIPMH-Validator-1.06-0/examples/oaipmh-validator.pl http://kohadev.myDNSname.org:8080/cgi-bin/koha/oai.pl

=> FAIL: There are several errors, some reported under the bug 21102 umbrella
=> FAIL: This error wasn't reported before:

FAIL:    Failed to extract any setSpec elements from ListSets but did not find an exception message. If sets are not supported by the repository then the ListSets response must be the noSetHierarchy error. See <http://www.openarchives.org/OAI/2.0/openarchivesprotocol.htm#ListSets>.
Comment 1 Tomás Cohen Arazi 2021-11-15 12:55:20 UTC
Created attachment 127637 [details] [review]
Bug 29484: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 Tomás Cohen Arazi 2021-11-15 12:55:24 UTC
Created attachment 127638 [details] [review]
Bug 29484: Make ListSets return noSetHierarchy when no sets defined

This simple patch makes our OAI-PMH server return the appropriate error
code when no sets are defined.

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ t/db_dependent/OAI/Server.t
=> FAIL: Tests fail. Boo!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! YAY!
5. Sign off :-D

Bonus: you can check with the tool mentioned on the bug report, that
the error is gone. No sets need to be defined.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 3 David Nind 2021-11-25 08:26:15 UTC
Created attachment 127995 [details] [review]
Bug 29484: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 4 David Nind 2021-11-25 08:26:19 UTC
Created attachment 127996 [details] [review]
Bug 29484: Make ListSets return noSetHierarchy when no sets defined

This simple patch makes our OAI-PMH server return the appropriate error
code when no sets are defined.

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ t/db_dependent/OAI/Server.t
=> FAIL: Tests fail. Boo!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! YAY!
5. Sign off :-D

Bonus: you can check with the tool mentioned on the bug report, that
the error is gone. No sets need to be defined.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 Jonathan Druart 2021-12-02 14:15:26 UTC
I will amend the patch with
-    ) unless scalar @{$sets} > 0;
+    ) unless @$sets;
Comment 6 Jonathan Druart 2021-12-02 14:23:39 UTC
$ cpanm HTTP::OAIPMH::Validator

This fails for me
Comment 7 Jonathan Druart 2021-12-02 14:24:08 UTC
Created attachment 128169 [details] [review]
Bug 29484: Regression tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 8 Jonathan Druart 2021-12-02 14:24:12 UTC
Created attachment 128170 [details] [review]
Bug 29484: Make ListSets return noSetHierarchy when no sets defined

This simple patch makes our OAI-PMH server return the appropriate error
code when no sets are defined.

To test:
1. Apply the regression tests
2. Run:
   $ kshell
  k$ t/db_dependent/OAI/Server.t
=> FAIL: Tests fail. Boo!
3. Apply this patch
4. Repeat 2
=> SUCCESS: Tests pass! YAY!
5. Sign off :-D

Bonus: you can check with the tool mentioned on the bug report, that
the error is gone. No sets need to be defined.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>

JD amended patch
-    ) unless scalar @{$sets} > 0;
+    ) unless @$sets;

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 9 Tomás Cohen Arazi 2021-12-02 14:25:33 UTC
(In reply to Jonathan Druart from comment #5)
> I will amend the patch with
> -    ) unless scalar @{$sets} > 0;
> +    ) unless @$sets;

Why? I prefer what I wrote and have been adding it all the time. If it makes more sense to do it your way, we should make sure it is in the guidelines. I still prefer to have the reader understand we are talking about the list size.
Comment 10 Jonathan Druart 2021-12-02 14:39:28 UTC
(In reply to Tomás Cohen Arazi from comment #9)
> (In reply to Jonathan Druart from comment #5)
> > I will amend the patch with
> > -    ) unless scalar @{$sets} > 0;
> > +    ) unless @$sets;
> 
> Why? I prefer what I wrote and have been adding it all the time. If it makes
> more sense to do it your way, we should make sure it is in the guidelines. I
> still prefer to have the reader understand we are talking about the list
> size.

ok, reverted.

@$sets does exactly what you are describing, just less verbose. Nevermind.
Comment 11 Tomás Cohen Arazi 2021-12-02 15:17:11 UTC
(In reply to Jonathan Druart from comment #10)
> (In reply to Tomás Cohen Arazi from comment #9)
> > (In reply to Jonathan Druart from comment #5)
> > > I will amend the patch with
> > > -    ) unless scalar @{$sets} > 0;
> > > +    ) unless @$sets;
> > 
> > Why? I prefer what I wrote and have been adding it all the time. If it makes
> > more sense to do it your way, we should make sure it is in the guidelines. I
> > still prefer to have the reader understand we are talking about the list
> > size.
> 
> ok, reverted.
> 
> @$sets does exactly what you are describing, just less verbose. Nevermind.

I like your approach, is more Perl-ish, and we should adopt it as a guideline.
Comment 12 Fridolin Somers 2021-12-11 06:16:18 UTC
Pushed to master for 22.05, thanks to everybody involved 🦄
Comment 13 Kyle M Hall 2021-12-17 13:37:39 UTC
Pushed to 21.11.x for 21.11.01
Comment 14 Andrew Fuerste-Henry 2021-12-17 19:55:03 UTC
Pushed to 21.05.x for 21.05.08
Comment 15 Victor Grousset/tuxayo 2021-12-22 21:38:07 UTC
Not backported to oldoldstable (20.11.x). Feel free to ask if it's needed.