Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

API 2.0 #958

Merged
merged 3 commits into from
May 21, 2021
Merged

API 2.0 #958

merged 3 commits into from
May 21, 2021

Conversation

paddatrapper
Copy link
Contributor

@paddatrapper paddatrapper commented Jan 30, 2020

This is the implementation of the API 2.0 based on discourse and Mattermost discussions. It is a Django application using a REST framework and is designed to use the database defined by Zend. This ensures that the UI can be migrated over in stages without breaking everything else.

It currently just provides GET requests and I'm pretty sure that authentication does not yet work. I have not attempted to integrate this into the install script or have it run under Vagrant yet. That will have to wait at least until #951 is merged.

I work on it by running Vagrant to get Zend to initialise the database and then changing the PostgresSQL configuration to listen on all interfaces and allow admin access from the vagrant network. The API can then be installed into a Python virtualenv and run on a non-standard port (I use localhost:8083). It can be accessed in the browser from http://localhost:8083. Later on this will be integrated better and be hidden from the user.

After provisioning vagrant and going through the setup, the API can be browsed from http://127.0.0.1:8080/api/v2/

Fixes #1056

@Robbt
Copy link
Member

Robbt commented Feb 24, 2020

This is pretty cool. I'll try to play with it soon.

@paddatrapper
Copy link
Contributor Author

I suggest we follow https://cloud.google.com/apis/design for the structure and design of the API

@paddatrapper

This comment has been minimized.

@paddatrapper
Copy link
Contributor Author

paddatrapper commented Nov 11, 2020

I need to get the api_client python application working with the new API. This would mean that all the python apps have been migrated. I've been working on getting the Now/Next that is pulled into liquidsoap working, but then life got a bit hectic. I'm hoping to be able to get back to it in a week or so.

Once the new API is working with the api_client and integrated into the installer, we can merge this PR and start looking at rewriting parts of the front-end. I would like to be able to do that piecemeal, so that pages can be done individually without requiring an entire UI re-write before things can be merged. That way development isn't stalled while everyone waits for a new UI to develop against.

@paddatrapper
Copy link
Contributor Author

Some of the installation integration is now complete, and development can happen within Vagrant. I have documented the required steps in api/README.md

@paddatrapper
Copy link
Contributor Author

Authentication and writing to the DB both work now. I need to work on the permissions, as they do not work currently.

@Robbt we could potentially replace Propel with an interface to this once it is merged...

@zklosko
Copy link
Member

zklosko commented Nov 30, 2020

@paddatrapper I've been looking into development with Django in general. What do you think about a Django rewrite of Libretime's front end?

@paddatrapper
Copy link
Contributor Author

Eventually, that is how I would serve the frontend, but I don't think templating will be the best way forward here. A lot of what we do involves continuous updating (schedule, now/next, what is playing, input switching, etc), which I think works better with a JavaScript frontend with API. It also ensures that the API works and is actively maintained

@hairmare
Copy link
Member

I'd go for a static vue based SPA (ie a drf first approach) in the long run... This means that the django server will (if at all) only host static files during dev. On production those can be hosted directly by whatever webserver we end up supporting.

From a Vue/frontend PoV we will most likely already start using some Vue components (hosted from PHP) before this aligns with the target architecture where Vue is not only a component but also manages things top-level routing as a real SPA.

@paddatrapper
Copy link
Contributor Author

paddatrapper commented Nov 30, 2020

Yeah, I agree with @hairmare. Initially serve the Vue components from PHP and eventually get to the point where all PHP does is serve Vue. At which point Django can serve the Vue and the PHP can be removed

@paddatrapper
Copy link
Contributor Author

paddatrapper commented Dec 17, 2020

This is now testable without any special vagrant commands. It should setup and install a init script for libretime-api that does things like create required database tables (for tables that are specific to django). I have added unit tests for the custom functionality implemented, but I am waiting for #1125 before I add them to the CI test suite.

