Bug 13799

This patch is actually testable on sandbox 16 where I installed the required dependencies. There is no special configuration, so it will run in CGI mode.

Base URL: http://pro.test16.biblibre.com/cgi-bin/koha/rest

Tested this patch locally on a koha dev setup, and some remarks:

* incredibly fast! ~50msec on 50 borrowers...promising!
* few deps:

apt-get install -y make cpanm
cpanm Mojolicious
cpanm Swagger2

* clean and little "glue-code" to Koha
* self-documenting, in a way, with Swagger

I propose that this POC is moved forward to a fully working example covering Borrowers, that can be part of Koha 3.20 if the New_REST_API_RFC (http://wiki.koha-community.org/wiki/New_REST_API_RFC) are fullfilled.

What's missing now:
- authentication
- C4::Borrowers vs Koha::Borrowers?
- versioning
- content negotiation
- full CRUD

(In reply to Benjamin Rokseth from comment #4)
> Tested this patch locally on a koha dev setup, and some remarks:
> 
> * incredibly fast! ~50msec on 50 borrowers...promising!
> * few deps:
> 
> apt-get install -y make cpanm
> cpanm Mojolicious
> cpanm Swagger2

Yes, the dependancies will definitely need to be added to PerlDependancies.pm
 
> * clean and little "glue-code" to Koha
> * self-documenting, in a way, with Swagger
> 
> I propose that this POC is moved forward to a fully working example covering
> Borrowers, that can be part of Koha 3.20 if the New_REST_API_RFC
> (http://wiki.koha-community.org/wiki/New_REST_API_RFC) are fullfilled.

I completely agree, this should definitely move forward!

> What's missing now:
> - authentication

Yes, there are a number of ways for handle this:
1) The way /svc works now
2) Ability to pass in a auth_username and auth_password ( simpler, stateless, easy for command line use of /rest, only safe over https or from localhost )
3) API tokens ( best idea, will require a whole separate feature )

I think one or both of the first two ways would be perfectly acceptable.

> - C4::Borrowers vs Koha::Borrowers?

We should use this as an opportunity to take full advantage of the new Koha::Object system wherever possible.

> - versioning

Versioning should be a simple matter ( as described in http://wiki.koha-community.org/wiki/New_REST_API_RFC )

> - content negotiation

I personally think supporting JSON is adequate, but that's just my opinion. Supporting other formats would be great as long as we implement it at a low enough level that future developers will not have to deal with it ; )

I believe there is already a patch floating around that adds a to_json method to Koha::Object. We could add a method 'serialize' instead that takes a format and could output json, xml, yaml or whatever.

> - full CRUD

Definitely!

This is a great start!

Not sure about the last patch, maybe there's a better way to transform Koha::Object(s) into JSON.

A couple of notes before I get into it too deeply:

libmojolicious-perl in wheezy is 2.98, hopefully it'll work with that.
Swagger2 isn't in Debian at all, but is probably buildable.

More comments:
* version 6.0 is mandated in the dependencies. That's in no Debian release at all right now. We need to ensure it can work on wheezy.
* the httpd conf stuff hasn't been updated in the packages.
* the code is woefully underdocumented, some new files having exactly zero comments at all, let alone POD and copyright headers.

Now to start looking into getting it actually running...

So there's a bug in the version of tar that debian includes that means I can't currently build mojolicious. I'll have to find a workaround to that.

It seems that Swagger2 can't work with Mojolicious versions packaged in Debian wheezy

Tested with Mojolicious 2.98 (wheezy):
  $ perl -MSwagger2 -e ''
  Can't locate encode_json.pm in @INC (@INC contains: /etc/perl /usr/local/lib/perl/5.14.2 /usr/local/share/perl/5.14.2 /usr/lib/perl5 /usr/share/perl5 /usr/lib/perl/5.14 /usr/share/perl/5.14 /usr
  /local/lib/site_perl .) at /usr/share/perl5/Mojo/Base.pm line 32.
  BEGIN failed--compilation aborted at /usr/local/share/perl/5.14.2/Swagger2.pm line 50.
  Compilation failed in require.
  BEGIN failed--compilation aborted.

and with Mojolicious 3.97 (wheezy-backports)
  $ perl -MSwagger2 -e ''
  "encode_json" is not exported by the Mojo::JSON module
   "decode_json" is not exported by the Mojo::JSON module
  Can't continue after import errors at /usr/local/share/perl/5.14.2/Swagger2.pm line 50
  BEGIN failed--compilation aborted at /usr/local/share/perl/5.14.2/Swagger2.pm line 50.
  Compilation failed in require.
  BEGIN failed--compilation aborted.

With Mojolicious 5.54 (jessie) it works.

