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
add support to API for params: "sort_field", "sort_order" #162
Conversation
d9bfe68
to
2cbb221
Compare
2cbb221
to
6ef9970
Compare
So I'm trying to test this with the following URL (on a staging server): https://librivox.org/api/feed/audiobooks?author=twain&limit=2&sort_field=id&sort_order=desc That doesn't seem to change anything. Whatever I put as the sort_order, I get ID 59 first, followed by ID 141. If you're willing to put some time into this, the Ansible playbooks that we use (https://github.com/LibriVox/librivox-ansible) can be used to set up local development environments (so that you can test and iterate locally), and there's an anonymized database dump available to play around with. It's not perfect, but others have done it before. |
Shoot. Thanks for testing. I don't miss PHP. I'll try to figure out how to run this thing in Laragon. update:
update: |
so.. I got a server running at localhost. And.. drum roll.. it worked perfectly for me.
Rather than dumping a bunch of scripts and config files here.. |
Thanks for doing the legwork! Can you post what URL you hit to get those results? I can double check on my end as well, this is very strange indeed. |
The "query" links point to my local server; But with regards to the querystrings.. and the PHP code that produces the output.. nothing has changed in the PR. |
Sorry about the drive-by comment, but I just thought I'd leave a comment to say that this seems to be working on my local as well, I tried a bunch of variations on the link @notartom posted and it all seemed reasonable: https://librivox.org/api/feed/audiobooks?author=twain&limit=2&sort_field=id&sort_order=desc (Note that my localdev is not very standard either though - I'm jamming the ansible stuff into Docker, and there are a few rough edges.) |
OK, not sure what was going I before (I suspect weirdness between Firefox, /etc/hosts, and/or cache), but trying with I think this is fine. I'm a bit weary of changing the API, but as this is purely additive, I think we're OK and are not going to break any existing code that's out there in the wild that's consuming the API. The default sort order is unchanged (it is currently the MariaDB default of ASC); because there's a hardcoded limit of 50 there's no possibility of unintentional DOS by asking to sort all of the books by the un-indexed text field |
That's a good point though, whether the limit is 50 or 10,000, the database will still need to compare all the rows to figure out the sort order if it's not indexed. I don't think any of the new sort fields are indexed |
OK.. looks like I'm late to the party 😃
|
OK, do you mind if for now I push a small change that limits sorting to just the Edit: c9e6c00 |
I'd absolutely understand and agree.. my only question is whether The flip side.. is that if there isn't already an index of this field, then there absolutely should be. |
off topic, but just wondering..
|
When I put some effort into making localdev kinda work, my assumption was that WordPress would be initialized with an empty database. There isn't a lot (or any, really) dev work to be done on the WordPress side. The only custom stuff there is the theme, and we haven't touched that in years. |
gotcha. In that case, I think I'll remove it from the main install script.. and add an optional install script specifically for Wordpress + theme.. same as is the case for PhpMyAdmin. (not that anyone will likely ever use it) update: FWIW, that's all done, and releases has an updated prebuilt server that includes all of the options: PhpMyAdmin, WordPress, and the Librivox Wordpress theme. Seems to work quite well. |
very minor patch that adds some much-needed functionality to the API.
please test this before merging..
my PHP is very rusty..
nearly 20 years rusty..
but it seems like a very straight-forward update.