There are a couple things to note:

  1. The static files are collected in /usr/share/airtime/api. This is configurable through LIBRETIME_STATIC_ROOT env variable, but this does not change the Apache directory which is actually serving the files. That needs updating manually. It also doesn't change anything before it was set. So if libretime-api collectstatic was run, then the env variable set, collectstatic needs to be run again before anything will be placed in the new directory.
  2. The unit tests assume that there is a working config file /etc/airtime/airtime.cfg or ${LIBRETIME_CONF_DIR}/airtime.cfg. This can be created during the run, but I will do that once I have CI running.
  3. The unit test assume that there is a working postgres server. It creates the tables required and destroys them afterwards for testing. This does not affect any existing database that may be present on the server. I could change the DB to a sqlite3 file or leave it as is if the CI DB works
  4. Django requires libretime-api migrate to be run after install, however this requires access to the DB, so it cannot be run before the DB is created during set up. Is there a way to have it run from the PHP after the DB is created?

@Robbt
Copy link
Member

Robbt commented Dec 17, 2020

PHP can run shell commands but I suspect they will run with the permissions associated with the user that runs the apache process typically www-data. I'm not sure if that would have sufficient permissions but perhaps it could be made to work since it just touches the database.

@paddatrapper
Copy link
Contributor Author

Yeah, provided the user can read the config file, it has enough permissions. The command is libretime-api migrate. I don't know PHP, how do I go about doing that?

@Robbt
Copy link
Member

Robbt commented Dec 17, 2020

Should in theory be as simple as adding the following command in the installer
shell_exec(`libretime-api migrate`);
See https://www.php.net/manual/en/function.shell-exec.php - that is in theory as I haven't tested it.

@paddatrapper
Copy link
Contributor Author

@Robbt thanks. I have added that, though I have no idea how to tell it if actually ran or not...

@zklosko
Copy link
Member

zklosko commented Dec 18, 2020

We can add the command to the final screen of the configuration wizard, where we ask the user to copy and paste commands into the terminal to start necessary services.

@paddatrapper
Copy link
Contributor Author

We could, but I'd rather keep the amount of manual operations to a minimum

@zklosko
Copy link
Member

zklosko commented Dec 19, 2020

It would be nice if we could eventually incorporate the entire setup process into the installer script, so users would only need to touch the terminal once (with exception to troubleshooting).

@paddatrapper
Copy link
Contributor Author

I have implemented guest and admin permissions. It is based on the existing structure and doesn't have any flexibility using Django groups yet (this can be added fairly trivially later once Django manages DB migrations. The API explorer is current only available to admins. This will change as I work on further user types.

@paddatrapper
Copy link
Contributor Author

paddatrapper commented May 10, 2021

I have fixed support for Guests, Programme Managers and Admins. However, I'm running into issues adding support for DJs' 'own' permissions, i.e. permissions that DJs have to edit objects that they own. This is because ownership is represented in a variety of ways and I'm finding I'm having to special-case every one...

For example a DJ has the permission change_own_schedule, which represents their ability to change items in the schedule that are connected to show instances that they are one of the hosts for, while change_own_file represents their ability to change files that has their user in the owner field. I am reluctant to have a if/else set for every one of these types of use cases, but I'm not sure how else to approach it... Essentially the logic needs to determine if the permission being requested requires an own_ substring inserted here

@paddatrapper
Copy link
Contributor Author

I have found a way of doing it by placing the ownership logic in the models. All that is remaining before this is ready to be merged is the rest of the test suite 🎉

@paddatrapper paddatrapper force-pushed the api2.0 branch 2 times, most recently from af98ccf to 33c5d0e Compare May 14, 2021 10:26
@paddatrapper
Copy link
Contributor Author

Finally, after almost 1.5 years, API 2.0 is ready for review and merge! All permissions are implemented and the API gives users direct access to the DB and media files. You can see the API working in the schedule and file download requests made by the API client

@Robbt
Copy link
Member

Robbt commented May 19, 2021

So far looks good on a clean install. I installed and everything worked.

When trying to upgrade an existing debian buster vagrant box by running ./install -fiap
The pypo & pypo-liquidsoap appear to be failing related to the rest/media calls.

