Bug 20134 - Remove /api/v1/app.pl from the generated URLs
Summary: Remove /api/v1/app.pl from the generated URLs
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST api (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Tomás Cohen Arazi
QA Contact:
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-02-05 13:25 UTC by Tomás Cohen Arazi
Modified: 2018-06-04 20:15 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 20134: Remove /api/v1/app.pl from the generated URLs (1.58 KB, patch)
2018-02-05 18:09 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 20134: Remove /api/v1/app.pl from the generated URLs (1.64 KB, patch)
2018-02-06 08:36 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20134: Remove /api/v1/app.pl from the generated URLs (1.69 KB, patch)
2018-02-07 20:25 UTC, Katrin Fischer
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 2018-02-05 13:25:11 UTC
The REST api is exposed using /api/v1, but that is rewritten to hit /api/v1/app.pl/api/v1 (app.pl being a Mojolicious::Command wrapper for running in Mojolicious under Plack). The problem is that Mojolicious then creates URLs containing that unneeded /api/v1/app.pl prefix. We should strip that out.

To reproduce:
- Point your browser to http://your_host_name/api/v1/
(don't forget about the trailing slash)
=> FAIL: Notice basePath: "/api/v1/app.pl/api/v1"
Comment 1 Tomás Cohen Arazi 2018-02-05 18:09:40 UTC
Created attachment 71239 [details] [review]
Bug 20134: Remove /api/v1/app.pl from the generated URLs

This patch removes the /api/v1/app.pl prefix from generated baseUrl.
It has the consequence of hardcoding / as the base path for the API.

This is ok because we don't currently support mounting Koha pieces in
different than hardcoded paths (/cgi-bin/koha?), but certainly worth
mentioning because this will need to depend on a syspref or config entry
when someone chages this behviour.

To test:
- On master, point your browser to:
  http://kohadev.mydnsname.org:8080/api/v1/
=> FAIL: basePath looks like /api/v1/app.pl/api/v1
- Apply this patch
- Restart memcached and plack:
  $ restart_all
or
  $ sudo systemctl restart memcached && sudo systemctl restart
  koha-common
- Reload http://kohadev.mydnsname.org:8080/api/v1/
=> SUCCESS: basePath is /api/v1
- Sign off :-D
Comment 2 Julian Maurice 2018-02-06 08:35:55 UTC
I just discovered that I can go to http://kohadev.mydnsname.org:8080/api/v1.html to have the API spec in a readable format, which is nice :)
And also that it doesn't work in CGI mode (http://kohadev.mydnsname.org/api/v1/.json works curiously, but not the html version ; etc/koha-httpd.conf probably needs some changes)

Anyway, the patch works, so, sign-off coming soon ;)
Comment 3 Julian Maurice 2018-02-06 08:36:24 UTC
Created attachment 71244 [details] [review]
Bug 20134: Remove /api/v1/app.pl from the generated URLs

This patch removes the /api/v1/app.pl prefix from generated baseUrl.
It has the consequence of hardcoding / as the base path for the API.

This is ok because we don't currently support mounting Koha pieces in
different than hardcoded paths (/cgi-bin/koha?), but certainly worth
mentioning because this will need to depend on a syspref or config entry
when someone chages this behviour.

To test:
- On master, point your browser to:
  http://kohadev.mydnsname.org:8080/api/v1/
=> FAIL: basePath looks like /api/v1/app.pl/api/v1
- Apply this patch
- Restart memcached and plack:
  $ restart_all
or
  $ sudo systemctl restart memcached && sudo systemctl restart
  koha-common
- Reload http://kohadev.mydnsname.org:8080/api/v1/
=> SUCCESS: basePath is /api/v1
- Sign off :-D

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 4 Katrin Fischer 2018-02-07 07:25:55 UTC
I get a not found trying to access:
http://localhost:8080/api/v1

Am I missing something?
Comment 5 Josef Moravec 2018-02-07 10:00:48 UTC
(In reply to Katrin Fischer from comment #4)
> I get a not found trying to access:
> http://localhost:8080/api/v1
> 
> Am I missing something?

Working for me:
http://localhost:8080/api/v1/
Comment 6 Katrin Fischer 2018-02-07 10:08:02 UTC
Josef, feel free to pass QA then - I think my devbox might just be a bit outdated.
Comment 7 Tomás Cohen Arazi 2018-02-07 10:26:12 UTC
(In reply to Katrin Fischer from comment #4)
> I get a not found trying to access:
> http://localhost:8080/api/v1
> 
> Am I missing something?

Yes, the trailing slash.
Comment 8 Katrin Fischer 2018-02-07 10:37:13 UTC
Hah! Will try again in the evening.
Comment 9 Katrin Fischer 2018-02-07 20:25:05 UTC
... and it sure was the trailing slash!
Comment 10 Katrin Fischer 2018-02-07 20:25:54 UTC
Created attachment 71325 [details] [review]
Bug 20134: Remove /api/v1/app.pl from the generated URLs

This patch removes the /api/v1/app.pl prefix from generated baseUrl.
It has the consequence of hardcoding / as the base path for the API.

This is ok because we don't currently support mounting Koha pieces in
different than hardcoded paths (/cgi-bin/koha?), but certainly worth
mentioning because this will need to depend on a syspref or config entry
when someone chages this behviour.

To test:
- On master, point your browser to:
  http://kohadev.mydnsname.org:8080/api/v1/
=> FAIL: basePath looks like /api/v1/app.pl/api/v1
- Apply this patch
- Restart memcached and plack:
  $ restart_all
or
  $ sudo systemctl restart memcached && sudo systemctl restart
  koha-common
- Reload http://kohadev.mydnsname.org:8080/api/v1/
=> SUCCESS: basePath is /api/v1
- Sign off :-D

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 11 Jonathan Druart 2018-02-09 17:09:19 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 12 Nick Clemens 2018-02-16 02:32:51 UTC
Awesome work all, pushed to stable for 17.11.03