(In reply to Julian Maurice from comment #16)
> Created attachment 38246 [details] [review] [review]
> Bug 13799: Change Mojolicious minimum version required (5.54)

That won't help because the version we need to support is 2.98. 

I would strongly suggest separating the documentation stuff into another bug, and making the API stuff work with mojolicious 2.98, and deal with the documentation separately and more leisurely.

As it is, I can't see it getting in as no one will be able to run it without manually cpanning things, which isn't an option.

Swagger2 is not only for documentation.
The API code needs Mojolicious::Plugin::Swagger2, which needs Swagger2, which needs Mojolicious >= 5.54 because Swagger2 fails to compile with a smaller version of Mojolicious.

Anyway, Jessie will become the new stable in few days. Should we just wait before integrating this ?

But what about swagger2, which is not packaged at all ?
It is not the only Perl module that needs to be "cpanned" for Koha. So why should we block for this one ? Koha will continue to work without it.

If really needed, it shouldn't be hard to make a Debian package of it.

(In reply to Julian Maurice from comment #18)
> Anyway, Jessie will become the new stable in few days. Should we just wait
> before integrating this ?

We'd be waiting for a year or three, we're only dropping squeeze support as at 3.20.
 
> But what about swagger2, which is not packaged at all ?
> It is not the only Perl module that needs to be "cpanned" for Koha. So why
> should we block for this one ? Koha will continue to work without it.

There are no modules that need to be cpanned for Koha, and if there are, that's a bug (with the exception of some test case things, I think.) I think having a significantly useful feature that can't be readily used isn't good, so I'm trying to get it to a point where it can work.

> If really needed, it shouldn't be hard to make a Debian package of it.

At the moment, it's not particularly easy, as it needs a version of mojolicious that isn't packaged at all. I attempted to package that, but ran into https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=748244 which caused it to fail.

I'll have another go when I get the chance, but depending on bleeding edge modules is always going to be problematic.

(In reply to Robin Sheat from comment #19)
> There are no modules that need to be cpanned for Koha, and if there are,
> that's a bug (with the exception of some test case things, I think.) I think
> having a significantly useful feature that can't be readily used isn't good,
> so I'm trying to get it to a point where it can work.

Ok, so just for pointing it out, required version of DBIx::Class::Schema::Loader (0.07039) is not in wheezy (0.07025-1), and not in debian.koha-community.org neither.

> At the moment, it's not particularly easy, as it needs a version of
> mojolicious that isn't packaged at all. I attempted to package that, but ran
> into https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=748244 which caused
> it to fail.
> 
> I'll have another go when I get the chance, but depending on bleeding edge
> modules is always going to be problematic.

How did you try to package it ? I just tried this morning and was successful packaging Mojolicious 6.08 and Swagger2 0.30 for wheezy (+ additionnal Mojo dependency IO::Socket::IP - packaged in wheezy but version too low)
In the process, I had to make several other packages, but they are only build dependencies (Test::Warnings, Test::Deep, CPAN::Meta::Check)
I only used dh-make-perl and debuild to build packages (and dpkg to install them).

I tested this few weeks ago and I had no issues running them?
Just installed from cpan.
Maybe I missed something?

I ran the manual test cases defined.

We definetely should use the latest Mojolicious and Swagger2 implementations.
No compromise here.
Documentation and all in one bundle like it was meant to be.

Very strange that the latest Mojolicious would be unavailable via packages.

(In reply to Julian Maurice from comment #20)
> Ok, so just for pointing it out, required version of
> DBIx::Class::Schema::Loader (0.07039) is not in wheezy (0.07025-1), and not
> in debian.koha-community.org neither.

That's not needed to run or build Koha. Only the release people need that. Also, IIRC it wasn't feasible to build it without upsetting half the rest of the system.

> How did you try to package it ? 

The proper way for upstreaming it. See here:

https://lists.debian.org/debian-perl/2015/04/msg00009.html

This is the proper way to build a package, as it bases it on the older existing version and hard work has generally been done for you.

> I only used dh-make-perl and debuild to build packages (and dpkg to install
> them).

That won't result in a package acceptable to Debian.

(In reply to Olli-Antti Kivilahti from comment #21)
> I tested this few weeks ago and I had no issues running them?
> Just installed from cpan.

That's fine for testing, just not acceptable for distribution.

> We definetely should use the latest Mojolicious and Swagger2 implementations.
> No compromise here.

No, that's wrong. We can't expect everyone to have it available, and we can't always build it for them. I'm still hopeful we can in this case, but unless that becomes possible, then we have problems.

> Very strange that the latest Mojolicious would be unavailable via packages.

I am a little surprised by that too, to be honest. I would have at least expected it to be in git, unless it's because everyone else is being bitten by this tar bug too.

When I get the chance, I'm going to try to downgrade tar to at least allow things to build. Of course, if someone wants to patch tar for me, that'd be great :)

(In reply to Robin Sheat from comment #22)
> (In reply to Julian Maurice from comment #20)
> > I only used dh-make-perl and debuild to build packages (and dpkg to install
> > them).
> 
> That won't result in a package acceptable to Debian.

But it will be acceptable to debian.koha-community.org, won't it ?

(In reply to Julian Maurice from comment #23)
> (In reply to Robin Sheat from comment #22)
> > (In reply to Julian Maurice from comment #20)
> > > I only used dh-make-perl and debuild to build packages (and dpkg to install
> > > them).
> > 
> > That won't result in a package acceptable to Debian.
> 
> But it will be acceptable to debian.koha-community.org, won't it ?

No, we build proper packages for debian.koha-community.org and upstream them also. eg https://qa.debian.org/developer.php?login=robin@catalyst.net.nz this gets them into debian proper, and from there into ubuntu.

Regardless of the packages, I think this should be split into a bunch of bugs, certainly shifting the documentation one off on to its on thing. I don't think we need to build any front end stuff for this. If we do that should be on it's on bug also. 
To get something like this in, its much better to do what was done with the acquisitions work, seperate bugs that don't mean the entire thing has to pass at once.

(In reply to Chris Cormack from comment #24)
> Regardless of the packages, I think this should be split into a bunch of
> bugs, certainly shifting the documentation one off on to its on thing. I
> don't think we need to build any front end stuff for this. If we do that
> should be on it's on bug also. 
> To get something like this in, its much better to do what was done with the
> acquisitions work, seperate bugs that don't mean the entire thing has to
> pass at once.

I disagree. The "front end stuff" is the API documentation, and I don't think we can integrate the new API without a proper documentation.

(In reply to Robin Sheat from comment #19)
> (In reply to Julian Maurice from comment #18)
> > Anyway, Jessie will become the new stable in few days. Should we just wait
> > before integrating this ?
> 
> We'd be waiting for a year or three, we're only dropping squeeze support as
> at 3.20.

With 3.20 around the corner is this really an issue? It just means that it wouldn't be backportable to earlier versions of Koha, correct? Considering this is an enhancement I wouldn't expect it to be backported anyway. Are there other considerations I'm missing?

(In reply to Julian Maurice from comment #25)
> (In reply to Chris Cormack from comment #24)
> > Regardless of the packages, I think this should be split into a bunch of
> > bugs, certainly shifting the documentation one off on to its on thing. I
> > don't think we need to build any front end stuff for this. If we do that
> > should be on it's on bug also. 
> > To get something like this in, its much better to do what was done with the
> > acquisitions work, seperate bugs that don't mean the entire thing has to
> > pass at once.
> 
> I disagree. The "front end stuff" is the API documentation, and I don't
> think we can integrate the new API without a proper documentation.

I would tend to agree with Julian on this. The documentation is part and parcel of the api. I don't see any advantage to splitting it into a separate bug.

(In reply to Kyle M Hall from comment #26)
> (In reply to Robin Sheat from comment #19)
> > (In reply to Julian Maurice from comment #18)
> > > Anyway, Jessie will become the new stable in few days. Should we just wait
> > > before integrating this ?
> > 
> > We'd be waiting for a year or three, we're only dropping squeeze support as
> > at 3.20.
> 
> With 3.20 around the corner is this really an issue? It just means that it
> wouldn't be backportable to earlier versions of Koha, correct? Considering
> this is an enhancement I wouldn't expect it to be backported anyway. Are
> there other considerations I'm missing?

Ignore this comment, I had my versions crossed. Any idea what the possibility of taking the package for Jesse and using it on Wheezy is?

Another idea, can't we have a temporary package for Mojo even if it's not proper for upstreaming? Once the debian mojo package hit's the right version we could remove our own package. Would that work?

This feels like making work for users and package maintainers, to make something arguably easier for developers. This is something that has made a mess for us a lot in the past. 

However, we probably should not be worrying about this, as it's missed the window for 3.20 already and be expending energy on testing and fixing bugs. We can argue about adding unnessecary dependencies again after the release.

(In reply to Kyle M Hall from comment #28)
> Ignore this comment, I had my versions crossed. Any idea what the
> possibility of taking the package for Jesse and using it on Wheezy is?

Dependency packages need to be actually built for the environment, we can't just grab a .deb file and throw it in.

(In reply to Kyle M Hall from comment #29)
> Another idea, can't we have a temporary package for Mojo even if it's not
> proper for upstreaming? Once the debian mojo package hit's the right version
> we could remove our own package. Would that work?

That's exactly what I'm trying to avoid. Now it's something I have to maintain for the next few years. Hence my policy being that if I can't make a package that can go into Debian, then I'm not really willing to put it into the Koha repo. This also forces me to check the package for overall worthiness (e.g. people depending on modules that have no copyright notice, therefore we can't legally redistribute - not the case here though, this is just simply unbuildable until I find a version of tar without the bug.)

If you want to help out on this front, have a look here: http://wiki.koha-community.org/wiki/Building_Debian_Dependencies

(In reply to Robin Sheat from comment #31)
> (In reply to Kyle M Hall from comment #28)
> > Ignore this comment, I had my versions crossed. Any idea what the
> > possibility of taking the package for Jesse and using it on Wheezy is?
> 
> Dependency packages need to be actually built for the environment, we can't
> just grab a .deb file and throw it in.

I tried to build Mojolicious package for Wheezy from here: git://git.debian.org/git/pkg-perl/packages/libmojolicious-perl.git
I had to install several *build* dependencies from Jessie, but the resulting binary package should work well on wheezy.
In Jessie libmojolicious-perl depends on libjs-prettify which is not in Wheezy so I had to also build this package.
Building source packages requires much more work, as there is a lot of build dependencies not packaged in wheezy, but do we need them ?

(for libjs-prettify I retrieved files from packages.debian.org and built from there)

(In reply to Julian Maurice from comment #32)
> I tried to build Mojolicious package for Wheezy from here:
> git://git.debian.org/git/pkg-perl/packages/libmojolicious-perl.git

This is version 5.54 though. The problem is when you run uscan to create the DFSG-compliant source tarball of version 6.08, and swagger2 declares a dependency on Mojolicious 6.0+, so changing the minimum version that Koha requires doesn't change a whole lot.

> I had to install several *build* dependencies from Jessie, but the resulting
> binary package should work well on wheezy.
> In Jessie libmojolicious-perl depends on libjs-prettify which is not in
> Wheezy so I had to also build this package.
> Building source packages requires much more work, as there is a lot of build
> dependencies not packaged in wheezy, but do we need them ?

We will need the source packages, because we need to build them. This is because we need to modify the changelogs to specify which pockets of the repo it's going to go into.

I avoid doing it in hacky ways, or ways that are not Debian-compliant, because that ends up causing maintenance headaches and more unnecessary work in the long run, and will also interfere with my planned efforts to automate some parts of this process.

All this said, I just downgraded tar in my package building VM to the version from squeeze, and uscan succeeds. So there is hope yet.

(In reply to Robin Sheat from comment #33)
> All this said, I just downgraded tar in my package building VM to the
> version from squeeze, and uscan succeeds. So there is hope yet.

Yay! \o/

About 5.54 vs 6.0, maybe we could ask the author of Swagger2 to change the required version of Mojolicious (if Swagger2 is proven to work with 5.54 of course). What do you think ? Will that make things easier ?
On the other hand, having the latest version of Mojolicious into Debian would be great if this doesn't require extra work.

(In reply to Julian Maurice from comment #34)
> About 5.54 vs 6.0, maybe we could ask the author of Swagger2 to change the
> required version of Mojolicious (if Swagger2 is proven to work with 5.54 of
> course). What do you think ? Will that make things easier ?

It certainly would, but it might be the difference between 5 minutes and half and hour, or 5 minutes and 5 weeks :) it remains to be seen.

Just asked: https://github.com/jhthorsen/swagger2/issues/14

New version of Swagger2 on CPAN (0.31) now requires Mojolicious 5.54

Darn, messed up order when fixing resolve issue on first patch.
Will fix ASAP

libmojolicious-perl_5.54+dfsg-1~koha1_all.deb (etc.) and libjs-prettify_2013.03.04+dfsg-4~koha1_all.deb (etc.) are now in the repo. I had to install Java on my build vm to get that last one to build. I hope you're happy! ;)

Now to look into swagger2.

And, libswagger2-perl is in the repo now too.

Great! Thanks Robin!

Wow! Awesome work! Good karma!

What are the next steps here?

(In reply to Katrin Fischer from comment #51)
> What are the next steps here?

Someone to do some testing and signoffing. It's on my list to look at.

Comment on attachment 39163 [details] [review]
Bug 13799: Fix Koha::Objects::find when no results are found

Bug 13799: Fix Koha::Objects::find when no results are found
is obsoleted now by 
Bug 13967 - System preferences need a package ??

(In reply to Olli-Antti Kivilahti from comment #54)
> Comment on attachment 39163 [details] [review] [review]
> Bug 13799: Fix Koha::Objects::find when no results are found
> 
> Bug 13799: Fix Koha::Objects::find when no results are found
> is obsoleted now by 
> Bug 13967 - System preferences need a package ??

I think so.

Comment on attachment 40094 [details] [review]
Bug 13799 - Set up logging for Mojolicious.

>From 17fe4147396127c87be5d0270ad78a7d44576cd5 Mon Sep 17 00:00:00 2001
>From: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi>
>Date: Thu, 11 Jun 2015 17:48:54 +0300
>Subject: [PATCH] Bug 13799 - Set up logging for Mojolicious.
>
>Somebody smarter can set the logging path as dynamic.
>---
> Koha/REST/V1.pm |    4 ++++
> 1 file changed, 4 insertions(+)
>
>diff --git a/Koha/REST/V1.pm b/Koha/REST/V1.pm
>index 3becf12..197c17b 100644
>--- a/Koha/REST/V1.pm
>+++ b/Koha/REST/V1.pm
>@@ -2,10 +2,14 @@ package Koha::REST::V1;
> 
> use Modern::Perl;
> use Mojo::Base 'Mojolicious';
>+use Mojo::Log;
> 
> sub startup {
>     my $self = shift;
> 
>+    $self->app->log( Mojo::Log->new( path => "/home/koha/koha-dev/var/log/kohaapi.mojo.log", level => 'debug' ) );
>+
>+
>     my $route = $self->routes->under->to(
>         cb => sub {
>             my $c = shift;
>-- 
>1.7.9.5

How will that interact with Plack? Environment variables aren't the easiest thing to pass through to that at this stage.

(In reply to Robin Sheat from comment #63)
> How will that interact with Plack? Environment variables aren't the easiest
> thing to pass through to that at this stage.

I think this will be overwritten by plack.
I just created this system because there was no other system published in bugzilla for me to start building stuff on top of the Koha REST API.

I think the big question is how to pass variables to Mojolicious app, without using env-variables. I don't know and the solution is good enough for me.

What I would be worried of, is the redirection of STDERR and STDOUT to /dev/null in Mojo::Server::damonize().
Thus logging those streams from the service-script doesn't work, but it works when running Mojolicious directly from the terminal.

Maybe we could add some of the open questions/topics to the wiki? The next dev meeting is supposed to be mostly about the RESTful API.

Olli, while I'm not against the idea to moving into nginx and/or hypnotoad, I think putting this patches here just make more confusion.

If it is possible, could you please move your integration work into the wiki? So on this bug we discuss the REST patches only?
If you have a serious proposal of moving away from Apache, you should definitely put that on a new bug report.

AS I see it, your patch(es) aid testing the REST implementation, so they are really useful. Just move them outside this specific bug. Please :-D

Also Olli, you replaced all patch titles by 'Rebased', making it harder to see which patch does what. Could you please fix that ?

Julian,
1) This needs a squash, to avoid add/del of the same file/dir in the same patch set and an up-to-date test plan.

2) /v1/doc returns "Page not found... yet!"

3) /v1/borrowers/7853
Check the encoding, I get 
    "address2":"éééصةصةصة"

With
MariaDB [koha]> select address2 from borrowers where borrowernumber=7874\G
*************************** 1. row ***************************
address2: éééصةصةصة

Comment on attachment 40469 [details] [review]
Rebased

Patch moved to new bug 14448

Comment on attachment 40470 [details] [review]
Rebased

Patch moved to new bug 14448

Comment on attachment 40471 [details] [review]
Bug 13799 - Nginx reverse proxy config baseline for Koha REST API. Tweaking needed to work with makefile.

Patch moved to new bug 14448

Squashed all patches + minor encoding bug fix

The order of applying these following bugs is as follow:
13799 -> 13895 -> 13903 -> 13920

Swagger UI removed from the first patch and moved in its own bug 14458

*** Bug 13920 has been marked as a duplicate of this bug. ***

Comment on attachment 41540 [details] [review]
Bug 13799 - 2. Set up logging and configuration file reading for Mojolicious.

Patch moved to bug 14746

Comment on attachment 41539 [details] [review]
Bug 13799 - 1.1 RESTful API with reverse proxy configuration without api.host.domain

Patch moved to bug 14747

Comment on attachment 41542 [details] [review]
Bug 13799: 4. API Authentication, part 1: API keys management in interface

Patch moved to bug 13920

Comment on attachment 41543 [details] [review]
Bug 13799: 5. API authentication system - Swagtenticator authentication

Patch moved to bug 13920

Comment on attachment 41547 [details] [review]
Bug 13799 - 6. REST API Testing framework

Patch moved to bug 14748

All patches moved to their own bugs so we have a simple patch that just works (I hope...) and without a huge dependency tree.
It should make testing a lot easier.

A feature is still missing however: the cookie-based authentication. I'll try to work on that soon.

(In reply to Julian Maurice from comment #134)
> A feature is still missing however: the cookie-based authentication. I'll
> try to work on that soon.
... but feel free to test the patch right now! :)

reattached the patch because i've changed the webservices url in the commit message (http://(OPAC|INTRANET)/api/v1/borrowers instead of http://(OPAC|INTRANET)/v1/borrowers)

The 'borrowers' permission is required for both /borrowers and /borrowers/XXXX except if XXXX is the borrowernumber of the loggedinuser

Test plan for authentication:
  1/ Log in to staff interface with a borrower that have 'borrowers' permission
  2/ Go to http://INTRANET/api/v1/borrowers and http://INTRANET/api/v1/borrowers/XXXX (where XXXX is a valid borrowernumber). You should see borrowers data
  3/ Remove the 'borrowers' permission
  4/ Go to http://INTRANET/api/v1/borrowers. You should see an error (and HTTP code 403)
  5/ Go to http://INTRANET/api/v1/borrowers/XXXX (where XXXX is a valid borrowernumber different from the logged-in user's borrowernumber). You should see an error (and HTTP code 403)
  6/ Go to http://INTRANET/api/v1/borrowers/XXXX (where XXXX is the logged-in user's borrowernumber). You should see borrower's data.

Patch complexity is 'Large' because this change has many architectural connections

Hi there!

We cannot push this bug to master without also pushing Bugs 13920 and Bug 14748.
Bug 14748 doesn't need to be implemented like I implemented it, but we must have a established and formal way of doing the complete authentication, permission checking and formal enforced testing, before unleashing this monster to master.

If we push just this Bug, people will start creating features on top of this one, and then we will lose the opportunity to integrate automatic documentation driven permission checking and a formal testing framework (which simplifies test writing a lot).
It will be much harder to include them later on.

Yes, we can, and we should, as we agreed on that during the previous dev meeting.
It's better to have a tiny patch in master than a big, full-featured patch that will never be pushed because of its complexity.
And the API key authentication system needs some discussion, as some people disagree with it.

Hi!

Had a discussion in IRC with jajm and ashimema about REST API permission checking. And since the author of Mojolicious:Plugin::Swagger2 has kindly improved the plugin per the discussions I have had with him.
I decided to utilize the new around-action -hook of the Swagger2-plugin to implement Swagger2-driven permission checking for the REST API authentication mechanism.

I really need to start migrating another legacy ILS to Koha, but it would be a real shame to let this great improvement pass.

Commit "RESTful API with Mojolicious and Swagger2" is missing license notices from the start of some files.

I have a few concerns using x-mojo-around feature of Swagger2:
1/ It is marked as experimental (from the POD: "This feature is EXPERIMENTAL and can change without notice.")
2/ It is introduced only in Swagger2 0.45 and thus we'll need to repackage it for Debian (packaged version in debian.koha-community.org is only 0.31)

I think these two points will block integration, so I'm wondering if it's not better to include the patch in a new bug. What do you think ?

No movement for a week, I'll move the last patch into its own bug.

Comment on attachment 42518 [details] [review]
Bug 13799 - Swagger2-driven Permission checking

Patch moved to its own bug (bug 14868)

I tested the patches successfully on a dev install. I didn't manage to have it working on a packages+Plack environment. Once I have the needed followups I'll sign this successfull implementation.

Just a reminder before I scurry off again.

We also need to solve how to use Etags to prevent mid-air collissions when modifying DB-objects.

This is currently a BIG issue in Koha and causes very hard to track and understand issues.
Getting it solved using the new REST API would be super awesome and 100% inline (actually a REQUIREMENT) with the REST architecture.

Testing so far looks good! A couple things I'd like to address before passing qa:
1) Accessing invalid paths gives us a Mojo generated page that gives data that while useful for development should probably just give us 404's in production. How do you think we should address this?

2) One of the bits of data from that invalid page leads me to this: http://mojolicio.us/perldoc/Mojolicious/Guides/FAQ#What-does-Your-secret-passphrase-needs-to-be-changed-mean
   Is this something we should be setting? Perhaps a hard coded default secret for Koha and the option to specify a different one in koha-conf.xml?

3) A wiki page for RESTful api developer guidelines would be most helpful. Nothing too fancy, just a "here are the bits you need to change and add to extend the REST api". The code itself is a good document but an actual guide would certainly be helpful.

Setting to failed qa until 1 and 2 are addressed. 3 is more of a wish list item.

I have also started QAing this one yesterday.
Back today with few comments:
1/ 404 is not documented in the swagger.json file
2/ The properties for "borrowers" are not exhaustive
3/ Wording: borrowers vs patrons? "Patron" should be used now, especially for new code.
4/ this patch set introduces swagger.json but nothing to use it, how is it useful?
5/ We need a way to split this json file, it will be a mess to edit!
And something to validate it. We need guidelines.
6/ Koha::Borrowers->find is called in Koha::REST::V1::Borrowers::get_borrower and Koha::REST::V1->startup (?)

(In reply to Jonathan Druart from comment #161)
> 6/ Koha::Borrowers->find is called in
> Koha::REST::V1::Borrowers::get_borrower and Koha::REST::V1->startup (?)

Forget that!
(I am hidden already).

(In reply to Jonathan Druart from comment #161)
> 1/ 404 is not documented in the swagger.json file

Forget that again, it is!

(In reply to Jonathan Druart from comment #161)
> 2/ The properties for "borrowers" are not exhaustive
> 3/ Wording: borrowers vs patrons? "Patron" should be used now, especially
> for new code.

Ok, these points need to be fixed

> 4/ this patch set introduces swagger.json but nothing to use it, how is it
> useful?

swagger.json, apart from being the api documentation, is also used by Mojolicious' plugin Swagger2 to create the routes (map URL to controller/action) and do some basic type check on input and output. So it is useful (or at least, used) even without documentation generators like SwaggerUI

> 5/ We need a way to split this json file, it will be a mess to edit!

There is an issue on Github for splitting the JSON spec
https://github.com/jhthorsen/swagger2/issues/33
which was moved to
https://github.com/jhthorsen/json-validator/issues/4
which ends with a commit in master
https://github.com/jhthorsen/json-validator/commit/5d7d9db22b891758ac979cc87f1ab018030df2c2

It seems there is something to test here.

> And something to validate it. We need guidelines.

What kind of validation ?

(In reply to Julian Maurice from comment #166)
> (In reply to Jonathan Druart from comment #161)
> > 2/ The properties for "borrowers" are not exhaustive
> > 3/ Wording: borrowers vs patrons? "Patron" should be used now, especially
> > for new code.
> 
> Ok, these points need to be fixed

Agreed. The external api terminology must be patron / patrons at the least. At some point we need to change Koha::Borrower(s) to Koha::Patron(s) but that is entirely separate and distinct from this work.
 
> > 5/ We need a way to split this json file, it will be a mess to edit!
> 
> There is an issue on Github for splitting the JSON spec
> https://github.com/jhthorsen/swagger2/issues/33
> which was moved to
> https://github.com/jhthorsen/json-validator/issues/4
> which ends with a commit in master
> https://github.com/jhthorsen/json-validator/commit/
> 5d7d9db22b891758ac979cc87f1ab018030df2c2
> 
> It seems there is something to test here.

Agreed. It would be *much* better if we can split this file up. If the Swagger2 version we are using doesn't support it, we may have to table this issue until a newer version is available for us to use with Koha.
 
> > And something to validate it. We need guidelines.
> 
> What kind of validation ?

I think a unit test using Test::JSON::is_valid_json should be sufficient.

Also, thanks for the followups Julian!

I've been working allot lately with the developer of the Swagger2 perl tools. We have got $ref support working well now, with relative paths, full paths and url's all passing tests.  I asusme it's this piece you were refering to when talking about splitting up the spec file khall?


Regarding 'patrons' vs 'borrowers' can we not use the 'americanised' term pretty please.. to me it would be much better to generalise it.. they're not limited to borrowers/patrons at all.. they're 'users' ;)

Back on topic, I'll be looking at all this stuff today.. great work Julian in my brief look so far.

(In reply to Martin Renvoize from comment #170)
> I've been working allot lately with the developer of the Swagger2 perl
> tools. We have got $ref support working well now, with relative paths, full
> paths and url's all passing tests.  I asusme it's this piece you were
> refering to when talking about splitting up the spec file khall?

Yes, I believe so.

> Regarding 'patrons' vs 'borrowers' can we not use the 'americanised' term
> pretty please.. to me it would be much better to generalise it.. they're not
> limited to borrowers/patrons at all.. they're 'users' ;)

The general consensus has been for internal terminology to match outward terminology. Right now we use patrons, members and borrowers internally. Since we use patrons externally we should use it internally. I we want to change the nomenclature further we should put it to a dev meeting vote!

> Back on topic, I'll be looking at all this stuff today.. great work Julian
> in my brief look so far.

Excellent!

Firs comment: 

Unless we're actually going to use Mojolicious Sessions, and thus mojo cookies then we need not confuse users by making the secret configurable.

As far as I can tell, secrets are only used to sign mojo cookies (Whether we should eventually switch to mojo hmac signed cookies over csgisessid is another question all together)

As such, I would suggest we hard code a suitably silly secret just to quell warnings that will be output otherwise safe in the knowledge that we are not actually using said secret anyway.

x-mojo-controller has recently been deprecated, though not entirely removed from the Swagger2 Mojo Plugin, withe the prefernce now being that the core swagger key 'operationId' is used in preference to it.

I'm happy to submit a patch that updates both our spec and the resultant method names in the Borrowers controller.

Error responses..  I see you've added a nice simple schema for validating error responses.

However, there seems to be a general move toward standardising on the error format at described by the Swagger2 plugins error handler..

Perhaps we should follow suite and adopt that same format, which is pretty darn close to what we have already (with the exception that it allows for an array of errors with messages as a posed to just a single error message string).

I'm also happy to supply a patch if this is how we wish to proceed.

Rebased my two patches (marked with 'Followup' keyword) and signed off on prior patches.

QA Note, let's get he patrons example right and not only define all fields with descriptions but actually define types so validation may become useful.

I'm happy to do this as another followup.

My patches require the latest versions of Swagger2 and JSON::Validator modules.  I highly recommend especially whilst in development that we stick to the latest versions of these modules...  The code is moving fast and becoming more and more refined almost daily.

The latest changes include..

* Validating the swagger.json specification file against the swagger schema during application startup (This should prevent 'bad schema' making it into release).
* Correct handling of $ref json-references in schemata
* Stricter coercion functions and validation (JSON::Boolean and Integer coercion handling improvements)

and various other bugfixes.

(In reply to Martin Renvoize from comment #189)
> Rebased my two patches (marked with 'Followup' keyword) and signed off on
> prior patches.
> 
> QA Note, let's get he patrons example right and not only define all fields
> with descriptions but actually define types so validation may become useful.
> 
> I'm happy to do this as another followup.

Please do! Once you've got that dealt with I'll continue with QA

I'm having problems with the latest patches

When I go to http://pro.koha-dev/api/v1/patrons/7680, error:

  Can't use string ("error.json") as a HASH ref while "strict refs" in use at /usr/local/share/perl/5.14.2/JSON/Validator.pm line 515.

If I remove patch "Followup: Refactor spec file", another error:

  {"errors":[{"path":"\/","message":"Controller could not be loaded."}]}

Then, if I remove patch "Followup: x-mojo-controller deprecation", everyting is ok.

$ pmvers JSON::Validator
0.59

$ pmvers Swagger2
0.60

Removing 'type' from the definitions removes validation of the requests and responses.. that loses 2/3rds of the reason for using swagger with the final 3rd being the documentation of the api which by removing 'types' your also massively limiting the usefulness of.

If we're not using 'null' as a meaningful value then we should not be using it at all.  Also, the definitions still do not contain a 'requires' block which should really be present to show what fields are always required.  For a fully restful api, this would be all of them for POST, PUT and GET as rest is all about dealing with full documents.

Started writing some guidelines: http://wiki.koha-community.org/wiki/Coding_Guidelines_-_API

Still very much a work in progress

My current feeling now is that this is a great base and should be pushed sooner rather than later.

I expect as we refine the guidelines and documentation I will continue to update the Patrons route to conform to the evolving guidelines.

As such, I don't see any of the outstanding comments being blockers for the push.

(In reply to Martin Renvoize from comment #198)
> My current feeling now is that this is a great base and should be pushed
> sooner rather than later.
+1

Required versions of dependencies are now in Debian Sid

https://packages.debian.org/sid/libswagger2-perl
https://packages.debian.org/sid/libjson-validator-perl

Now we need to re-package them for debian.koha-community.org

(In reply to Julian Maurice from comment #214)
> Required versions of dependencies are now in Debian Sid
> 
> https://packages.debian.org/sid/libswagger2-perl
> https://packages.debian.org/sid/libjson-validator-perl
> 
> Now we need to re-package them for debian.koha-community.org

(In reply to Julian Maurice from comment #215)
> Created attachment 44285 [details] [review] [review]
> Bug 13799: Update dependencies

Is it necessary to update those versions? Because the older versions are in the repo as it is.

I haven't tried yet, but I suspect that JSON::Validator will be hard to port to wheezy, as it requires packaging a newer mojolicious, and I fear we'll end up in a bit of dependency hell. I'll give it a go though, maybe it'll be surprisingly easy.

(In reply to Robin Sheat from comment #217)
> I had a look, and I don't think it's going to happen. Attached is the
> dependency tree that I worked out I'd need to build. The red edge is where
> the process becomes difficult, and I'm not going to backport a new Perl
> version.

Actually, it's possible I spoke too soon. But it's still going to be a bit of work, so in the interests of laziness, how important are these new versions?

There's still no guarantee that it'll work overall, but I may have found a way around the perl version issue.

(In reply to Robin Sheat from comment #218)
> Actually, it's possible I spoke too soon. But it's still going to be a bit
> of work, so in the interests of laziness, how important are these new
> versions?

The newer version of Swagger2 module is useful to be able to split the swagger.json specification file into multiple pieces (so we don't have a single huge unreadable file)
I don't know if there are others benefits.

We jumped on the Swagger2 Plugin way too early basically.

I've been developing an app based around it for a few months now... and have been working closely with the plugin author to write tests, fix issues and make it stable.. it's only just hitting the point where it's a stable and reliable platform for us to work atop so I wouldn't suggest using earlier versions. (Unless we wan't to fork it and maintain that fork.. which I think is a terrible idea).

For further info, I wouldn't even consider using Mojolicious in a package environment if we could avoid it.. I've voiced this before though ;).  Mojolicious is a state of the art library that's pretty fast moving.  I use it in our companies apps, but we're running in a Carton + Plenv environment and I wouldn't really advocate running a Mojo stack under much else.

Being able to split the swagger.json to many files is really important.
The single file will become too big too soon.

(In reply to Olli-Antti Kivilahti from comment #221)
> Being able to split the swagger.json to many files is really important.
> The single file will become too big too soon.

Yeah, but we can leave that for the next release; at that point we won't have the compromise to suport Wheezy or Trusty at all.

(In reply to Tomás Cohen Arazi from comment #222)
> (In reply to Olli-Antti Kivilahti from comment #221)
> > Being able to split the swagger.json to many files is really important.
> > The single file will become too big too soon.
> 
> Yeah, but we can leave that for the next release; at that point we won't
> have the compromise to suport Wheezy or Trusty at all.

So, what should we do ?
Moving the patches that need more recent versions of Perl modules into their own bug and push remaining patches into master now (so they go in 3.22) ?
Or wait for 3.22 to be released and then push the whole bug's patches for 3.24 ?

Backporting Perl to oldstable doesn't seem to be the way to go

Created attachment 44375 [details] [review]
Bug 13799: RESTful API with Mojolicious and Swagger2

Rebased on master

Created attachment 44376 [details] [review]
Bug 13799: Add cookie-based authentication to REST API

Rebased on master

Created attachment 44377 [details] [review]
Bug 13799: Add missing license notices

Rebased on master

Created attachment 44378 [details] [review]
Bug 13799: Fix API unit tests

Rebased on master

Created attachment 44379 [details] [review]
Bug 13799: (QA followup) make tests use random data

Rebased on master

Created attachment 44380 [details] [review]
Bug 13799: Change Mojolicious default mode to 'production'

Rebased on master

Created attachment 44381 [details] [review]
Bug 13799: Allow to set Mojolicious secret in $KOHA_CONF

Rebased on master

Created attachment 44382 [details] [review]
Bug 13799: Change 'required' value type from string to boolean

Rebased on master

Created attachment 44383 [details] [review]
Bug 13799: Reword 'borrower' to 'patron'

Rebased on master

Created attachment 44384 [details] [review]
Bug 13799: Rename t/.../v1/borrowers.t to t/.../v1/patrons.t

Rebased on master

Created attachment 44385 [details] [review]
Bug 13799: Add types for patron's fields in Swagger spec

Comment on attachment 43998 [details] [review]
[PASSED QA] Bug 13799 - Followup: x-mojo-controller deprecation

Patch moved to bug 15126

Comment on attachment 43999 [details] [review]
[PASSED QA] Bug 13799 - Followup: Refactor spec file

Patch moved to bug 15126

Comment on attachment 44000 [details] [review]
[PASSED QA] Bug 13799: Followup of "Followup: Refactor spec file"

Patch moved to bug 15126

Comment on attachment 44001 [details] [review]
[PASSED QA] Bug 13799: Followup of "Followup: x-mojo-controller deprecation"

Patch moved to bug 15126

Comment on attachment 44285 [details] [review]
Bug 13799: Update dependencies

Patch moved to bug 15126

Comment on attachment 44313 [details]
Dependency tree for Swagger2 0.60

Moved to bug 15126

Re-tested with Swagger2 0.31, Mojolicious 0.54.
Tests pass, all is working well.

I meant: Mojolicious 5.54

Patches pushed to master.

Thanks everyone, specially Julian and Martin!
Things will get interesting for sure :-D

(In reply to Tomás Cohen Arazi from comment #243)
> Patches pushed to master.
> 
> Thanks everyone, specially Julian and Martin!
> Things will get interesting for sure :-D

Whoa! You can say that again!

Is there going to be some documentation for using and developing the REST APIs?

Super intrigued by this!!!

(In reply to David Cook from comment #244)
> Is there going to be some documentation for using and developing the REST
> APIs?
> 

Still wondering about this... 

I see lots of blockers but no coherent documentation of the REST API for Koha...

(In reply to David Cook from comment #245)
> (In reply to David Cook from comment #244)
> > Is there going to be some documentation for using and developing the REST
> > APIs?
> > 
> 
> Still wondering about this... 
> 
> I see lots of blockers but no coherent documentation of the REST API for
> Koha...

/api/v1/spec will give you a json
you can then copy/paste it to editor.swagger.io (file > paste json)

See also bug 14458

I've added https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17694 to create a more human friendly form of the documentation based on the specification file.

Aim is to resemble somthing along the lines of https://ptfs-europe.github.io/rebus-list/api/