2021-05-19 14:16:01,500 [pypofile] [INFO ] copying from http://localhost:8080/rest/media/7 to local cache /var/tmp/airtime/pypo/cache/scheduler/7.mp3 2021-05-19 14:16:01,500 [pypofile] [INFO ] {'id': 7, 'type': 'file', 'metadata': {'id': 7, 'name': '', 'mime': 'audio/mp3', 'ftype': 'audioclip', 'directory': 1, 'filepath': 'imported/1/YouTube.mp3', 'import_status': 0, 'currentlyaccessing': 0, 'editedby': None, 'mtime': '2021-05-19 13:59:43', 'utime': '2021-05-19 13:59:36', 'lptime': None, 'md5': 'dc9bc53d561822e3541f66b05eeb4ebc', 'track_title': 'YouTube.mp3', 'artist_name': None, 'bit_rate': 256000, 'sample_rate': 48000, 'format': None, 'length': '00:01:33.168', 'album_title': None, 'genre': None, 'comments': None, 'year': None, 'track_number': None, 'channels': 2, 'url': None, 'bpm': None, 'rating': None, 'encoded_by': None, 'disc_number': None, 'mood': None, 'label': None, 'composer': None, 'encoder': None, 'checksum': None, 'lyrics': None, 'orchestra': None, 'conductor': None, 'lyricist': None, 'original_lyricist': None, 'radio_station_name': None, 'info_url': None, 'artist_url': None, 'audio_source_url': None, 'radio_station_url': None, 'buy_this_url': None, 'isrc_number': None, 'catalog_number': None, 'original_artist': None, 'copyright': None, 'report_datetime': None, 'report_location': None, 'report_organization': None, 'subject': None, 'contributor': None, 'language': None, 'replay_gain': '-5.27', 'owner_id': 1, 'cuein': '00:00:03.697979', 'cueout': '00:01:33.144958', 'hidden': False, 'filesize': 2982189, 'description': None, 'artwork': '', 'track_type': ''}, 'row_id': 93, 'uri': 'http://localhost:8080/rest/media/7', 'fade_in': 500, 'fade_out': 500, 'cue_in': 3.698, 'cue_out': 93.145, 'start': datetime.datetime(2021, 5, 19, 14, 32, 11), 'end': datetime.datetime(2021, 5, 19, 14, 33, 40), 'show_name': 'Untitled Show', 'replay_gain': -5.27, 'independent_event': False, 'filesize': 2982189, 'file_ext': '.mp3', 'dst': '/var/tmp/airtime/pypo/cache/scheduler/7.mp3', 'file_ready': False} 2021-05-19 14:16:01,516 [pypofile] [ERROR] <Response [401]> 2021-05-19 14:16:01,516 [pypofile] [ERROR] Could not copy from http://localhost:8080/rest/media/7 to /var/tmp/airtime/pypo/cache/scheduler/7.mp3 2021-05-19 14:16:01,516 [pypofile] [ERROR] 401 - Error occurred downloading file

I tried rebooting the vagrant server and that didn't solve anything.
running libretime-api migrate did not fix it.
http://localhost:8080/api/v2/ gives an error and
I see this in zendphp.log -

2021-05-19T14:20:53+00:00 ERR (3): localhost [ErrorController.php:54 - errorAction()] - The requested action is not supported.: Zend_Controller_Action_Exception: Action "v2" does not exist and was not trapped in __call() in /vagrant/vendor/zf1s/zend-controller/library/Zend/Controller/Action.php:485

So I suspect something additional needs to be done for people who are upgrading existing libretime instances before we can merge this.

Can you explain what part of the existing system this API v2 replaces - ie is pypo using the API v1 in theory able to use this w/o any internal modifications.

@Robbt
Copy link
Member

Robbt commented May 19, 2021

Ok I figured out what is going wrong on upgrade, basically the airtime.conf under /etc/apache2/sites-enabled/ is not upgraded which it needs to be, since the new version contains the proxy information to be able to use the Django based API v2.
We have an upgrade variable in the install bash script but it is only triggered by the presence of [media-monitor] - in airtime.conf
This wipes out and replaces the apache config and also the airtime.conf file

There is no way to pass this variable via the command line.
One way we could fix this.

