Bug 29484 - ListSets doesn't return noSetHierarchy when appropriate
Summary: ListSets doesn't return noSetHierarchy when appropriate
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Web services (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Jonathan Druart
URL:
Keywords:
Depends on:
Blocks: 21102
  Show dependency treegraph
 
Reported: 2021-11-15 12:50 UTC by Tomás Cohen Arazi
Modified: 2021-12-22 21:38 UTC (History)
6 users (show)

See Also:
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


Attachments
Bug 29484: Regression tests (1.91 KB, patch)
2021-11-15 12:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29484: Make ListSets return noSetHierarchy when no sets defined (1.50 KB, patch)
2021-11-15 12:55 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29484: Regression tests (1.91 KB, patch)
2021-11-25 08:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 29484: Make ListSets return noSetHierarchy when no sets defined (1.55 KB, patch)
2021-11-25 08:26 UTC, David Nind
Details | Diff | Splinter Review
Bug 29484: Regression tests (1.98 KB, patch)
2021-12-02 14:24 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 29484: Make ListSets return noSetHierarchy when no sets defined (1.68 KB, patch)
2021-12-02 14:24 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.