Add a new check in the install to see if one of the proxy statements is missing from the /etc/apache2/sites-enabled/airtime.conf and if it is then to run the script to replace the apache config.

This would work transparent to users that are already used to running ./install -fiap as a means of upgrading their instances.
Choosing to install the Apache config via the install script prompt did not replace airtime.conf

Copy link
Member

@Robbt Robbt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to work well except for the need to trigger installation of the new apache config for people upgrading from an existing libretime install.

@paddatrapper
Copy link
Contributor Author

Can you explain what part of the existing system this API v2 replaces - ie is pypo using the API v1 in theory able to use this w/o any internal modifications.

This replaces the schedule and file download calls made by api_client with v2 equivalent. There is work required to migrate things to use v2 from v1. I plan on doing that incrementally - slowly replacing v1 calls in api_client with v2. Currently there is a version1 client that supports the existing v1 API that is unchanged and a version2 client that supports a subset of calls. The goal is to replace the v1 calls with v2 in a way that applications using api_client do not need to change their calls.

Tangentially, we may want to look at uploading api_client to PyPi as an official API client library for LibreTime so that 3rd-party applications can use it eventually.

@paddatrapper
Copy link
Contributor Author

Installer update has been added. This will transparently update apache config if the user runs ./install -fiap

@paddatrapper paddatrapper requested a review from Robbt May 20, 2021 08:01
@Robbt
Copy link
Member

Robbt commented May 21, 2021

Ok, this fix appears to work. People will need to restart the libretime services except for API after running the install.
I had an issue where I was getting a 500 api error even though everything was working after running the install on a ubuntu-bionic vagrant box but then it worked after halting and restarting the virtualbox.

I've also noticed a false upload error that seems to occur the first time I bulk upload tracks. This may or may not be related to the API change and the track it shows an error on seems to actual work so I don't know what is going on there but I wanted to note it in case it ends up being something.

I think we could merge this and be in good shape. I didn't actually test much of the API functionality itself but it does appear to work for the sections where we starting using the new API.

Does anyone else want to chime in before we merge it ?

@paddatrapper
Copy link
Contributor Author

I've also noticed a false upload error that seems to occur the first time I bulk upload tracks. This may or may not be related to the API change and the track it shows an error on seems to actual work so I don't know what is going on there but I wanted to note it in case it ends up being something.

Can you post the error log for reference in the future?

@paddatrapper
Copy link
Contributor Author

Ah just noticed that the API unit tests aren't actually run in CI. Fixing that

@paddatrapper
Copy link
Contributor Author

Actually, I'm going to leave them out of the CI for the moment and work on getting the GitHub actions CI working. Can integrate it into that without waiting 5 years for a build to complete

@Robbt
Copy link
Member

Robbt commented May 21, 2021

Ok I'm going to merge this as I think it is ready and should work as long as people run install after upgrading to the latest master and we can start to move forward with it. Thanks so much for the work. If I notice the issue with the false error again I'll upload logs. It may just be some kind of AJAX issue and unrelated to this as none of the imports actually failed.

@zklosko
Copy link
Member

zklosko commented Dec 28, 2021

I have a question about the API for @paddatrapper.

As I've been working on building our Vue UI, I've been fiddling with the new API to help me prototype. I've noticed that getting the title of a show on the schedule requires three endpoint requests:

  1. Request ../schedule, retrieve instance_id ->
  2. Request ../show-instances, retrieve show_id ->
  3. Request ../shows, match show_id to return title

This would result in a ton of information getting sent to the client and a long wait time for the three requests to be received. This especially effects the calendar and weekly schedule widgets. Is there a more efficient method being planned or in the works? It seems that the API is returning tables from the database, but no more. It would be helpful if the app could query the dataset, like GraphQL, so only the data needed would be returned and would require minimal parsing on the client.

@paddatrapper
Copy link
Contributor Author

Is there a more efficient method being planned or in the works?

Not currently. That was left as an extension for once we started using the API and experienced where we needed to optimise. We probably should include things like the show_id in the schedule as that is a very common thing that we need

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

New API
4 participants